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

Support lint expectations for --force-warn lints (RFC 2383) #97757

Merged
merged 1 commit into from
Jun 16, 2022

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Jun 5, 2022

Rustc has a --force-warn flag, which overrides lint level attributes and forces the diagnostics to always be warn. This means, that for lint expectations, the diagnostic can't be suppressed as usual. This also means that the expectation would not be fulfilled, even if a lint had been triggered in the expected scope.

This PR now also tracks the expectation ID in the ForceWarn level. I've also made some minor adjustments, to possibly catch more bugs and make the whole implementation more robust.

This will probably conflict with #97718. That PR should ideally be reviewed and merged first. The conflict itself will be trivial to fix.


r? @wesleywiser

cc: @flip1995 since you've helped with the initial review and also discussed this topic with me. 🙃

Follow-up of: #87835

Issue: #85549

Yeah, and that's it.

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/rustfmt.

cc @rust-lang/rustfmt

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 5, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2022
@xFrednet

This comment was marked as off-topic.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 5, 2022
@xFrednet

This comment was marked as off-topic.

@rust-timer

This comment was marked as off-topic.

@bors

This comment was marked as off-topic.

@rust-lang rust-lang deleted a comment from rust-timer Jun 5, 2022
@bors
Copy link
Contributor

bors commented Jun 5, 2022

☀️ Try build successful - checks-actions
Build commit: 8608dd3a0505ab79dd32b8873a7df850c163b711 (8608dd3a0505ab79dd32b8873a7df850c163b711)

@rust-timer

This comment was marked as off-topic.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8608dd3a0505ab79dd32b8873a7df850c163b711): comparison url.

Instruction count

  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-0.6% -0.6% 1
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -0.6% -0.6% 1

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)
3.8% 3.8% 1
Regressions 😿
(secondary)
3.4% 4.1% 2
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-3.7% -3.7% 1
All 😿🎉 (primary) 3.8% 3.8% 1

Cycles

This benchmark run did not return any relevant results for this metric.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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.

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

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 5, 2022
@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 5, 2022
@xFrednet
Copy link
Member Author

xFrednet commented Jun 5, 2022

It looks like hiding the comment triggered rust-timer again. :/

@xFrednet xFrednet force-pushed the rfc-2383-expect-with-force-warn branch from 61a746c to ebbe09c Compare June 10, 2022 18:59
@xFrednet
Copy link
Member Author

Alright, I've rebased on master. This PR should also be ready for review now. The performance run should still be valid.

As a side note, I love how stable the CI is. I can just rebase on master and expect everything to be green. This is so beautiful!

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few comments about the documentation and a missing test. Impl LGTM

compiler/rustc_errors/src/lib.rs Show resolved Hide resolved
compiler/rustc_lint/src/levels.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/levels.rs Show resolved Hide resolved
@xFrednet xFrednet force-pushed the rfc-2383-expect-with-force-warn branch from ebbe09c to 16ecd42 Compare June 14, 2022 05:41
@xFrednet
Copy link
Member Author

Nice catches, thank you for the review @flip1995!

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Since it touches the compiler I'll leave the final r+ to Wesley again.

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

r=me with that one nit addressed

compiler/rustc_lint/src/levels.rs Outdated Show resolved Hide resolved
@xFrednet xFrednet force-pushed the rfc-2383-expect-with-force-warn branch from 16ecd42 to 8527a3d Compare June 16, 2022 06:17
@xFrednet
Copy link
Member Author

Awesome, thank you for the review! I think we have two smaller things that need to be updated, and then this feature should be complete 🥳

@bors r=wesleywiser,flip1995 rollup=always

@bors
Copy link
Contributor

bors commented Jun 16, 2022

📌 Commit 8527a3d has been approved by wesleywiser,flip1995

@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 Jun 16, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2022
…askrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#97757 (Support lint expectations for `--force-warn` lints (RFC 2383))
 - rust-lang#98125 (Entry and_modify doc)
 - rust-lang#98137 (debuginfo: Fix NatVis for Rc and Arc with unsized pointees.)
 - rust-lang#98147 (Make #[cfg(bootstrap)] not error in proc macros on later stages )

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 95be954 into rust-lang:master Jun 16, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 16, 2022
@xFrednet xFrednet deleted the rfc-2383-expect-with-force-warn branch June 16, 2022 10:16
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 23, 2022
…-warn, r=wesleywiser,flip1995

Support lint expectations for `--force-warn` lints (RFC 2383)

Rustc has a `--force-warn` flag, which overrides lint level attributes and forces the diagnostics to always be warn. This means, that for lint expectations, the diagnostic can't be suppressed as usual. This also means that the expectation would not be fulfilled, even if a lint had been triggered in the expected scope.

This PR now also tracks the expectation ID in the `ForceWarn` level. I've also made some minor adjustments, to possibly catch more bugs and make the whole implementation more robust.

This will probably conflict with rust-lang#97718. That PR should ideally be reviewed and merged first. The conflict itself will be trivial to fix.

---

r? `@wesleywiser`

cc: `@flip1995` since you've helped with the initial review and also discussed this topic with me. 🙃

Follow-up of: rust-lang#87835

Issue: rust-lang#85549

Yeah, and that's it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-perf Status: Waiting on a perf run to be completed. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants