Centril commented 5 years ago

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

Steps:

[x] Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
- [x] Implement reason = #54683
- [x] Implement #[expect(lint)] -- see https://github.com/rust-lang/rust/issues/85549
[ ] Adjust documentation (see instructions on forge)
[ ] Stabilization PR (see instructions on forge)

Unresolved questions:

[ ] The use sites of the reason parameter.

zackmdavis commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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. :crying_cat_face:

(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 commented 5 years ago

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

myrrlyn commented 5 years ago

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 commented 4 years ago

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 commented 3 years ago

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 :upside_down_face:.

nikomatsakis commented 3 years ago

@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 commented 3 years ago

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: https://github.com/rust-lang/rust/pull/94580

xFrednet commented 2 years ago

Hey, while implementing the expect attribute in rust-lang/rust#87835 I came across a discussion in the RFC suggestion to change the expect attribute name to something else. (Initiated by @Ixrec https://github.com/rust-lang/rfcs/pull/2383#issuecomment-378424091 and the suggestion from @scottmcm to use #[should_lint(...)] https://github.com/rust-lang/rfcs/pull/2383#issuecomment-378648877). No real conclusion was drawn on that point from my understanding. Is the name expect the final name that should be used, or is this still something that needs to be discussed?

At first, I was a bit skeptical of the name, but now I really like it. Therefore, I would just leave it as it is :upside_down_face:

oli-obk commented 2 years ago

Changing the name can be some before stabilization, so imo you can just roll with expect

nikomatsakis commented 2 years ago

We reviewed this as part of our @rust-lang/lang team "backlog bonanza" meeting today. Now that @xFrednet's PR is landed, it seems like this is ready for stabilization? (Except that we don't have any docs...)

If so, we'd love to see somebody prepare a stabilization report and open a PR to stabilize (contra what that page says, I prefer the stabilization report to be attached to the PR).

nikomatsakis commented 2 years ago

On a personal note, I'm excited to use this :)

xFrednet commented 2 years ago

The implementation currently still has an ICE #95540 and doesn't catch some lint emissions in some very specific cases (There is no issue for that). Both of these items are still on my todo list, but I sadly didn't have the time to address them. So, it's sadly not quite ready yet.

Once those issues are solved, I wanted to write the documentation and propose stabilizing it. I've done some testing in Clippy, by swapping almost every #[allow] attribute with #[expect] and I'm happy to report that everything works great for us. I would also really like to see this on stable :upside_down_face:

On a personal note, I'm excited to use this :)

Thank you very much!

If it's alright, I would ping you, once the remaining issues have been solved :upside_down_face:

xFrednet commented 2 years ago

Also, even if that's not directly a blocker, it would be good if Clippy had some more time to investigate the impact of the expect attribute. @Jarcho has started a conversation in Clippy's zulip channel. I will have this addressed in the next Clippy meeting. :upside_down_face:

xFrednet commented 2 years ago

9046)

xFrednet commented 2 years ago

Stabilization Report RFC 2383

Hey everyone, with the #[expect] implementation done, I'd like to propose stabilizing this feature. I've crated two stabilization PRs, one updating the documentation and one removing the feature from rustc:

Summary

The RFC 2383 adds a reason parameter to lint attributes and a new #[expect()] attribute to expect lint emissions.

Here is an example how the reason can be added and how it'll be displayed as part of the emitted lint message (Playground):
```
#![feature(lint_reasons)]
fn main() {
    #[deny(unused_variables, reason = "unused variables, should be removed")]
    let unused = "How much wood would a woodchuck chuck?";
}
```
See Output
```text error: unused variable: `unused` --> src/main.rs:5:9 | 5 | let unused = "How much wood would a woodchuck chuck?"; | ^^^^^^ help: if this is intentional, prefix it with an underscore: `_unused` | = note: unused variables, should be removed note: the lint level is defined here --> src/main.rs:4:12 | 4 | #[deny(unused_variables, reason = "unused variables, should be removed")] | ^^^^^^^^^^^^^^^^ ```
Here is an example, that fulfills the expectation and compiles successfully (Playground):
```
#![feature(lint_reasons)]
fn main() {
    #[expect(unused_variables, reason = "WIP, I'll use this value later")]
    let message = "How much wood would a woodchuck chuck?";

    #[expect(unused_variables, reason = "is this unused?")]
    let answer = "about 700 pounds";
    println!("A: {answer}")
}
```
See Output
```text warning: this lint expectation is unfulfilled --> src/main.rs:4:14 | 6 | #[expect(unused_variables, reason = "is this unused?")] | ^^^^^^^^^^^^^^^^ | = note: `#[warn(unfulfilled_lint_expectations)]` on by default = note: is this unused? ```

