Tracking issue for RFC 2383, "Lint Reasons RFC" #54503

Centril · 2018-09-23T13:12:47Z

This is a tracking issue for the RFC "Lint Reasons RFC" (rust-lang/rfcs#2383).

Steps:

Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
- Implement reason = lint reasons (RFC 2383, part 1) #54683
- Implement #[expect(lint)] -- see Implement expect attribute from RFC 2383, "Lint Reasons RFC" #85549
Adjust documentation (see instructions on forge)
Stabilization PR (see instructions on forge)

Unresolved questions:

The use sites of the reason parameter.

The text was updated successfully, but these errors were encountered:

zackmdavis · 2018-09-24T05:02:24Z

I started looking at this tonight. (Briefly; I regret that my time is limited.)

The example output in the RFC puts a reason: line immediately below the top-level error: line, but from a consilience-with-the-existing-implementation perspective, I'm inclined to think it would make more sense to use an ordinary note: (just as the existing "#[level(lint)] on by default" messages and must-use rationales do).

The reason should probably be stored as a third field inside of LintSource::Node.

zackmdavis · 2018-09-30T02:00:15Z

PR for reason = is #54683.

Unresolved questions:

The use sites of the reason parameter.

@myrrlyn I'm not sure what "use sites" means in this context?

This is just for the `reason =` name-value meta-item; the `#[expect(lint)]` attribute also described in the RFC is a problem for another day. The place where we were directly calling `emit()` on a match block (whose arms returned a mutable reference to a diagnostic-builder) was admittedly cute, but no longer plausibly natural after adding the if-let to the end of the `LintSource::Node` arm. This regards rust-lang#54503.

zackmdavis · 2018-12-15T22:08:08Z

It's a little bit sad/awkward that reason comments are most useful in practice for allow (rather than warn, deny, or forbid, which authors are more likely to regard as self-explanatory), and yet allow is the only case for which the new reason = functionality doesn't actually do anything. 😿

(This observation prompted by my thought that we should be able to dogfood reason = in this repo now that that functionality is in the beta bootstrap compiler, but ag '//.*\n\w*#\[(warn|deny|forbid|allow)' --ignore tools --ignore test only turns up instances of allow.)

oli-obk · 2018-12-17T08:26:38Z

We could add a lint that suggests turning allow with reason into expect because $reasons

myrrlyn · 2019-07-08T15:22:08Z

I apologize for my absence on this issue.

I'm not sure what "use sites" means in this context?

My standing question was "how should the reason value be used in the diagnostic", and it appears that the simplest answer is the one you provided: "put it in a note item". I have no firm attachment to rendering it as reason: User text here, and if prior art exists for note: User text here, then that is perfectly fine.

JohnTitor · 2020-03-19T21:46:53Z

Here's the initial work of expect lint level: https://github.com/JohnTitor/rust/tree/lint-expect

The remaining thing is to trigger the expectation_missing lint but I have no idea how to implement. If someone could do, feel free to steal something from there. Or I'm happy to continue the work if someone could mentor me :)

xFrednet · 2021-05-20T17:52:00Z

Hello everyone,
I would like to work on the expect attribute and move this issue towards stabilization.

Is there someone here who could mentor me? My contributions so far have only focussed on Clippy. If not I would most likely ask on Zulip for help 🙃.

nikomatsakis · 2021-05-21T14:24:34Z

@xFrednet I would happily mentor you on this! Let me start by opening an issue dedicated to jus the implementaton, since I dislike using the tracking issue for that sort of thing. I'll assign it to you.

xFrednet · 2021-05-31T20:35:44Z

Issue #55112 is related to the implementation of the reason field. The current implementation apparently allows the usage of an attribute with only a reason #[allow(reason = "foo")]. Source:

// FIXME (#55112): issue unused-attributes lint if we thereby
// don't have any lint names (`#[level(reason = "foo")]`)

Noted as a FIXME in the LintsLevelBilder

Just to keep track that this also has to be fixed before stabilization.

PR: #94580

xFrednet · 2023-05-23T11:31:05Z

For migration, a auto-fixable opt-in (Clippy) lint could be implemented, that looks for allow attributes and suggests to replace them with expect.

Clippy already has the clippy::allow_attributes lint for this :)

laralove143 · 2023-05-23T19:16:57Z

ah, thats what the discussion is about then, i agree expect is a bit hard to understand for this, it makes sense but it's not obvious, i imagine most people will replace allows with expect, how about suppress or permit, they dont give the meaning as clearly across but it fits better as an alternative to allow in my opinion

xFrednet · 2023-07-10T08:18:44Z

After the lang meeting in March, there was a discussion on Zulip what the mental model of the expect attribute should be. Two possible explanations came up:

The expectation is fulfilled, if a #[warn] attribute in the same location would cause a diagnostic to be emitted. The suppression of this diagnostic fulfills the expectation. (src) (Current implementation in rustc)
The expectation is fulfilled if removing the #[expect] attribute would cause the warning to be emitted. (src)

I've created a list of use cases to hopefully help with the discussion of these two models. Usually, both models work equally well, except for use case 4 which would only be possible with the first model.

