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

Implementation of the expect attribute (RFC 2383) #87835

Merged
merged 13 commits into from
Mar 3, 2022

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Aug 7, 2021

This is an implementation of the expect attribute as described in RFC-2383. 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

// 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

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

Moved to: #85549

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, 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?
  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: #54503

Mentoring/Implementation issue: #85549

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 7, 2021
@rust-log-analyzer

This comment has been minimized.

@xFrednet xFrednet force-pushed the rfc-2383-expect-attribute-with-ids branch from b7c9122 to bb85726 Compare August 7, 2021 00:21
@LeSeulArtichaut
Copy link
Contributor

I don't think I can help much with this. You might want to add a test for macros however.

@xFrednet
Copy link
Member Author

xFrednet commented Aug 8, 2021

That's a very good pointer, thank you for taking a look 🙃

@xFrednet xFrednet force-pushed the rfc-2383-expect-attribute-with-ids branch from d79019e to 3cdf066 Compare August 8, 2021 13:42
@rust-log-analyzer

This comment has been minimized.

@xFrednet xFrednet force-pushed the rfc-2383-expect-attribute-with-ids branch from 3cdf066 to 9e43706 Compare August 8, 2021 14:10
@xFrednet
Copy link
Member Author

xFrednet commented Aug 8, 2021

Hello, I believe that this PR is ready for review. There is definitely one To-Do that has to be addressed before the merge when it comes to incremental compilation and the stability of the LintExpectationId. However, I would first like to get some feedback if the structure implementation is good before diving further into this.

@flip1995 You offered in the last PR to aid with the review. I would appreciate it if you could take a look at this one and maybe give some feedback 🙃.

r? @wesleywiser Would you be willing to take over the main review of this? 🙃

@flip1995
Copy link
Member

Something that came to my mind this morning under the shower: What if you expect an EarlyLintPass and a LateLintPass lint at the same item. So:

#[expect(some_early_lints)]
#[expect(some_late_lints)]
fn foo() {
    // code triggering all specified lints
}

I think this isn't a problem after #87337 gets merged, but I would add a test for it anyway.

@xFrednet
Copy link
Member Author

What if you expect an EarlyLintPass and a LateLintPass lint at the same item.

That's a good thought. The expectations are currently checked at the end of the last lint pass, meaning that it should still work as expected. Adding a test for it is still a very good idea. I'll look up some lints for a test like that.


What are your thoughts on an example where an item has multiple expect attributes with the same lint? For instance

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

A test with normal lint levels shows that they are processed in order top-down (example playground). This means that the current implementation would mark the second expectation as fulfilled, and lint about the first one. Does this seem reasonable? 🙃

@flip1995
Copy link
Member

flip1995 commented Aug 10, 2021

This means that the current implementation would mark the second expectation as fulfilled, and lint about the first one. Does this seem reasonable? 🙃

Intuitively I would expect no warning in this case, if adding 2 identical allow attributes don't produce a warning either. IMO the current behavior is fine for now as long as it is unstable, but before stabilizing it this is an open question that should be addressed.

@inquisitivecrystal inquisitivecrystal added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 24, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2021
@bors

This comment was marked as outdated.

@xFrednet

This comment was marked as resolved.

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2021
@crlf0710 crlf0710 added I-needs-decision Issue: In need of a decision. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 9, 2021
@crlf0710

This comment was marked as off-topic.

@JohnCSimon JohnCSimon removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 24, 2021
@xFrednet xFrednet force-pushed the rfc-2383-expect-attribute-with-ids branch from 5c1229f to 5275d02 Compare March 2, 2022 17:11
@xFrednet
Copy link
Member Author

xFrednet commented Mar 2, 2022

The usage of FxHashMap made a few problems as the expectation check order was not constant. I've rebased and added a new commit to use a Vec instead. The other commits stayed the same. 🙃 The usage of the HashMap was actually a relic of the past. This is a nice improvement :)

@bors try

@bors
Copy link
Contributor

bors commented Mar 2, 2022

⌛ Trying commit 5275d02 with merge 5d83a73fa38efb9763fdd19f1d6ffdb90cc99de7...

@bors
Copy link
Contributor

bors commented Mar 2, 2022

☀️ Try build successful - checks-actions
Build commit: 5d83a73fa38efb9763fdd19f1d6ffdb90cc99de7 (5d83a73fa38efb9763fdd19f1d6ffdb90cc99de7)

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2022

📌 Commit 5275d02 has been approved by wesleywiser

@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 Mar 3, 2022
@bors
Copy link
Contributor

bors commented Mar 3, 2022

⌛ Testing commit 5275d02 with merge 10913c0...

@bors
Copy link
Contributor

bors commented Mar 3, 2022

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 10913c0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 3, 2022
@bors bors merged commit 10913c0 into rust-lang:master Mar 3, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 3, 2022
@xFrednet xFrednet deleted the rfc-2383-expect-attribute-with-ids branch March 3, 2022 21:47
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (10913c0): comparison url.

Summary: This benchmark run did not return any relevant results. 9 results were found to be statistically significant but too small to be relevant.

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

@rustbot label: -perf-regression

@rustbot rustbot removed the perf-regression Performance regression. label Mar 4, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 14, 2022
…-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 :)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 16, 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.
calebcartwright pushed a commit to calebcartwright/rustfmt that referenced this pull request Jun 23, 2022
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.