Tests

test/ui/lint/rfc-2383-lint-reason

All tests for #[expect] in rustc.

More test files for tool lints can be found here:
- src/test/rustdoc-ui/expect-tool-lint-rfc-2383.rs
- src/tools/clippy/tests/ui/expect_tool_lint_rfc_2383.rs
- Clippy was also used as a test: https://github.com/rust-lang/rust-clippy/pull/8797
src/test/ui/lint/reasons.rs

Contains tests for the reason information in lint emissions
src/test/ui/lint/reasons-erroneous.rs

Contains several checks testing the syntax and the related messages if the reason declaration is incorrect.

Changes from the RFC

As part of my implementation, I renamed the #[expect] lint from expectation_missing to unfulfilled_lint_expectations. I think the name works better with other lint attributes and is more descriptive.

Resolutions of unresolved questions

Where should the reason parameter be allowed?
- The current implementation only allows it as the last parameter in all lint attributes
How should #[expect(unfulfilled_lint_expectations)] be handled?
- In the RFC, it was suggested that the unfulfilled_lint_expectations can be expected by outer attributes. However, it was also questioned how useful this would actually be. The current implementation doesn't allow users to expect this lint. For #[expect(unfulfilled_lint_expectations)] the lint will be emitted as usual, with a note saying that unfulfilled_lint_expectations can't be expected.
How should #[expect(XYZ)] and --force-warn XYZ work?
- This implementation, will emit the lint XYZ, as the lint level has been defined by --force-warn and also track the expectation as it usually would with only the #[expect] attribute. See: force_warn_expected_lints_fulfilled.rs and force_warn_expected_lints_unfulfilled.rs

Updates

Since the initial report, a few questions have been discussed by the lang team, here is a quick overview of the questions and resolutions:

Should the attribute really be called #[expect] or is the name too generic?
- #[expect] is good (Decision)
What are the semantics of the #[expect] attribute?
- Decision: An expectation should count as fulfilled, if a #[warn] attribute at the same location would result in a lint emission (Decision)

Open issues

#[expect(lint)] currently doesn't work on macros. This is in line with other lint attributes. This bug is tracked in #87391 and tested in expect_lint_from_macro.rs

CryZe commented 2 years ago

This does not seem to address the unresolved question regarding the name of the attribute. #[expect] is in my opinion too generic of a name. You for example might want to use expect on tests or maybe even normal functions to check for pre- and post-conditions: #[expect(arg > 0)]. Also I could see expect on branches where you want to annotate the branch that you expect to be taken (for optimization purposes). For a niche attribute like this, should_lint seems more descriptive and doesn't collide with other uses for a generic name such as expect.

ehuss commented 2 years ago

In reviewing the documentation PR, I was a bit confused by the behavior of:

#![feature(lint_reasons)]
#![expect(unused_variables)]

pub fn f() {
    #![warn(unused_variables)]
    let x = 1;
}

This not only generates the unused_variables lint, but also generates the unfulfilled_lint_expectations lint. Why would changing the level (with warn, forbid, or deny) after the expect attribute cause it to be unfulfilled? The unused_variables lint is being generated, so why would it not be fulfilled?

xFrednet commented 2 years ago

This does not seem to address the unresolved question regarding the name of the attribute.

