Implement expect attribute from RFC 2383, "Lint Reasons RFC"

[nikomatsakis]

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:

• Handle #[expect(unfulfilled_lint_expectations)] -- Improve expect impl and handle #[expect(unfulfilled_lint_expectations)] (RFC 2383) #94670
• Update unstable expectation ids in HandlerInner::stashed_diagnostics -- Improve expect impl and handle #[expect(unfulfilled_lint_expectations)] (RFC 2383) #94670
• Make #[expect()] work for rustdoc:: lints Support tool lints with the #[expect] attribute (RFC 2383) #95542
• ICE: no warnings or errors encountered even though delayed_good_path_bugs issued #![feature(lint_reasons)] #95540
• #[expect(..)] not supported on function arguments #97650
• Handle --force-warn with expect (Support lint expectations for --force-warn lints (RFC 2383) #97757)
• #[expect(...)] doen't catch lints from different check_* function #97660
• Make confusable_idents work with #[expect] currently, the lint is emitted and doesn't fulfill the expectation.
• Update unstable documentation to reflect this change.

The open questions will be asked in the RFC Tracking issue 🙃

[nikomatsakis]

Here are some high-level mentoring instructions. Each of these could be a commit or even a PR (I like doing things in stages):

• Create a feature gate lint_reasons per the directions here
• Create some ui tests covering the expected behavior (instructions)
  • #[expect(dead_code)] on a dead function, for example
  • #[expect(dead_code)] on a live function, for example
  • you can add in the incorrect behavior and use --bless and add a FIXME(Implement expect attribute from RFC 2383, "Lint Reasons RFC" #85549) into the test

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:

• when you would generate a lint, you lookup the lint level using the lint_levels query.
• once you know the level, you can emit the lint

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.

[nikomatsakis]

cc @xFrednet -- feel free to "rustbot claim" this issue

[xFrednet]

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 expect attribute as a lint level. Similar to how @JohnTitor has done it in this commit. I was debating if this should actually be a lint level at all. I believe that it makes sense and this also allows the reuse of the LintLevelsBuilder. In either case it will be good enough for a prototype and later adjustment.

The feature flag has already been implemented as lint_reasons. The name might not be descriptive but splitting them into two features where the expect-feature requires the reasoning feature would be weird.

The next step is not to actually track the expectations. My plan looks roughly like this:

1. Collect all expectations (This can be done by the LintLevelsBuilder or a more specific walker if needed)
2. Save these expectations in an accessible manner
3. Now we start the normal lint passes
   1. This includes all lint passes
   2. If a lint is emitted it will check which level is set for it in the specific context
   3. If the level is expect it will be suppressed and notify the expectation-collection that
      a lint has been issued at this location
4. After all lint passes are done it will be checked if the collected expectations have been met. A message will be issued if not.

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 LintLevelsBuilder but the struct gets recreated for every lint emission and is therefor not really a possibility. My current sights are either on storing them in the Session or the LintStore but both feel not 100% correct 🤔

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

[nikomatsakis]

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

[xFrednet]

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?

[nikomatsakis]

@xFrednet I recommend reading about the query system and the incremental system

[xFrednet]

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. 🤔

[xFrednet]

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 breakdown

Collapsed detailed breakdown

This 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 diagnostics

So, 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 rustc_errors::Handler::emit_diagnostic with an instance that is stored in the current session. This means we can collect emitted lints from that point forward.

Normal lint emission

The 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:

1. Entry point: The actual lint emission start by calling struct_span_lint on the current LintContext it also passes a closure with it called decorate, that is used to create the lint message at the end.
2. The lint information and closure get passed around until it ends up in rustc_middle::lint::struct_lint_level(lint, lint_level, span, decorate, ..). Now here is something important we return if the lint_level is Allow.
   1. This is important as it means that we don't track and therefor not save allowed lints between sessions. The interesting question would be if query system is sensitive to this and relints the module if the passed console arguments about lint level change. This is more a side note for later testing :)
   2. Anyways... There are some further checks and additions which are all not that important, and then we call the decorate closure with a DiagnosticBuilder.
3. Inside the decorate closure: The passed DiagnosticBuilder is used to add additional information etc. The builder requires the call of DiagnosticBuilder::cancelled() which would simply cancel the emission or DiagnosticBuilder::emit() (The builder will panic during the drop otherwise, so nice protection there!)
4. Following the branch of DiagnosticBuilder::emit() leads us to rustc_error::HandlerInner::emit_diagnostic(diag) and this is where the magic happen and where we rejoin the diagnostics that have been loaded by the incremental compilation implementation.

rustc_error::HandlerInner::emit_diagnostic(diag)

The function can be found here 2.

What it does:

1. It calls (*TRACK_DIAGNOSTICS)(diagnostic); with diagnostic being the created diagnostic from the builder.
   1. TRACK_DIAGNOSTICS is a callback to here 3
   2. This callback adds the diagnostic to the collection of diagnostics inside the current ImplicitCtxt object. This object collects the diagnostics from this thread and later passes them on to TyCtxt::store_diagnostic(diagnostic).
   3. The calls are then simply forwarded to TyCtxt::on_disk_cache.store_diagnostics(diagnostic) 4. This function collects all diagnostics during the run and later saves them to
2. It inserts the emitted code into rustc_error::HandlerInner::emitted_diagnostic_codes.insert(code.clone());.
3. It checks if this specific diagnostic has been emitted before by comparing the hash with rustc_error::HandlerInner::emitted_diagnostics
4. Then inserts its hash into rustc_error::HandlerInner::emitted_diagnostics
5. The last thing it does it call rustc_error::HandlerInner::emitter.emit_diagnostic(diagnostic); which basically just outputs the diagnostic to the console/json or some other medium. This part can be ignored here.

Summary

1. Only lints with a level above Allow are even emitted.
2. Every emission (also the ones loaded from cache) are passed to rustc_error::HandlerInner::emit_diagnostic()
   1. This function passed the diagnostic to the ImplicitCtxt and therefore indirectly to TyCtxt which passes them on to the on_disk_cache instance. Every emitted diagnostics during a session can be found there, but a warning above the field states "Do not access this directly" and it can be None if incremental compilation is turned off. So, this is not really an option for the implementation unless we want to track the diagnostics inside TyCtxt as well.
   2. The rustc_error::HandlerInner is one central instance that is used for all emissions and already tracks some information like emitted diagnostic hashes and error codes. It would therefore most likely also be the right place to store all lint emissions with their source span and level.

Further steps

I'll now investigate some things and then try to add basic lint tracking inside rustc_error::HandlerInner. If that works it should be possible to add a new query that checks if all expectations have been met. (This is obviously a bit simplified because we still need to check some stuff when it comes to incremental compilation... But that's a problem for tomorrow's @xFrednet)