Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IMPLIED_BOUNDS_ENTAILMENT lint #105575

Merged
merged 4 commits into from
Dec 20, 2022
Merged

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Dec 11, 2022

Implements a lint (#105572) version of the hard-error introduced in #105483. Context is in that PR.

r? @lcnr
cc @oli-obk who had asked for this to be a lint first

Not sure if this needs to be an FCP, since it's a lint for now.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 11, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Dec 11, 2022

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 11, 2022
@bors
Copy link
Contributor

bors commented Dec 11, 2022

⌛ Trying commit b85a28fbbf85acf442b5c57c45ad1d563fd2e938 with merge 9490fcf9c4ecf3ed0b3ae1a007f2f8e5b26d5692...

@bors
Copy link
Contributor

bors commented Dec 12, 2022

☀️ Try build successful - checks-actions
Build commit: 9490fcf9c4ecf3ed0b3ae1a007f2f8e5b26d5692 (9490fcf9c4ecf3ed0b3ae1a007f2f8e5b26d5692)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9490fcf9c4ecf3ed0b3ae1a007f2f8e5b26d5692): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Warning ⚠: The following benchmark(s) failed to build:

  • rustc

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.9% [0.3%, 2.8%] 29
Regressions ❌
(secondary)
1.5% [1.0%, 2.0%] 7
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.9% [0.3%, 2.8%] 29

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.6%, 0.6%] 1
Regressions ❌
(secondary)
2.4% [1.8%, 2.9%] 2
Improvements ✅
(primary)
-1.3% [-2.2%, -0.5%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.8% [-2.2%, 0.6%] 4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.8% [1.2%, 2.7%] 13
Regressions ❌
(secondary)
1.9% [1.9%, 1.9%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.8% [1.2%, 2.7%] 13

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 12, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2022
…yn514

Fix unsoundness in bootstrap cache code

Discovered via rust-lang#105575, which showed that rustc was failing to build during a perf run.
@compiler-errors
Copy link
Member Author

@lcnr told me r=me on Zulip after updating the tracking issue (#105572) to be more descriptive.

We also discussed this during the compiler team meeting (starting here) and decided the perf hit is acceptable, as this lint will be removed and turned into a hard error hopefully in a few versions.

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Dec 15, 2022

📌 Commit 49176abdd7ff51b3433a072222895785758951b7 has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 15, 2022
@bors
Copy link
Contributor

bors commented Dec 16, 2022

⌛ Testing commit 49176abdd7ff51b3433a072222895785758951b7 with merge c09c28be814c4b2d4a851edbcfcec79ef3e90cb8...

@bors
Copy link
Contributor

bors commented Dec 16, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 16, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@compiler-errors
Copy link
Member Author

@bors r=lcnr lol let's try this again

@bors
Copy link
Contributor

bors commented Dec 20, 2022

⌛ Testing commit 8c86773 with merge 7f42e58...

@bors
Copy link
Contributor

bors commented Dec 20, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 7f42e58 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 20, 2022
@bors bors merged commit 7f42e58 into rust-lang:master Dec 20, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 20, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7f42e58): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.9% [0.2%, 2.3%] 20
Regressions ❌
(secondary)
1.6% [0.6%, 2.6%] 12
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.9% [0.2%, 2.3%] 20

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.5% [-3.5%, -3.5%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.3% [0.9%, 2.1%] 6
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.3% [0.9%, 2.1%] 6

@oli-obk
Copy link
Contributor

oli-obk commented Dec 20, 2022

@rustbot label: +perf-regression-triaged

This regression was anticipated and accepted by the compiler team and types team on the grounds that we'll get this perf back once the future incompat lint becomes a hard error.

Also: this addresses a soundness issue

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 20, 2022
@jackh726
Copy link
Member

I'm going to add relnotes here. It's not certain at what stage this lint will be (still warn or upgraded to deny) at release time, but given the size of breakages reported by crater, I think it would be good to point this out.

@jackh726 jackh726 added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 20, 2022
@apiraino
Copy link
Contributor

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Dec 22, 2022
@Mark-Simulacrum
Copy link
Member

@compiler-errors This doesn't seem to backport cleanly -- can you prepare a PR targeting the beta branch?

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 28, 2022
…AILMENT, r=Mark-Simulacrum

Backport `IMPLIED_BOUNDS_ENTAILMENT` lint

r? `@Mark-Simulacrum` as requested rust-lang#105575 (comment)
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 29, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 5, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 13, 2023
…S_ENTAILMENT, r=lcnr

Bump `IMPLIED_BOUNDS_ENTAILMENT` to Deny + ReportNow

rust-lang#105575 (comment)

> and then later in the same cycle increase the lint to `deny` and change it to `FutureCompatReportNow` in this nightly cycle.

r? `@lcnr` when they're back from holiday 😄
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 13, 2023
…S_ENTAILMENT, r=lcnr

Bump `IMPLIED_BOUNDS_ENTAILMENT` to Deny + ReportNow

rust-lang#105575 (comment)

> and then later in the same cycle increase the lint to `deny` and change it to `FutureCompatReportNow` in this nightly cycle.

r? ``@lcnr`` when they're back from holiday 😄
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 13, 2023
…S_ENTAILMENT, r=lcnr

Bump `IMPLIED_BOUNDS_ENTAILMENT` to Deny + ReportNow

rust-lang#105575 (comment)

> and then later in the same cycle increase the lint to `deny` and change it to `FutureCompatReportNow` in this nightly cycle.

r? ```@lcnr``` when they're back from holiday 😄
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 19, 2023
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.68.0, 1.67.0 Jan 19, 2023
@compiler-errors compiler-errors deleted the impl-wf-lint branch August 11, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.