True, I've asked in PRs etc what others thought and in general, it was a: "let's keep it like this for now". At first, I also was a bis skeptical and favored a name like #[expect_lint()]. But by now I prefer #[expect]. That is just my opinion though, so I'm open to other suggestions and a related discussion :upside_down_face:

In reviewing the documentation PR, I was a bit confused by the behavior of:

The #[expect] attribute works like every other lint attribute. This is how the RFC described it:

This RFC adds an expect lint attribute that functions identically to allow, but will cause a lint to be emitted when the code it decorates does not raise a lint warning.

To me, it would be weird, if the #[expect] attribute would be the only attribute that has an effect after other lint attribute. (Besides forbid, which overrides other attributes, but also issues a warning.) The way I think of it is, that the expectation is only fulfilled if it actually suppressed a lint emission.

In the given example, the intention of the user seems to be expecting an unused value somewhere, but if x is unused, the user wants to be warned. Since the user wishes to be warned about x it shouldn't fulfill the expectation IMO. Does that make sense, or do you have another perspective? :upside_down_face:

ehuss commented 2 years ago

I think the RFC is somewhat vague on how it works. I would interpret it as: expect can be placed anywhere allow can, and applies to anything below it, and is only fulfilled if a warning or error would otherwise be emitted.

It seems that the actual behavior is: expect is a level, and enables the lint, and changing the level will disable the expect?

The way I think of it is, that the expectation is only fulfilled if it actually suppressed a lint emission.

That doesn't seem to be a complete description of how it works. For example:

#[expect(unsafe_code)]
pub fn f() {
    unsafe {
    }
}

Here, the unsafe_code lint isn't emitted because it is "allow" by default. Nothing is being suppressed. Yet, this is fulfilling the expectation.

I think my fundamental disconnect here is that expect is a level, and that's not what I would ...anticipate... it to be. My mental model of levels is that they go from low (allow) to high (forbid). expect doesn't seem to fit in that continuum.

This isn't a strong objection because the current design is likely serviceable. I just think it is a bit more difficult to describe, and I want to make sure everyone clearly understands how it works and interacts with existing language elements.

xFrednet commented 2 years ago

Now I fully understand the confusion. A level like implementation feels natural to me, but I've also implemented it, so take that with a grain of salt.

I feel like the hierarchy still (kind of) works. A lint expectation is basically an allow, which is stricter (as it requires the lint emission), but at the same time less than a warning as it usually suppresses a lint emission. #[expect] can be used for allowed lints, but I would expect very few users to do so.

Is there a good place where we could potentially get some more feedback from others? :upside_down_face:

xFrednet commented 2 years ago

Hey @ehuss, do you know how we should/can proceed with this?

ehuss commented 2 years ago

I think you'll just need to wait for the lang team to respond and start an FCP. I think there are two concerns that have been raised that they'll need to make a decision on:

The name expect may not be too generic: https://github.com/rust-lang/rust/issues/54503#issuecomment-1179570739
The behavior of expect being a "level" which both enables a lint, and suppresses it, and which can be overridden but other lint controls seems surprising to me: https://github.com/rust-lang/rust/issues/54503#issuecomment-1180773121

Neither issue seems particularly major, but I think they should at least acknowledge and address whether they want to continue, or would like to see changes.

crumblingstatue commented 2 years ago

I really like this feature! It motivates me to enable more lints, for example clippy's family of cast lints, which helped solve real bugs in my application, while elsewhere I can use expect to better document invariants.

For example

let [r, g, b] = egui::color::rgb_from_hsv((f32::from(byte) / 288.0, 1.0, 1.0));
#[expect(
    clippy::cast_possible_truncation,
    clippy::cast_sign_loss,
    reason = "Ranges are in 0-1, they will never be multiplied above 255"
)]
Color::rgb((r * 255.0) as u8, (g * 255.0) as u8, (b * 255.0) as u8)

Here I document the invariant that r g and b are always in the range 0..=1, as returned by the rgb_from_hsv function. I think this is great for helping write correct code, besides the usual tools provided by Rust.

xFrednet commented 2 years ago

Hey @rust-lang/lang, I've created a stabilization report and we identified two concerns. Would you mind reviewing them and giving feedback? :upside_down_face:

Mark-Simulacrum commented 2 years ago

I tend to agree with @ehuss in https://github.com/rust-lang/rust/issues/54503#issuecomment-1180773121 that the current behavior feels pretty awkward to me. My sense is that the "goal" we should have for our rule set is:

Removing #[expect] always causes a warning
- Which also means that adding #[expect] always silences at least one warning

I think that also matches an intuition that lints are essentially somewhat like a "lexical exception", where the various lint attributes act as try/catch blocks which alter propagation. Under that model, a lint is always "thrown", but as it propagates up the "stack" any of the lint attributes will catch it and (potentially) re-throw it. Allow catches and silences; Warn catches and re-throws at a warn level; Deny catches and re-throws at an error level; Forbid catches, hard errors, and throws a new "lint" that makes sure there's no allow, warn, or deny above it. In this framework expect would throw a lint if it didn't catch. Force warn throws a special kind of lint that is "final/constant" on the way it'll get emitted, but still gets caught and re-thrown otherwise.

If this isn't the case, I think it's harder for users to think about #[expect] as a form of assertion -- i.e., that my code is doing something, and I'm acknowledging that. I think in practice it's pretty rare that there's an allow'd lint (either by default or by set lint level) that you explicitly want to assert is present, but I could definitely see that being the case for more esoteric situations. (Something like expecting a wildcard match, maybe, with https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_enum_match_arm, where you don't want to accidentally replace the code with an exhaustive match, even though it's possible).

Looking at https://github.com/rust-lang/rust/issues/54503#issuecomment-1179588519, I find the current behavior confusing for I think similar reasons. Generally I wouldn't treat #[warn(lint)] as an assertion that I'm expecting an unused variable, so that making #[expect(...)] not "catch" that lint is weird.

joshtriplett commented 2 years ago

@Mark-Simulacrum I agree. I think it would make sense for expect to warn if used on a lint that's already marked as allow.

lopopolo commented 2 years ago

If this isn't the case, I think it's harder for users to think about #[expect] as a form of assertion -- i.e., that my code is doing something, and I'm acknowledging that. I think in practice it's pretty rare that there's an allow'd lint (either by default or by set lint level) that you explicitly want to assert is present, but I could definitely see that being the case for more esoteric situations

one use case I have to this is:

I upgrade a project to the newest Rust toolchain.
A clippy lint fires and is a false positive.
CI runs clippy with RUSTFLAGS="-D warnings"
I file a bug.
I allow the lint so I can land the rust upgrade.
I want to remove the allow once the upstream clippy bug fixes the false positive.

This bug in clippy was closed and pointed to #[expect(...)] as the path toward linting on unused allow attributes:

https://github.com/rust-lang/rust-clippy/issues/4636

In these situations, expect is an ESLint-style no-unused-disable rule or RuboCop's Lint/RedundantCopDisableDirective violation.

I would like to turn every item and statement positioned allow into an expect to ensure that these lint directives don't live longer than they should.

xFrednet commented 2 years ago

I think that also matches an intuition that lints are essentially somewhat like a "lexical exception", where the various lint attributes act as try/catch blocks which alter propagation

This is interesting, since this is a totally different from my mental model. I see the lint attributes as region markers, identifying the lint behavior in the marked scope. If the lint emission behaves like try/catch blocks, then it wouldn't make sense that the following code emits a warning, since the #[allow] should catch all lint emissions.

#[allow(unused_variables)]
pub fn test() {
    #[warn(unused_variables)]
    let duck = 9;
}

I'm still in favor of having the #[expect] attribute work like every other lint attribute. When I started to work with Rust, I had to get used to how lint attributes work. With other languages, it was less common to change lint levels in code, since other languages tend to have fewer and less strict lints. In Clippy, we're also received some requests to better document how lint levels can be changed. This indicates that the behavior is not always intuitive. The current implementation allows the user to just replace every #[allow] to be replaced by #[expect]. Having a different implementation would add extra behavior that would need to be learned.

