Adding the `expect` attribute (RFC 2383) #86024

xFrednet · 2021-06-05T14:35:40Z

This is a draft implementation of the expect attribute as defined in RFC 2383. It basically allows to allow a lint but cause a lint message itself if no lint has been triggered in the defined scope.

Example:

// check-pass

#![feature(lint_reasons)]

#[expect(unused_mut)]
#[expect(unused_variables)]
fn main() {
    let x = 0;
}

Output:

warning: this lint expectation is unfulfilled
  --> $DIR/trigger_lint.rs:5:1
   |
LL | #[expect(unused_mut)]
   | ^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unfulfilled_lint_expectation)]` on by default

warning: 1 warning emitted

Implementation

Rust now has a new lint level called Expect. This indicates that a diagnostic should be emitted to the context, but it will not be passed to the user. It is instead saved inside rustc_error::HandlerInner during rustc_error::HandlerInner::emit_diagnostic(diag). This suppresses all lint message with the level expect. (Some lints in rustc and Clippy have a specific check to see if they are allowed in the current scope. Adding a new level allows the addition of the feature without changing such implementations)

Lint expectations have to be checked after all lint passes have been completed. This is done in a new check_expect_lint query. It works similar to the LintLevelMapBuilder like a stack machine. Here is an example:

// Push A on the stack
// Stack [] -> [A]
#[expect(A)]
fn some_function() {
    // Push B on the stack
    // Stack [A] -> [A, B]
    #[expect(B)]
    let x = 0;
    // Pop B from the stack
    //   - consume all issued B's in the scope
    //   - issue a warning if no B has been encountered
    // Stack [A, B] -> [A]

    // Nothing pushed on the stack
    // Stack [A] -> [A]
    println!("{}", a); 
    // Nothing popped off the stack
    // Stack [A] -> [A]
}
// Pop A from the stack
//   - consume all issued A's in the scope
//   - issue a warning if no A has been encountered
// Stack [A] -> []

TODOs I need help with

Adapt documentation
Add tests for tools (make sure that #[expect(clippy::nice)] issues a warning, when Clippy runs)
Test with incremental compilation
Deal with macros correctly (The current implementation strait up ignores the existence of macros)

Open questions

The RFC discussion included a suggestion to change the expect attribute to something else. (Initiated by @lcrec here, suggestion from @scottmcm to use #[should_lint(...)] here). 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?
I've renamed the lint from the proposed name expectation_missing to unfulfilled_lint_expectation. The name was marked as "name open to change" Is this okay with everyone, or should the name be changed back?
New: How should the expect attribute deal with the new force-warn lint level?

I know that this is quite a chunk of code. I sadly don't see a simple way of splitting it up into smaller batches without having partial implementations with loose ends in the compiler. The changes are now split up into 5 commits, with each of them focussing on one area of this implementation.

r? @nikomatsakis as you mentored me in the issue :)

Mentoring/Implementation issue: #85549

RFC tracking issue: #54503

xFrednet · 2021-06-19T10:48:49Z

Hey @nikomatsakis, have you been able to have a look at this implementation draft yet? 🙃

xFrednet · 2021-07-23T12:17:03Z

Hey @nikomatsakis, sorry to ping you again. This PR has been lying around for a while, which is understandable due to the size and that you're probably also quite busy with the 2021 edition. If you're too busy please say so, I can also try to find another reviewer.

This PR could probably be split up into smaller packages of ~300 lines, with the disadvantage that the implementation could only be tested with the last package. The changes could be split up into:

Add the expect lint level & attribute
Suppress lints with the expect level and store them
Check expectations (This would sadly still be ~400 lines without tests)
Add more tests

Would that help with the review?

xFrednet · 2021-07-31T11:20:14Z

I've now restructured the changes into 5 commits, each of them is focussing on a certain aspect of this implementation and should hopefully aid with the review. The previous development commits can still be found under backup/54503-expect-lint-attribute-old-commits

jyn514 · 2021-07-31T13:12:36Z

@flip1995 might be a good reviewer - I know they're on T-clippy, but niko is pretty busy unfortunately :/ I think most people in T-compiler would be a good final approver, I'll assign

r? @wesleywiser

but feel free to reassign :)

jyn514 · 2021-07-31T13:14:34Z

Add tests for tools (make sure that #[expect(clippy::nice)] issues a warning, when Clippy runs)

It might be easier to test rustdoc instead, so you don't have to mess with testing clippy from rust-lang/rust (it's quite difficult currently: #76495)

LeSeulArtichaut

I don't really know this code but here are a few things that I noticed. My main concerns are that it might not behave correctly in more complex scenarios (with macros and such), and that it might not be efficient enough for the compiler's standards. I hope this review helps.

compiler/rustc_errors/src/lib.rs

LeSeulArtichaut · 2021-07-31T13:46:52Z

compiler/rustc_lint/src/expect.rs

+    let krate = tcx.hir().krate();
+
+    let mut checker = LintExpectationChecker::new(tcx, tcx.sess, store);
+    checker.check_crate(krate);


Visiting the whole crate to check that expects are fulfilled seems very inefficient to me. We already collected all the lint level attributes in lint_levels, can't we use that information instead?

I also have this concern, but at the time of development I couldn't think of a nicer solution. It's relatively simple to also collect all expectations in the lint_levels query, but then I was lost what to do with that data. However, your suggestion here to do the expectation checking could solve this and enable the collection in the lint_level query.

compiler/rustc_lint/src/expect.rs

src/test/ui/lint/rfc-2383-lint-reason/expect_enabled_feature_gate.rs

compiler/rustc_lint_defs/src/builtin.rs

LeSeulArtichaut · 2021-07-31T14:24:43Z

compiler/rustc_lint/src/expect.rs

+}
+
+impl CheckScope {
+    fn includes_span(&self, emission: &MultiSpan) -> bool {


Using spans is pretty hacky, and I suspect it might break because of things like macro expansion and stuff.

This was one of the main problems with this task in general. Clippy and I suspect rustc as well emits almost all of its diagnostics via struct_span_lint which only provides us with very limited information. I asked on Zulip and there seems to be no real way to map from a Span to something like the HirId. This method could most likely support macros etc. by going through the span expansion data, but that seems performance intensive as it would also need to be done during check_expect_query.

Any other way of matching the emission span to the expectation would probably require a change to the way lints are emitted. Any suggestions are very welcome!

I like the suggestion you have here about tracking the expectations in the diagnostic emission. I'll first investigate possible solutions over there before going further into this. 🙃

LeSeulArtichaut · 2021-07-31T14:33:43Z

compiler/rustc_errors/src/lib.rs

@@ -332,6 +337,10 @@ struct HandlerInner {
    /// twice.
    emitted_diagnostics: FxHashSet<u128>,

+    /// This contains all lint emission information from this compiling session
+    /// with the emission level `Expect`.
+    expected_lint_emissions: Vec<LintEmission>,


Suggestion: what if we turn this the other way? Say we have a FxHashMap (which is a hash map with a faster hashing algorithm that we use throughout the compiler) that represents all the #[expect] attributes that need to be fulfilled. It takes a (LintStackIndex, LintId) pair as its key and everything that we need to emit the lint (the #[expect] attribute's span, the reason, etc) as its value. Every time we emit an expected lint, we remove that lint's entry from the map, and at the end of compilation, we emit an unfulfilled_lint_expectation lint for every remaining entry in the map.

This would require a few changes to how lints are emitted so I don't feel confident to say "it's the way to go", maybe you'll want to wait on someone more qualified to weigh in.

This is a very interesting idea that I haven't thought about! I actually think that it won't require any major change to the lint emission. The logic already retrieves the lint level for a specific lint. The implementation would only find away to store which expectation set the level to expect in this concrete stance and mark that one as fulfilled.

I'll try to do some prototyping for this suggestion, as I believe it would be the nicer solution. As log as we can find a way to match the diagnostic to the specific expectation.

xFrednet

Thank you very much @LeSeulArtichaut, this was the kind feedback I was hoping for! You gave me some new ideas and suggested some awesome improvements. I'll update the small NITs which only require small renames and refactorings in a new commit.

The prototyping for your bigger suggestions will be done in a different branch. Can I ping you again, when I've made some progress on that? 🙃

xFrednet · 2021-08-01T09:46:33Z

compiler/rustc_lint/src/expect.rs

+    let krate = tcx.hir().krate();
+
+    let mut checker = LintExpectationChecker::new(tcx, tcx.sess, store);
+    checker.check_crate(krate);


I also have this concern, but at the time of development I couldn't think of a nicer solution. It's relatively simple to also collect all expectations in the lint_levels query, but then I was lost what to do with that data. However, your suggestion here to do the expectation checking could solve this and enable the collection in the lint_level query.

xFrednet · 2021-08-01T10:03:59Z

compiler/rustc_lint/src/expect.rs

+}
+
+impl CheckScope {
+    fn includes_span(&self, emission: &MultiSpan) -> bool {


This was one of the main problems with this task in general. Clippy and I suspect rustc as well emits almost all of its diagnostics via struct_span_lint which only provides us with very limited information. I asked on Zulip and there seems to be no real way to map from a Span to something like the HirId. This method could most likely support macros etc. by going through the span expansion data, but that seems performance intensive as it would also need to be done during check_expect_query.

Any other way of matching the emission span to the expectation would probably require a change to the way lints are emitted. Any suggestions are very welcome!

I like the suggestion you have here about tracking the expectations in the diagnostic emission. I'll first investigate possible solutions over there before going further into this. 🙃

compiler/rustc_lint_defs/src/builtin.rs

xFrednet · 2021-08-01T10:14:07Z

compiler/rustc_errors/src/lib.rs

@@ -332,6 +337,10 @@ struct HandlerInner {
    /// twice.
    emitted_diagnostics: FxHashSet<u128>,

+    /// This contains all lint emission information from this compiling session
+    /// with the emission level `Expect`.
+    expected_lint_emissions: Vec<LintEmission>,


This is a very interesting idea that I haven't thought about! I actually think that it won't require any major change to the lint emission. The logic already retrieves the lint level for a specific lint. The implementation would only find away to store which expectation set the level to expect in this concrete stance and mark that one as fulfilled.

I'll try to do some prototyping for this suggestion, as I believe it would be the nicer solution. As log as we can find a way to match the diagnostic to the specific expectation.

src/test/ui/lint/rfc-2383-lint-reason/expect_enabled_feature_gate.rs

LeSeulArtichaut · 2021-08-01T10:48:01Z

Can I ping you again, when I've made some progress on that? 🙃

Sure! Feel free to ping me on Zulip as well if you have any questions that I can try to answer or forward to the knowledgeable people

flip1995 · 2021-08-02T09:22:02Z

@flip1995 might be a good reviewer - I know they're on T-clippy, but niko is pretty busy unfortunately

Thanks for the ping! Yeah, I can help reviewing this. But since @LeSeulArtichaut already started with the review, I first want to ask if you want to continue or pass this on?

xFrednet · 2021-08-02T09:38:43Z

@LeSeulArtichaut Suggested moving the logic into a different part of the compiler. I really like this suggestion and want to try it at least. I would therefore suggest waiting on further reviews until I've investigated the new lead. 🙃

xFrednet · 2021-08-07T00:16:02Z

I've created a new implementation in #87835 based on the feedback in this PR. The new implementation is in my opinion better in every regard. I'll therefore close this PR and focus on the new implementation.

Thank you to everyone that has given me feedback in this thread! 🙃

rust-highfive assigned nikomatsakis Jun 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2021

This comment has been minimized.

Sign in to view

xFrednet marked this pull request as draft June 6, 2021 20:11

This comment has been minimized.

Sign in to view

xFrednet force-pushed the 54503-expect-lint-attribute branch from ecd043e to 31ec6f7 Compare July 4, 2021 11:58

xFrednet changed the title ~~WIP: Adding the expect attribute (RFC 2383)~~ Adding the expect attribute (RFC 2383) Jul 4, 2021

This comment has been minimized.

Sign in to view

xFrednet force-pushed the 54503-expect-lint-attribute branch from 31ec6f7 to 1cbef78 Compare July 12, 2021 20:39

This comment has been minimized.

Sign in to view

xFrednet force-pushed the 54503-expect-lint-attribute branch from 1cbef78 to 65bec5b Compare July 12, 2021 20:56

This comment has been minimized.

Sign in to view

xFrednet force-pushed the 54503-expect-lint-attribute branch from 65bec5b to de5f21d Compare July 22, 2021 17:31

xFrednet added 4 commits July 30, 2021 00:20

Added expect lint level and attribute (RFC-2383)

243029b

Collect diagnostics with level expect (RFC 2383)

9066544

Extracted lint reason parsing for reusability

a56f4b4

Lint on unfulfilled lint expectation (RFC 2383)

d073638

xFrednet force-pushed the 54503-expect-lint-attribute branch from de5f21d to bbe13f3 Compare July 31, 2021 11:13

This comment has been minimized.

Sign in to view

Added expect attribute tests (RFC 2383)

5e176b0

xFrednet force-pushed the 54503-expect-lint-attribute branch from bbe13f3 to 5e176b0 Compare July 31, 2021 11:32

rust-highfive assigned wesleywiser and unassigned nikomatsakis Jul 31, 2021

LeSeulArtichaut reviewed Jul 31, 2021

View reviewed changes

xFrednet commented Aug 1, 2021

View reviewed changes

expect attribute: Addressed review feedback

43e8b58

xFrednet mentioned this pull request Aug 7, 2021

Implementation of the expect attribute (RFC 2383) #87835

Merged

xFrednet closed this Aug 7, 2021

xFrednet deleted the 54503-expect-lint-attribute branch August 7, 2021 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the `expect` attribute (RFC 2383) #86024

Adding the `expect` attribute (RFC 2383) #86024

xFrednet commented Jun 5, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

xFrednet commented Jun 19, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

xFrednet commented Jul 23, 2021

xFrednet commented Jul 31, 2021

This comment has been minimized.

jyn514 commented Jul 31, 2021

jyn514 commented Jul 31, 2021

LeSeulArtichaut left a comment

LeSeulArtichaut Jul 31, 2021

xFrednet Aug 1, 2021

LeSeulArtichaut Jul 31, 2021

xFrednet Aug 1, 2021

LeSeulArtichaut Jul 31, 2021

xFrednet Aug 1, 2021

xFrednet left a comment

xFrednet Aug 1, 2021

xFrednet Aug 1, 2021

xFrednet Aug 1, 2021

LeSeulArtichaut commented Aug 1, 2021

flip1995 commented Aug 2, 2021

xFrednet commented Aug 2, 2021

xFrednet commented Aug 7, 2021

Adding the expect attribute (RFC 2383) #86024

Adding the expect attribute (RFC 2383) #86024

Conversation

xFrednet commented Jun 5, 2021 • edited Loading

Example:

Implementation

TODOs I need help with

Open questions

This comment has been minimized.

This comment has been minimized.

xFrednet commented Jun 19, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

xFrednet commented Jul 23, 2021

xFrednet commented Jul 31, 2021

This comment has been minimized.

jyn514 commented Jul 31, 2021

jyn514 commented Jul 31, 2021

LeSeulArtichaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xFrednet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeSeulArtichaut commented Aug 1, 2021

flip1995 commented Aug 2, 2021

xFrednet commented Aug 2, 2021

xFrednet commented Aug 7, 2021

Adding the `expect` attribute (RFC 2383) #86024

Adding the `expect` attribute (RFC 2383) #86024

xFrednet commented Jun 5, 2021 •

edited

Loading