Tracking issue for RFC 2383, "Lint Reasons RFC"

[Centril]

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.

[zackmdavis]

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]

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?

[zackmdavis]

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]

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

[myrrlyn]

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]

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]

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]

@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]

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]

Has a decision on the name been addressed yet?

The topic has been discussed as part of the design meeting. Generally, it was said, that expect could be used/doesn't need to be reserved, but the name might depend on how the expect attribute will work in the end.

At the risk of adding noise to a naming question, suppress reads well to me.

This might work, if the try-and-catch model is used for the expect attribute. Though, for me, this name doesn't imply, that a warning should be issued if nothing gets suppressed.

That's why I like expect (or similar variance) a bit more :)

---

If you have any thoughts on the mental model, a comment on Zulip would be appreciated. The discussion has sadly stalled, and some more feedback might help :)

[laralove143]

as far as i understand, unused allow annotations are only reported when expect is provided, why is that so? i'd be using this to notice allow annotations that are forgotten there or falsely added, and this adds another step to allowing lints

i'd prefer another lint that reports unused allows (and warns/denies/forbids as well)

the way eslint does this is you specify it in its config file, which is not what i prefer but its something to look at

[xFrednet]

as far as i understand, unused allow annotations are only reported when expect is provided, why is that so?

Some #[allow] attributes can be unused most of the time, this happens for instance, when cfged is the trigger reason. It would be possible to use cfg_attr(allow)... but that is less readable than a simple #[allow]. It can also be that a project preemptively enables some lints it doesn't care about. This can especially, the case for project templates. This is to say, there is a place for #[allow] attributes, which are not triggered rn.

The idea for expect is, that it would be used instead of #[allow] in most cases. This requires some migration, but then it should essentially be the same as using #[allow] with a lint like you requested. Otherwise, please, say so, I might be missing something.

i'd prefer another lint that reports unused allows (and warns/denies/forbids as well)

Implementing this as a lint, might be possible, but would require the same framework and tracking nightly rustc currently does for #[expect]. One of the problems that came up during development was incremental compilation. It would be possible, but I feel like having a separate attribute, like #[expect], is more versatile. :)

[laralove143]

i think allowing conditionally based on the cfg is better but we cant impose that i think, my idea was this feature would be opt-in but in a way that doesnt require users to adding another attribute to every allow they use, so if it gets in their way, they can just disable it just like any other lint that gets in their way

[flip1995]

require users to adding another attribute to every allow they use, so if it gets in their way, they can just disable it just like any other lint that gets in their way

They shouldn't add another attribute, they should replace the allow with expect, where possible. For migration, a auto-fixable opt-in (Clippy) lint could be implemented, that looks for allow attributes and suggests to replace them with expect.

[xFrednet]

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]

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]

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:

1. 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)

2. 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]

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

[nikomatsakis]

@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!