It was mentioned that removing the #[expect] should always give a warning. This is the only potential confusion example that I see. On the other hand, there would be no reason to add the #[expect] attribute in the first place.

Lint levels can also be defined via console flags. If the #[expect] would only be fulfilled by lints with the warn/error level, it would mean that these flags directly influence the behavior of the expectation. There might be some Rustaceans, that enable more lints in their CI or just periodically check them locally to reduce noise.

Therefore, I'm more in favor of having #[expect] behave like all other lint attributes

Mark-Simulacrum commented 2 years ago

Hm, interesting example. I think my intuition is that it still should be possible to map it to the exception model, though it would need some adjustments to my prior phrasing -- I think reversing the direction of flow would work, such that a lint goes "inwards" rather than outwards through the scopes, allowing for more tightly bound catching to alter the behavior.

#[expect(unused_variables)]
pub fn test() {
    #[warn(unused_variables)]
    let duck = 9;
}

What is your expected behavior with the above code? I think under the "every other lint attribute" model, this should still emit a warning, which is why I find that model confusing -- it doesn't make sense to me that expect can override a more tightly bound lint level if it's just a lint level. (forbid has somewhat similar properties but is also a fairly special lint, and even there it's not overriding in any successful compilation). FWIW, I would hope that we don't emit any warning for the above.

I think replacing every non-top-level[^1] allow with expect is the desired outcome once this change is on stable. But I think in all such cases, we would want a warning telling us about the unused attribute if it's not silencing a lint.

it would mean that these flags directly influence the behavior of the expectation

This seems desirable to me; e.g., for rustc we enable internal rustc lints via a CLI flag controlled by bootstrap, which avoids needing to remember to add this to every lib.rs in-tree. That works well, but we would want to use expect(...) with that set of lints too.

[^1]: top-level allows are typically a little different, in my eyes, they're less about current behavior and more about a desire to avoid that lint regardless of whether it's currently being emitted (e.g., maybe you disagree with clippy about some of its lints, and so add a set of allows to every crate).

xFrednet commented 2 years ago

I think reversing the direction of flow would work, such that a lint goes "inwards" rather than outwards through the scopes, allowing for more tightly bound catching to alter the behavior.

That's similar :+1:

What is your expected behavior with the above code?

That should emit two warnings. The #[warn] indicates that this value should issue a warning, if the affected statement triggers the lint. The #[expect] looks for a lint emission that can be suppressed. Since the user expressed that the lint for the duck value should be emitted as a warning, it shouldn't be suppressed and therefore not fulfill the expectation.

I think replacing every non-top-level1 allow with expect is the desired outcome once this change is on stable.

Agreed, we did that in Clippy and found some attributes which were no longer needed.

This seems desirable to me

Could you explain why this is desirable? Here is one example that I thought of, with clippy::cast_lossless which is allow by default and in the clippy::pedantic group.

#[expect(clippy::cast_lossless)]
fn as_u64(x: u8) -> u64 {
    x as u64
}

The user normally just uses cargo clippy but every once in a while they run cargo clippy -- -W clippy::pedantic to see if any new lints have been added or some quick wins can be achieved.

Now if #[expect] only is fulfilled if the lint is triggered with the warn/error level, this would mean that the code would be totally fine for cargo clippy -- -W clippy::pedantic but emit a unfulfilled_lint_expectations lint for cargo clippy. In this case, I think it's undesirable that the console flags affect the behavior.

Mark-Simulacrum commented 2 years ago

That should emit two warnings. The #[warn] indicates that this value should issue a warning, if the affected statement triggers the lint. The #[expect] looks for a lint emission that can be suppressed. Since the user expressed that the lint for the duck value should be emitted as a warning, it shouldn't be suppressed and therefore not fulfill the expectation.

Interesting. I think in this particular case the user's intent is maybe a little more clear, but it seems to me that in the general case (e.g., imagine the warn annotation on mod statement or similar), it's not obvious that we should be overriding expect for that scope.

I wonder if a good way out would be to disallow (similar to forbid) changing a lint level after tagging with expect (i.e., within the expect's scope). That would give us room to decide on either path in the future, and it seems like the use case for overriding an expect are pretty minimal -- typically you want to place that expect as local as possible, right? (Otherwise its documentation benefits over allow are minimal).

Now if #[expect] only is fulfilled if the lint is triggered with the warn/error level, this would mean that the code would be totally fine for cargo clippy -- -W clippy::pedantic but emit a unfulfilled_lint_expectations lint for cargo clippy. In this case, I think it's undesirable that the console flags affect the behavior.

My sense is that our story should be that the user would annotate with #![warn(clippy::cast_lossless)] at some higher scope after "finding out" about the lint for the first time. If we go with the behavior you suggest, it seems like the user is basically implicitly adding a warn (i.e., you can think of expect as expanding to a warn and expect set in immediate succession on the same level). I think the added benefit of being able to avoid explicitly opting into some lints that you care enough about to annotate expects over your code for isn't really strong enough.

If the user had some side-file that they were adding the known cases for pedantic lints to (i.e., not in their code), then I think the case for avoiding the warn would be stronger, but as-is it seems OK.

I also presume that under the model I propose, something like this would work if they wanted to be very limited in scope. Looks weird, but also not entirely bad?

#[warn(clippy::cast_lossless)]
#[expect(clippy::cast_lossless)]
fn as_u64(x: u8) -> u64 {
    x as u64
}

xFrednet commented 2 years ago

Interesting. I think in this particular case the user's intent is maybe a little more clear, but it seems to me that in the general case (e.g., imagine the warn annotation on mod statement or similar), it's not obvious that we should be overriding expect for that scope.

Hmm, on a larger scale, it could be confusing. However, then the user would see two warning (1x unfulfilled_lint_expectation, 1x expected/warned lint) which should indicate that the #[warn] overrode the lint expectation. The lint warn message might even include a reference to the #[warn] if it was the first trigger. Putting one and one together, I think this again becomes obvious.

There might even be a use case for this, where the user first put #![expect(unused)] at the root for WIP and is now slowly adding #[warn(unused)] to each module after the other to work on them step by step.

I wonder if a good way out would be to disallow (similar to forbid) changing a lint level after tagging with expect (i.e., within the expect's scope). That would give us room to decide on either path in the future, and it seems like the use case for overriding an expect are pretty minimal -- typically you want to place that expect as local as possible, right?

That would be one option I would be okay with. Though I'm still in favor of keeping it as it is. The change shouldn't be too hard, since it's pretty much the same as forbid. Then we should decide how macros interact with this. Forbid ignores where the changed lint level comes from and just issues a warning. See playground

I also presume that under the model I propose, something like this would work if they wanted to be very limited in scope. Looks weird, but also not entirely bad?

That's not entirely bad, but also not great either. As a side node, AFAIK, the order in which lint attributes are processed is not defined in the Reference. Though, this order will most likely stay, so this could be a solution.

Thinking about macros a bit more from my previous comment, it would be questionable how they handle expectations, if they are only fulfilled for lints at the warn/error level. While I believe it would be bad style to use #[expect] in a macro, it can happen.

Then this example would trigger the first case, but not the second one. This seems trivial, but could be problematic for macros used across multiple crates (Playground):

#![feature(lint_reasons)]

macro_rules! trigger_lint {
    () => {
        #[expect(unused_variables)]
        let x = 0;
    }
}

pub fn case_one() {
    // This would fulfill the expectation
    trigger_lint!();
}

#[allow(unused_variables)]
pub fn case_two() {
    // This wouldn't fulfill the expectation since the lint is allowed
    trigger_lint!();
}

With the current implementation, both are fulfilled, since the outer level doesn't matter. While most lints are disabled in lints, there are sadly still rare false positives in proc macros due to wrong spans.

ssokolow commented 2 years ago

I will say that there is a certain sense to "'expect' is to 'allow' as 'forbid' is to 'deny'".

xFrednet commented 2 years ago

Hey @ssokolow, is your comment in favor of forbidding other lint level attributes for expect like it's done for forbid or just a general statement? :upside_down_face:

xFrednet commented 2 years ago

Hey @Mark-Simulacrum, is there something I can do to move this forward? :upside_down_face:

Mark-Simulacrum commented 2 years ago

I think the lang team will need to discuss the best path forward; it might be worth proposing a design meeting to give the discussion a dedicated time, particularly if you're willing to put together a summary of the current design and the open questions in a document for that meeting.

nikomatsakis commented 2 years ago

Removing nomination.

What's needed for this to go forward is:

A write-up that lays out the alternatives that were discussed on this thread
Summarizes the edge cases that occur
Propose a design meeting for lang-team to review

Speaking personally, I for one would like to see some version of this go forward, so I hope somebody is able to do that!

xFrednet commented 1 year ago

The last few months have been pretty busy, but this has been on my radar. I plan to write the meeting proposal until the end of the year :)

EFanZh commented 1 year ago

Can we stabilize reason = first, without stabilizing #[expect] at the same time?

xFrednet commented 1 year ago

Yes, we could do that. However, since both parts have a working implementation, and I also want to finish this project, I'd prefer not to split them up. I've proposed a lang team meeting here https://github.com/rust-lang/lang-team/issues/191 and hope that this can resolve all questions and we can stabilize both soon.

JarredAllen commented 1 year ago

Any updates from the lang team meeting? I'd love to see this feature stabilized (especially the #[expect] attribute) soon

xFrednet commented 1 year ago

Hey @JarredAllen, the lang meeting sadly didn't reach a conclusion. Like in this issue, there was a discussion if #[expect] should be a lint level or a different construct. The goal is now to find a method to determine which one is more intuitive or gives more utility. The discussion was moved to Zulip. That message also contains links to the discussion. If anyone has any input on the matter, please share them. (This week is pretty busy for me, so I planned to push the topic further next week)

tgross35 commented 1 year ago

Has a decision on the name been addressed yet?

I think I have to agree with the others that expect on its own suffers by conflict with .expect(...), which is something that new users will come across often. So it may be a bit more difficult to search for the right thing, especially for anybody who doesn't know the term "attribute" yet.

expect_lint seems like a good option to me since it's unique and clear about what it does. The downside I see to should_lint is that "to lint" as a verb means moreso "to run a linter", not as much "to emit a lint-related error" (at least to me). That is, should_lint(unused) sounds like the linter for unused should be run, not that one should expect 1+ failures of the unused lint. expect or expect_lint are both more clear here imho.

A downside is that allow warn deny and expect are all nice short single words, expect_lint doesn't fit as well into the status quo.

(this all of course somewhat depends on the ongoing mental model discussion)

lopopolo commented 1 year ago

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

#[suppress(unused, reason = "foo and bar and baz")]

That combined with something like !#[warn(useless_suppression)] would make things pretty clear to me what was permitted and what wasn't.

xFrednet commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

rust-lang / rust

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

98507 should address the last missing NITs from the `#[expect]` implementation. I'll now start working on the documentation and create a stabilization report, once the last PRs are merged. (https://github.com/rust-lang/rust/pull/98507, https://github.com/rust-lang/rust-clippy/pull/9046)

Stabilization Report RFC 2383

Summary

Tests

Changes from the RFC

Resolutions of unresolved questions

Updates

Open issues

rust-lang / rust

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

98507 should address the last missing NITs from the #[expect] implementation. I'll now start working on the documentation and create a stabilization report, once the last PRs are merged. (https://github.com/rust-lang/rust/pull/98507, https://github.com/rust-lang/rust-clippy/pull/9046)

Stabilization Report RFC 2383

Summary

Tests

Changes from the RFC

Resolutions of unresolved questions

Updates

Open issues

98507 should address the last missing NITs from the `#[expect]` implementation. I'll now start working on the documentation and create a stabilization report, once the last PRs are merged. (https://github.com/rust-lang/rust/pull/98507, https://github.com/rust-lang/rust-clippy/pull/9046)