-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implement expect attribute from RFC 2383, "Lint Reasons RFC" #85549
Comments
Here are some high-level mentoring instructions. Each of these could be a commit or even a PR (I like doing things in stages):
Probably a good next step is to start looking into the lint code or reading the rustc-dev-guide section on lints. This is going to have to be a bit different than the existing lint infrastructure, though. The way that lints normally work is:
The challenge here is that this not an ordinary lint level. Hmm, this is going to take some thought! We have to find the best way to integrate this into the incremental system, and it's actually kind of non-trivial. I'll ponder it a bit. |
cc @xFrednet -- feel free to "rustbot claim" this issue |
Very, very awesome. Thank you very much for taking up the role of mentoring me! @nikomatsakis I've started to work on this implementation partially already by adding the The feature flag has already been implemented as The next step is not to actually track the expectations. My plan looks roughly like this:
What do you think about this plan? My main road block is that I'm not sure where to store the expectation-collection (Step 2). I first tried to save them in the Some background about my previous work: I'm a contributor to Clippy and fairly fluent when it comes to the diagnostic interfaces. The internals are a bit new though. @rustbot claim |
I opened a topic on Zulip to discuss how to manage the integration into the incremental system: https://zulip-archive.rust-lang.org/241847tcompilerwgincrcomp/33135implementingtheexpectRFC.html |
Okay, I'm sadly not very knowledgeable when it comes to rust's incremental compilation. So, I'll most likely only be a side reader in the stream for now. For reference, is there a state or session saved between the compilation runs? |
@xFrednet I recommend reading about the query system and the incremental system |
I'm starting to see the problem with my idea. Then let's wait for a bit to see if someone suggests something on Zulip or if either one of us gets an idea. 🤔 |
Hey reader, this is a quick breakdown of what I found while digging though the compiler to find a starting point for this implementation. It's probably only interesting if you plan to implement this or some other central diagnostic tracking thingy. Details diagnostic emission breakdownCollapsed detailed breakdownThis is just a quick breakdown and not really proofread as I don't expect it to read in details. So please excuse any typos 😅. This will also focus on lints but it should be pretty much the same for other diagnostics. Incremental compilation diagnosticsSo, first of all it was mentioned on Zulip that "Diagnostics are already saved and replayed by the incr. comp. engine.". The code for the replay can be found here 1. The loading does a bunch of magic, but the important part is that it emits the loaded diagnostics via Normal lint emissionThe second question was how lints end up being added to the cache of the incremental compilation. I won't show the complete call stack because it is all over the place and not very helpful, but there are a few important points here:
|
…th-ids, r=wesleywiser Implementation of the `expect` attribute (RFC 2383) This is an implementation of the `expect` attribute as described in [RFC-2383](https://rust-lang.github.io/rfcs/2383-lint-reasons.html). The attribute allows the suppression of lint message by expecting them. Unfulfilled lint expectations (meaning no expected lint was caught) will emit the `unfulfilled_lint_expectations` lint at the `expect` attribute. ### Example #### input ```rs // required feature flag #![feature(lint_reasons)] #[expect(unused_mut)] // Will warn about an unfulfilled expectation #[expect(unused_variables)] // Will be fulfilled by x fn main() { let x = 0; } ``` #### output ```txt warning: this lint expectation is unfulfilled --> $DIR/trigger_lint.rs:3:1 | LL | #[expect(unused_mut)] // Will warn about an unfulfilled expectation | ^^^^^^^^^^ | = note: `#[warn(unfulfilled_lint_expectations)]` on by default ``` ### Implementation This implementation introduces `Expect` as a new lint level for diagnostics, which have been expected. All lint expectations marked via the `expect` attribute are collected in the [`LintLevelsBuilder`] and assigned an ID that is stored in the new lint level. The `LintLevelsBuilder` stores all found expectations and the data needed to emit the `unfulfilled_lint_expectations` in the [`LintLevelsMap`] which is the result of the [`lint_levels()`] query. The [`rustc_errors::HandlerInner`] is the central error handler in rustc and handles the emission of all diagnostics. Lint message with the level `Expect` are suppressed during this emission, while the expectation ID is stored in a set which marks them as fulfilled. The last step is then so simply check if all expectations collected by the [`LintLevelsBuilder`] in the [`LintLevelsMap`] have been marked as fulfilled in the [`rustc_errors::HandlerInner`]. Otherwise, a new lint message will be emitted. The implementation of the `LintExpectationId` required some special handling to make it stable between sessions. Lints can be emitted during [`EarlyLintPass`]es. At this stage, it's not possible to create a stable identifier. The level instead stores an unstable identifier, which is later converted to a stable `LintExpectationId`. ### Followup TO-DOs All open TO-DOs have been marked with `FIXME` comments in the code. This is the combined list of them: * [ ] The current implementation doesn't cover cases where the `unfulfilled_lint_expectations` lint is actually expected by another `expect` attribute. * This should be easily possible, but I wanted to get some feedback before putting more work into this. * This could also be done in a new PR to not add to much more code to this one * [ ] Update unstable documentation to reflect this change. * [ ] Update unstable expectation ids in [`HandlerInner::stashed_diagnostics`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.HandlerInner.html#structfield.stashed_diagnostics) ### Open questions I also have a few open questions where I would like to get feedback on: 1. The RFC discussion included a suggestion to change the `expect` attribute to something else. (Initiated by `@Ixrec` [here](rust-lang/rfcs#2383 (comment)), suggestion from `@scottmcm` to use `#[should_lint(...)]` [here](rust-lang/rfcs#2383 (comment))). No real conclusion was drawn on that point from my understanding. Is this still open for discussion, or was this discarded with the merge of the RFC? 2. How should the expect attribute deal with the new `force-warn` lint level? --- This approach was inspired by a discussion with `@LeSeulArtichaut.` RFC tracking issue: rust-lang#54503 Mentoring/Implementation issue: rust-lang#85549 [`LintLevelsBuilder`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/levels/struct.LintLevelsBuilder.html [`LintLevelsMap`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/lint/struct.LintLevelMap.html [`lint_levels()`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.lint_levels [`rustc_errors::HandlerInner`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.HandlerInner.html [`EarlyLintPass`]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.EarlyLintPass.html
…-party, r=flip1995,wesleywiser Improve `expect` impl and handle `#[expect(unfulfilled_lint_expectations)]` (RFC 2383) 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](https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-levels) 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 a `unfulfilled_lint_expectations` diagnostic with an additional note. --- r? `@wesleywiser` I'm requesting a review from you since you reviewed the previous PR rust-lang#87835. You are welcome to reassign it if you're busy 🙃 rfc: [RFC-2383](https://rust-lang.github.io/rfcs/2383-lint-reasons.html) tracking issue: rust-lang#85549 cc: `@flip1995` In case you're also interested in this :)
…esleywiser Support tool lints with the `#[expect]` attribute (RFC 2383) This PR fixes the ICE rust-lang#94953 by making the assert for converted expectation IDs conditional. Additionally, it moves the lint expectation check into a separate query to support rustdoc and other tools. On the way, I've also added some tests to ensure that the attribute works for Clippy and rustdoc lints. The number of changes comes from the long test file. This may look like a monster PR, this may smell like a monster PR and this may be a monster PR, but it's a harmless monster. 🦕 --- Closes: rust-lang#94953 cc: rust-lang#85549 r? `@wesleywiser` cc: `@rust-lang/rustdoc`
…esleywiser Support tool lints with the `#[expect]` attribute (RFC 2383) This PR fixes the ICE rust-lang#94953 by making the assert for converted expectation IDs conditional. Additionally, it moves the lint expectation check into a separate query to support rustdoc and other tools. On the way, I've also added some tests to ensure that the attribute works for Clippy and rustdoc lints. The number of changes comes from the long test file. This may look like a monster PR, this may smell like a monster PR and this may be a monster PR, but it's a harmless monster. 🦕 --- Closes: rust-lang#94953 cc: rust-lang#85549 r? ``@wesleywiser`` cc: ``@rust-lang/rustdoc``
…esleywiser Support tool lints with the `#[expect]` attribute (RFC 2383) This PR fixes the ICE rust-lang#94953 by making the assert for converted expectation IDs conditional. Additionally, it moves the lint expectation check into a separate query to support rustdoc and other tools. On the way, I've also added some tests to ensure that the attribute works for Clippy and rustdoc lints. The number of changes comes from the long test file. This may look like a monster PR, this may smell like a monster PR and this may be a monster PR, but it's a harmless monster. 🦕 --- Closes: rust-lang#94953 cc: rust-lang#85549 r? ```@wesleywiser``` cc: ```@rust-lang/rustdoc```
…leywiser Support tool lints with the `#[expect]` attribute (RFC 2383) This PR fixes the ICE rust-lang#94953 by making the assert for converted expectation IDs conditional. Additionally, it moves the lint expectation check into a separate query to support rustdoc and other tools. On the way, I've also added some tests to ensure that the attribute works for Clippy and rustdoc lints. The number of changes comes from the long test file. This may look like a monster PR, this may smell like a monster PR and this may be a monster PR, but it's a harmless monster. 🦕 --- Closes: rust-lang#94953 cc: rust-lang#85549 r? `@wesleywiser` cc: `@rust-lang/rustdoc`
Support tool lints with the `#[expect]` attribute (RFC 2383) This PR fixes the ICE rust-lang/rust#94953 by making the assert for converted expectation IDs conditional. Additionally, it moves the lint expectation check into a separate query to support rustdoc and other tools. On the way, I've also added some tests to ensure that the attribute works for Clippy and rustdoc lints. The number of changes comes from the long test file. This may look like a monster PR, this may smell like a monster PR and this may be a monster PR, but it's a harmless monster. 🦕 --- Closes: rust-lang/rust#94953 cc: rust-lang/rust#85549 r? `@wesleywiser` cc: `@rust-lang/rustdoc`
…g, r=wesleywiser Support the `#[expect]` attribute on fn parameters (RFC-2383) A small PR to allow the `#[expect]` attribute on function parameters. Nothing more to say, I hope everyone reading this has a lovely day. --- r? `@wesleywiser` closes: rust-lang#97650 cc: rust-lang#85549
…g, r=wesleywiser Support the `#[expect]` attribute on fn parameters (RFC-2383) A small PR to allow the `#[expect]` attribute on function parameters. Nothing more to say, I hope everyone reading this has a lovely day. --- r? ``@wesleywiser`` closes: rust-lang#97650 cc: rust-lang#85549
…e-for-expect, r=wesleywiser Fix `delayed_good_path_bug` ice for expected diagnostics (RFC 2383) Fixes a small ICE with the `delayed_good_path_bug` check. --- r? `@wesleywiser` cc: `@eddyb` this might be interesting, since you've added a `FIXME` comment above the modified check which kind of discusses a case like this closes: rust-lang#95540 cc: rust-lang#85549
…e-for-expect, r=wesleywiser Fix `delayed_good_path_bug` ice for expected diagnostics (RFC 2383) Fixes a small ICE with the `delayed_good_path_bug` check. --- r? ``@wesleywiser`` cc: ``@eddyb`` this might be interesting, since you've added a `FIXME` comment above the modified check which kind of discusses a case like this closes: rust-lang#95540 cc: rust-lang#85549
…-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.
…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/rust#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/rust#87835 Issue: rust-lang/rust#85549 Yeah, and that's it.
…n-magic, r=wesleywiser Finishing touches for `#[expect]` (RFC 2383) This PR adds documentation and some functionality to rustc's lint passes, to manually fulfill expectations. This is needed for some lints in Clippy. Hopefully, it should be one of the last things before we can move forward with stabilizing this feature. As part of this PR, I've also updated `clippy::duplicate_mod` to showcase how this new functionality can be used and to ensure that it works correctly. --- changelog: [`duplicate_mod`]: Fixed lint attribute interaction r? `@wesleywiser` cc: rust-lang#97660, rust-lang#85549 And I guess that's it. Here have a magical unicorn 🦄
…n-magic, r=wesleywiser Finishing touches for `#[expect]` (RFC 2383) This PR adds documentation and some functionality to rustc's lint passes, to manually fulfill expectations. This is needed for some lints in Clippy. Hopefully, it should be one of the last things before we can move forward with stabilizing this feature. As part of this PR, I've also updated `clippy::duplicate_mod` to showcase how this new functionality can be used and to ensure that it works correctly. --- changelog: [`duplicate_mod`]: Fixed lint attribute interaction r? ``@wesleywiser`` cc: rust-lang#97660, rust-lang#85549 And I guess that's it. Here have a magical unicorn 🦄
…n-magic, r=wesleywiser Finishing touches for `#[expect]` (RFC 2383) This PR adds documentation and some functionality to rustc's lint passes, to manually fulfill expectations. This is needed for some lints in Clippy. Hopefully, it should be one of the last things before we can move forward with stabilizing this feature. As part of this PR, I've also updated `clippy::duplicate_mod` to showcase how this new functionality can be used and to ensure that it works correctly. --- changelog: [`duplicate_mod`]: Fixed lint attribute interaction r? ```@wesleywiser``` cc: rust-lang#97660, rust-lang#85549 And I guess that's it. Here have a magical unicorn 🦄
…n-magic, r=wesleywiser Finishing touches for `#[expect]` (RFC 2383) This PR adds documentation and some functionality to rustc's lint passes, to manually fulfill expectations. This is needed for some lints in Clippy. Hopefully, it should be one of the last things before we can move forward with stabilizing this feature. As part of this PR, I've also updated `clippy::duplicate_mod` to showcase how this new functionality can be used and to ensure that it works correctly. --- changelog: [`duplicate_mod`]: Fixed lint attribute interaction r? `@wesleywiser` cc: rust-lang#97660, rust-lang#85549 And I guess that's it. Here have a magical unicorn 🦄
…r=wesleywiser Finishing touches for `#[expect]` (RFC 2383) This PR adds documentation and some functionality to rustc's lint passes, to manually fulfill expectations. This is needed for some lints in Clippy. Hopefully, it should be one of the last things before we can move forward with stabilizing this feature. As part of this PR, I've also updated `clippy::duplicate_mod` to showcase how this new functionality can be used and to ensure that it works correctly. --- changelog: [`duplicate_mod`]: Fixed lint attribute interaction r? `@wesleywiser` cc: rust-lang/rust#97660, rust-lang/rust#85549 And I guess that's it. Here have a magical unicorn 🦄
I'm closing this, since the feature has been implemented :D |
Huzzah!
…On Wed, Jun 26, 2024, at 4:15 PM, Fridtjof Stoldt wrote:
I'm closing this, since the feature has been implemented :D
—
Reply to this email directly, view it on GitHub <#85549 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZW5E2VUXSVGZDO6TGLZJMOPPAVCNFSM6AAAAABJ6SRDU2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJSGU2DSMBQHA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
This issue is part of #54503. It is specifically focused on implementing the
#[expect]
attribute for lints.A working lint implementation can be found in #87835. The implementation still left the to-dos open for follow-up PRs:
#[expect(unfulfilled_lint_expectations)]
-- Improveexpect
impl and handle#[expect(unfulfilled_lint_expectations)]
(RFC 2383) #94670HandlerInner::stashed_diagnostics
-- Improveexpect
impl and handle#[expect(unfulfilled_lint_expectations)]
(RFC 2383) #94670#[expect()]
work forrustdoc::
lints Support tool lints with the#[expect]
attribute (RFC 2383) #95542delayed_good_path_bugs
issued#![feature(lint_reasons)]
#95540#[expect(..)]
not supported on function arguments #97650--force-warn
with expect (Support lint expectations for--force-warn
lints (RFC 2383) #97757)#[expect(...)]
doen't catch lints from differentcheck_*
function #97660confusable_idents
work with#[expect]
currently, the lint is emitted and doesn't fulfill the expectation.The open questions will be asked in the RFC Tracking issue 🙃
The text was updated successfully, but these errors were encountered: