-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Improve expect
impl and handle #[expect(unfulfilled_lint_expectations)]
(RFC 2383)
#94670
Improve expect
impl and handle #[expect(unfulfilled_lint_expectations)]
(RFC 2383)
#94670
Conversation
This comment has been minimized.
This comment has been minimized.
2ae4720
to
e97c9fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still a bit concerned about the assumption that is made here, that unfulfilled_lint_expectations
is not in a lint group. This is not statically enforced and might lead to surprising breakages in the future. Can we somehow add a test for this?
I assume warnings
is not a real lint group, right? Otherwise the changes in this PR could already fail with that.
Very good suggestions, thank you for the review!
I'm not sure how this can be tested. The current implementation doesn't break when a lint group includes the
I've added a test with I've rebased and addressed your feedback 🙃 |
76d97de
to
16b7fbf
Compare
16b7fbf
to
d39d609
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how this can be tested.
Me neither. The only thing, that could be done would be to use ids.iter().find(..)
to make sure that this doesn't happen. But this might have a performance impact and is in the current state unnecessary, because the unfulfilled_lint_expectations
lint is currently not in a group.
Maybe I'm overthinking this. I mean it is really unlikely that this lint will ever get added to a group.
#[expect(warnings, reason = "this suppresses all warnings and also suppresses itself. No warning will be issued")] | ||
pub fn expect_warnings() { | ||
// This lint trigger will be suppressed | ||
#[warn(unused_mut)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wondered why this doesn't warn here. It should override theexpect
/allow
of the outer scope. But it seems like warnings
is somehow treated differently to other lint groups. It behaves the same as with allow
, see Playground, so this is not a bug in the expect
attribute.
But I would argue, that this is generally surprising behavior. Is this documented somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the warnings
name seems to behave a bit differently. It looks like it sets the level for all warnings in a given scope. I only could find this documentation:
source and this. But both say that it behaves like a lint group and not overrides the level. I think this is fine for now as it behaves like the allow
attribute 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine for now
Agreed. Nothing to do in this PR. I just wanted to point out this behavior, because I thought it is generally unexpected/surprising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the added comment, this LGTM. I'll let @wesleywiser give this another look, but after that r=me.
// This checks for instances where the user writes `#[expect(unfulfilled_lint_expectations)]` | ||
// in that case we want to avoid overriding the lint level but instead add an expectation that | ||
// can't be fulfilled. The lint message will include an explanation, that the | ||
// `unfulfilled_lint_expectations` lint can't be expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior makes sense to me (as we discussed in the last PR) but before stabilization, we should decide if this is the correct approach. After stabilization, changing the behavior here will be potentially breaking. If we make this a hard error, then we can always relax that later backwards-compatibly.
@bors r=flip1995,wesleywiser |
📌 Commit be84049 has been approved by |
…askrgr Rollup of 5 pull requests Successful merges: - rust-lang#90621 (Stabilise `aarch64_target_feature`) - rust-lang#93977 (Type params and assoc types have unit metadata if they are sized) - rust-lang#94670 (Improve `expect` impl and handle `#[expect(unfulfilled_lint_expectations)]` (RFC 2383)) - rust-lang#94884 (Fix remaining meta-variable expression TODOs) - rust-lang#94931 (update miri) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This PR updates unstable
ExpectationIds
in stashed diagnostics and adds some asserts to ensure that the stored expectations are really empty in the end. Additionally, it handles the#[expect(unfulfilled_lint_expectations)]
case.According to the Errors and lints docs the
error
level should only be used "when the compiler detects a problem that makes it unable to compile the program". As this isn't the case with#[expect(unfulfilled_lint_expectations)]
I decided to only create a warning. To avoid adding a new lint only for this case, I simply emit aunfulfilled_lint_expectations
diagnostic with an additional note.r? @wesleywiser I'm requesting a review from you since you reviewed the previous PR #87835. You are welcome to reassign it if you're busy 🙃
rfc: RFC-2383
tracking issue: #85549
cc: @flip1995 In case you're also interested in this :)