I'm nominating this for T-lang to review.

nikomatsakis · 2023-09-19T18:33:34Z

@xFrednet I just want to say, these are some excellent write-ups / summarization.

nikomatsakis · 2023-09-19T18:40:13Z

@rustbot labels -I-lang-nominated

I'm removing the nomination -- @xFrednet thanks for your patience here. I've opened #115980 to drive this to a final decision. I'm really excited to see this land!

…u,blyxyas Let's `#[expect]` some lints: Stabilize `lint_reasons` (RFC 2383) Let's give this another try! The [previous stabilization attempt](rust-lang/rust#99063) was stalled by some unresolved questions. These have been discussed in a [lang team](rust-lang/lang-team#191) meeting. The last open question, regarding the semantics of the `#[expect]` attribute was decided on in rust-lang/rust#115980 I've just updated the [stabilization report](rust-lang/rust#54503 (comment)) with the discussed questions and decisions. Luckily, the decision is inline with the current implementation. This hopefully covers everything. Let's hope that the CI will be green like the spring. fixes #115980 fixes #54503 --- r? `@wesleywiser` Tacking Issue: rust-lang/rust#54503 Stabilization Report: rust-lang/rust#54503 (comment) Documentation Update: rust-lang/reference#1237  --- Roses are red, Violets are blue, Let's expect lints, With reason clues

In Rust, it is possible to `allow` particular warnings (diagnostics, lints) locally, making the compiler ignore instances of a given warning within a given function, module, block, etc. It is similar to `#pragma GCC diagnostic push` + `ignored` + `pop` in C: #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-function" static void f(void) {} #pragma GCC diagnostic pop But way less verbose: #[allow(dead_code)] fn f() {} By that virtue, it makes it possible to comfortably enable more diagnostics by default (i.e. outside `W=` levels) that may have some false positives but that are otherwise quite useful to keep enabled to catch potential mistakes. The `#[expect(...)]` attribute [1] takes this further, and makes the compiler warn if the diagnostic was _not_ produced. For instance, the following will ensure that, when `f()` is called somewhere, we will have to remove the attribute: #[expect(dead_code)] fn f() {} If we do not, we get a warning from the compiler: warning: this lint expectation is unfulfilled --> x.rs:3:10 | 3 | #[expect(dead_code)] | ^^^^^^^^^ | = note: `#[warn(unfulfilled_lint_expectations)]` on by default This means that `expect`s do not get forgotten when they are not needed. See the next commit for more details, nuances on its usage and documentation on the feature. The attribute requires the `lint_reasons` [2] unstable feature, but it is becoming stable in 1.81.0 (currently in beta), to be released on 2024-09-05 and it has already been useful to clean things up in this patch series, finding cases where the `allow`s should not have been there. Thus, enable `lint_reasons` and convert some of our `allow`s to `expect`s where possible. This feature was also an example of the ongoing collaboration between Rust and the kernel -- we tested it in the kernel early on and found an issue that was quickly resolved [3]. Cc: Fridtjof Stoldt <xfrednet@gmail.com> Cc: Urgau <urgau@numericable.fr> Link: https://rust-lang.github.io/rfcs/2383-lint-reasons.html#expect-lint-attribute [1] Link: rust-lang/rust#54503 [2] Link: rust-lang/rust#114557 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

In Rust, it is possible to `allow` particular warnings (diagnostics, lints) locally, making the compiler ignore instances of a given warning within a given function, module, block, etc. It is similar to `#pragma GCC diagnostic push` + `ignored` + `pop` in C: #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-function" static void f(void) {} #pragma GCC diagnostic pop But way less verbose: #[allow(dead_code)] fn f() {} By that virtue, it makes it possible to comfortably enable more diagnostics by default (i.e. outside `W=` levels) that may have some false positives but that are otherwise quite useful to keep enabled to catch potential mistakes. The `#[expect(...)]` attribute [1] takes this further, and makes the compiler warn if the diagnostic was _not_ produced. For instance, the following will ensure that, when `f()` is called somewhere, we will have to remove the attribute: #[expect(dead_code)] fn f() {} If we do not, we get a warning from the compiler: warning: this lint expectation is unfulfilled --> x.rs:3:10 | 3 | #[expect(dead_code)] | ^^^^^^^^^ | = note: `#[warn(unfulfilled_lint_expectations)]` on by default This means that `expect`s do not get forgotten when they are not needed. See the next commit for more details, nuances on its usage and documentation on the feature. The attribute requires the `lint_reasons` [2] unstable feature, but it is becoming stable in 1.81.0 (to be released on 2024-09-05) and it has already been useful to clean things up in this patch series, finding cases where the `allow`s should not have been there. Thus, enable `lint_reasons` and convert some of our `allow`s to `expect`s where possible. This feature was also an example of the ongoing collaboration between Rust and the kernel -- we tested it in the kernel early on and found an issue that was quickly resolved [3]. Cc: Fridtjof Stoldt <xfrednet@gmail.com> Cc: Urgau <urgau@numericable.fr> Link: https://rust-lang.github.io/rfcs/2383-lint-reasons.html#expect-lint-attribute [1] Link: rust-lang/rust#54503 [2] Link: rust-lang/rust#114557 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Centril added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Sep 23, 2018

Centril mentioned this issue Sep 23, 2018

Lint Reasons RFC rust-lang/rfcs#2383

Merged

flip1995 mentioned this issue Oct 7, 2019

Lint error when needlessly supressing a lint rust-lang/rust-clippy#4636

Closed

jonas-schievink added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Jan 31, 2020

oli-obk mentioned this issue Sep 26, 2020

Remove unused #[allow(...)] statements from compiler/ #77211

Merged

flip1995 mentioned this issue Feb 28, 2021

[Roadmap] Lint groups rust-lang/rust-clippy#6626

Open

7 tasks

nikomatsakis mentioned this issue May 11, 2021

Allow unused variables with todo! #79850

Closed

nikomatsakis mentioned this issue May 21, 2021

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

Closed

9 tasks

xFrednet mentioned this issue Jun 5, 2021

Adding the expect attribute (RFC 2383) #86024

Closed

4 tasks

xFrednet mentioned this issue Aug 7, 2021

Implementation of the expect attribute (RFC 2383) #87835

Merged

xFrednet mentioned this issue Nov 4, 2021

lint unused (clippy) allow attributes rust-lang/rust-clippy#3122

Open

hero78119 mentioned this issue Jul 10, 2023

Rm deadcode of EVM circuit privacy-scaling-explorations/zkevm-circuits#1525

Merged

1 task

xFrednet added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jul 10, 2023

ojeda mentioned this issue Jul 17, 2023

Enable various new clippy lints Rust-for-Linux/linux#1025

Open

xFrednet mentioned this issue Jul 18, 2023

My feedback for 0.1.1 rust-marker/marker#189

Closed

xxchan mentioned this issue Jul 29, 2023

Fix most clippy lints rust-lang/rustfmt#5400

Open

fee1-dead mentioned this issue Aug 4, 2023

rust: kernel: Mark rust_fmt_argument as extern "C" Rust-for-Linux/linux#967

Closed

This was referenced Aug 6, 2023

#[expect(dead_code)] does not behave identically to #[allow(dead_code)] #114557

Closed

New lint: generalize exit, todo, mem_forget, etc. rust-lang/rust-clippy#8031

Open

ojeda mentioned this issue Aug 18, 2023

Rust wanted features Rust-for-Linux/linux#354

Open

35 tasks

nikomatsakis mentioned this issue Sep 19, 2023

Decision: semantics of the #[expect] attribute #115980

Closed

rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 19, 2023

xFrednet mentioned this issue Feb 11, 2024

Let's #[expect] some lints: Stabilize lint_reasons (RFC 2383) #120924

Merged

Phoenix500526 mentioned this issue Apr 18, 2024

chore: bump ruct toolchain version from 1.71.0 to 1.74.0 xline-kv/Xline#758

Merged

bors closed this as completed in 4bc39f0 Jun 26, 2024

neolight1010 mentioned this issue Aug 14, 2024

#[instrument(err)] triggers clippy::blocks_in_conditions when used inside of #[async_trait] on nightly tokio-rs/tracing#2912

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracking issue for RFC 2383, "Lint Reasons RFC" #54503

Tracking issue for RFC 2383, "Lint Reasons RFC" #54503

Centril commented Sep 23, 2018 •

edited by xFrednet

Loading

zackmdavis commented Sep 24, 2018

zackmdavis commented Sep 30, 2018

zackmdavis commented Dec 15, 2018

oli-obk commented Dec 17, 2018

myrrlyn commented Jul 8, 2019

JohnTitor commented Mar 19, 2020

xFrednet commented May 20, 2021

nikomatsakis commented May 21, 2021

xFrednet commented May 31, 2021 •

edited

Loading

xFrednet commented May 23, 2023

laralove143 commented May 23, 2023

xFrednet commented Jul 10, 2023

nikomatsakis commented Sep 19, 2023

nikomatsakis commented Sep 19, 2023

Tracking issue for RFC 2383, "Lint Reasons RFC" #54503

Tracking issue for RFC 2383, "Lint Reasons RFC" #54503

Comments

Centril commented Sep 23, 2018 • edited by xFrednet Loading

zackmdavis commented Sep 24, 2018

zackmdavis commented Sep 30, 2018

zackmdavis commented Dec 15, 2018

oli-obk commented Dec 17, 2018

myrrlyn commented Jul 8, 2019

JohnTitor commented Mar 19, 2020

xFrednet commented May 20, 2021

nikomatsakis commented May 21, 2021

xFrednet commented May 31, 2021 • edited Loading

xFrednet commented May 23, 2023

laralove143 commented May 23, 2023

xFrednet commented Jul 10, 2023

nikomatsakis commented Sep 19, 2023

nikomatsakis commented Sep 19, 2023

Centril commented Sep 23, 2018 •

edited by xFrednet

Loading

xFrednet commented May 31, 2021 •

edited

Loading