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

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

Closed
nikomatsakis opened this issue Sep 19, 2023 · 27 comments · Fixed by #120924
Closed

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

nikomatsakis opened this issue Sep 19, 2023 · 27 comments · Fixed by #120924
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-lint_reasons `#![feature(lint_reasons)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

This issue is spun out from #54503 to serve as the decision issue for a specific question. The question is what the 'mental model' for the expect attribute should be. Two proposed options:

  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)

@xFrednet created a list of use cases to help with the discussion of these two models; they found both models work equally well, except for use case 4 which would only be possible with the first model.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 19, 2023
@nikomatsakis
Copy link
Contributor Author

@rustbot labels +T-lang

@rustbot rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Sep 19, 2023
@nikomatsakis
Copy link
Contributor Author

@rfcbot fcp merge

I propose that we adopt option 1.

@rfcbot
Copy link

rfcbot commented Sep 19, 2023

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 19, 2023
@nikomatsakis
Copy link
Contributor Author

@rustbot labels +F-lint_reasons

@rustbot rustbot added the F-lint_reasons `#![feature(lint_reasons)]` label Sep 19, 2023
@tgross35
Copy link
Contributor

Mind clarifying the title to reference the lint attribute, to not confuse it with .expect()?

@xFrednet xFrednet changed the title Decision: semantics of expect Decision: semantics of the #[expect] attribute Sep 19, 2023
@RalfJung
Copy link
Member

It also wouldn't hurt clarifying where the two semantics differ. I think they only differ in case the lint is allowed, in which case option 2 would always cause an error on expect since there's nothing being linted? So, option 1 is like having an implicit warn(lint) together with the expect(lint)?

@xFrednet
Copy link
Member

  • Option 1 acts like a new lint level. The lint level at the outer scope doesn't matter, since it's overwritten by the new "expect" lint level.
  • Option 2 acts like a try-and-catch block and the lint level is not affected.

You already mentioned the difference, when it comes to the lint being allowed in the outer scope. The semantics also change, how inner lint levels are handled. Let's take this example:

#![allow(unused)]

#[expect(unused_variables)]
fn foo() {
    #[warn(unused_variables)]
    let bar = "Marker is cool";
}
  • Option 1 acts like a new lint level, for the function foo. The #[warn] overwrites the lint level for the let-statement. This causes the unused_variables lint for bar to be emitted and the lint expectation for foo to be unfulfilled
  • Option 2 acts like a try-and-catch block. The unused_variables lint emission from bar would be suppressed by the lint expectation of foo. In this case, no warning would be emitted.

This is one of the examples, why I favor option 1. In this example, I would expect to get a warning for bar, since it states #[warn] above it.

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 19, 2023
@RalfJung
Copy link
Member

With option 2 your example would also error I think? Removing expect doesn't cause any change in output behavior (we get a warning either way). Ergo the expectation is unfulfilled and we should get an error.

@xFrednet
Copy link
Member

With option 2 your example would also error I think?

This is the difference, between the semantic models.

  • Option 1 acts as a new lint level, which can be changed like any other one.
  • Option 2 will suppress any warn or error lint emission in the marked scope. So from my understanding: No, it would not error, since this option suppresses the warn emission. (Zulip Discussion for reference, where the model comes from).

@RalfJung
Copy link
Member

RalfJung commented Sep 20, 2023

Option 2 got described as "The expectation is fulfilled if removing the #[expect] attribute would cause the warning to be emitted". Under that specification, clearly your example would error, since removing the expect attribute would not cause anything to happen, it would be a complete NOP.

Maybe there's a third option that was discussed on Zulip, but to me "option 2" described above does not agree with your characterization of "suppress any warn or error lint emission in the marked scope".

@xFrednet
Copy link
Member

xFrednet commented Sep 20, 2023

Clearly your example would error

This is my intuition as well, which is why I propose and have implemented option 1. However, during the lang meeting and discussion on Zulip, I understood option 2 as stated above.

With option 2 the #[warn] attribute would "create" a lint emission, which would be suppressed by the #[expect] attribute. If you remove the #[expect] attribute, you would get the lint emission from #[warn]. This maps to how @tmandry described the second option: "The expectation is fulfilled if removing the #[expect] attribute would cause the warning to be emitted."

@RalfJung
Copy link
Member

RalfJung commented Sep 21, 2023

This maps to how @tmandry described the second option: "The expectation is fulfilled if removing the #[expect] attribute would cause the warning to be emitted."

No, for me this doesn't map. You described two distinct options:

  • option 2a: The expectation is fulfilled if removing the #[expect] attribute would cause the warning to be emitted. IOW: with the attribute, there is no warning; without it, there is a warning. It's basically an allow that also checks that it actually allowed something.
  • option 2b: The expectation is fulfilled if a warning is emitted inside the scope of the attribute (and then that warning is suppressed). Whether the warning is "caused by" the expect attribute (i.e., whether it is present independent of the attribute) does not matter.

Those are not the same semantics, they behave different on your example above. From what you are saying, t-lang was considering 2b during the Zulip discussion, and maybe @nikomatsakis meant 2b in the issue description, but their summary is sufficiently ambiguous that it sounds like 2a to me.

@xFrednet
Copy link
Member

xFrednet commented Sep 21, 2023

During the meeting, I believe we discussed option 1 and option 2b. For option 2 (regardless of 2a or 2b) it was hard to find a short "one sentence" explanation of the semantics. The one in the issue description came from @tmandry. I guess it's the best to wait on a response from him which option he meant.

@traviscross
Copy link
Contributor

@rustbot labels +I-lang-nominated

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 18, 2023
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Oct 18, 2023

OK, I'm having a hard time following all of this. Let me try to restate the "options" as precisely as I can.

Definitions

When a lint is detected at a particular program point P, there is a stack of lint scopes determined by lexical structure of the program. Each one can be warn/allow/forbid or expect. Prior to expect, we would take the innermost one to determine whether or not a diagnostic was printed. The various options then introduce expect and describe the behavior.

In each case, the expect can be marked as fulfilled. If that does not happen, the expect reports an error.

Stack level

The stack level can be defined as:

fn stack_level(stack: &[Level] -> Level {
    let n = stack.len() - 1;
    match stack[n] {
        ALLOW | WARN | DENY => stack[n],
        EXPECT => stack_level(&stack[0..n-1]),
    }
}

Examples

Here are some examples

Obvious-case

#[expect(unused_variables)]
fn foo() {
    let x;
}

Lint stack here is just: [EXPECT]

Allowed-Outside

In this example, there is a crate-wide allow, but also an expect. Implication is that removing the expect will never cause a warning to be emitted.

#![allow(unused_variables)]
#[expect(unused_variables)]
fn foo() {
    let x;
}

Lint stack here is: [ALLOW, EXPECT]

Allowed-Inside

In this example, there is a warning block inside the function that is more specific than the expect.

#[expect(unused_variables)]
fn foo() {
    #[allow(unused_variables)]
    let x;
}

Lint stack here is: [EXPECT, ALLOW]

Warn-Inside

In this example, there is a warning block inside the function that is more specific than the expect.

#[expect(unused_variables)]
fn foo() {
    #[warn(unused_variables)]
    let x;
}

Lint stack here is: [EXPECT, WARN]

Options

Option 1: "expect must supress a lint from inside its scope"

If EXPECT is at the top of the stack, mark the EXPECT as fulfilled, and suppress the lint.

Option 2a: "removing the expect would cause something to be printed"

If EXPECT is at the top of the stack at position N, consider stack[0..N] and recurse. If that process results in a message being emitted, then mark EXPECT as fulfilled.

Option 2b: "expectation is fulfilled if a warning is emitted inside the scope of the attribute (and then that warning is suppressed)"

If stack_level(stack) is WARN|DENY, and there is at least one EXPECT on the stack, then mark all EXPECTS on the stack at a time as fulfilled and suppress the warning.

Table

Key (multiple options are allowed):

  • ❌ -- results in an error from an unfulfilled expect
  • 🟡 -- a warning is printed to the user
  • ✅ -- compiles successfully
Method Obvious Allowed-Outside Allowed-Inside Warn-Inside
Option 1 🟡❌
Option 2a 🟡❌
Option 2b
What Niko expects 🟡❌

(NOTE: table corrected from its original form, see comments below)

@nikomatsakis
Copy link
Contributor Author

@RalfJung @xFrednet please review the above 👆 -- does it make sense to you?

@RalfJung
Copy link
Member

Yes that makes sense, thanks.

I think the semantics I expected from expect before delving into this discussion are option 1.

Option 2b means many EXPECT can become fulfilled by a single lint being emitted. That seems very non-intuitive to me. But I also didn't really consider nesting #[expect] in the first place...

@xFrednet
Copy link
Member

Thank you for the table @nikomatsakis, I think this is an excellent way to showcase the differences. However, with my understanding of 2b would make the row slightly different:

Method Obvious Allowed-Outside Allowed-Inside Warn-Inside
Option 1 🟡❌
Option 2a 🟡❌
Option 2b 1 1 2
What Niko expects 🟡❌

From these options, I favor option 1. It also seems like this model is easier to grasp and explain than option 2*.

Footnotes

  1. There is an EXPECT on the stack, but the expectation is not fulfilled, as removing the #[expect()] would not cause a lint emission. So same reason as for option 2a 2

  2. The #[warn] inside causes a warning to be emitted. According to the description of option 2b: "expectation is fulfilled if a warning is emitted inside the scope of the attribute" this should suppress the warning and fulfill the expectation.

@nikomatsakis
Copy link
Contributor Author

@xFrednet Yes. I think you are correct, I made some mistakes. Here is how I defined Option 2b (annotated):

If (a) stack_level(stack) is WARN|DENY, and (b) there is at least one EXPECT on the stack, then mark all EXPECTs on the stack at a time as fulfilled and suppress the warning.

On this basis, I agree with your results for Option 2b. Allowed-Outside and Allowed-Inside ❌, because part a (stack_stack=WARN|DENY) is not fulfilled -- the stack-level is ALLOW. Warn-Inside is ✅ because we would get a warning and an EXPECT is on the stack, so it is fulfilled.

@tmandry
Copy link
Member

tmandry commented Jan 24, 2024

Reviewing the comments here, I agree that Option 1 is the most reasonable overall.

There is a use case I had in mind that essentially does want to act more like a try/catch, which would be overriding a warn attribute on some inner code you don't control (like a macro expansion). But that doesn't seem to come up in practice, and if it did it would best be serviced with a different attribute (or compiler flag).

@rfcbot reviewed

@riking
Copy link

riking commented Jan 25, 2024

Has anyone discussed what the behavior with renamed and removed lints will be?

One fairly straightforward possibility is to 🟡 renamed_and_removed_lints and turn it into a #[expect(warnings)] wildcard expect.

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 26, 2024
@rfcbot
Copy link

rfcbot commented Jan 26, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 26, 2024
@scottmcm
Copy link
Member

Thanks, the Allowed-Outside example is very helpful. Compositionally I agree that being in an allow context like that shouldn't make things "stop working", so option 1 sounds good.

@rfcbot reviewed

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

This is now in FCP, so we can unnominate this.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 31, 2024
@xFrednet
Copy link
Member

xFrednet commented Jan 31, 2024

Has anyone discussed what the behavior with renamed and removed lints will be?

One fairly straightforward possibility is to 🟡 renamed_and_removed_lints and turn it into a #[expect(warnings)] wildcard expect.

~@riking Comment

Turning it into a wildcard #[expect(warnings)] would seem weird to me. The implementation tries to behave as similar to other lint attributes as possible. This means that only the lint level of known lints is effected. Unknown lints, trigger the unknown_lints lint and are then ignored. Here is an example (Playground):

#![feature(lint_reasons)]

#[expect(
    unused_mut,
//  ^^^^^^^^^^
//  This creates an expectation which is fulfilled.
//  Nothing is emitted for this one.
    duck_and_such,
//  ^^^^^^^^^^^^^
//  This is an unknown lint for Rust 1.77. The
//  `unknown_lints` lint is emitted, but no expectation
//  is created.
    unused_variables,
//  ^^^^^^^^^^^^^^^^
//  This creates an expectation that is unfulfilled, since
//  all variables are used. Therefore, `unfulfilled_lint_expectations`
//  is emitted for this lint.
)]
fn main() {
    let mut x = 10;
    println!("{x}");
}

Edit: Just noticed that I misread unknown_lints sorry

@ehuss
Copy link
Contributor

ehuss commented Jan 31, 2024

Has anyone discussed what the behavior with renamed and removed lints will be?

To clarify, the behavior for renamed lints is that the old lint name still works as-if you used the new lint name, plus it will fire renamed_and_removed_lints.

Removed lints will fire renamed_and_removed_lints, which I would think is a sufficient indicator to the user to go figure out what they need to do.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 5, 2024
@rfcbot
Copy link

rfcbot commented Feb 5, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 8, 2024
@bors bors closed this as completed in 4bc39f0 Jun 26, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jun 27, 2024
…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

<!--
For Clippy:

changelog: [`allow_attributes`]: Is now available on stable, since the `lint_reasons` feature was stabilized
changelog: [`allow_attributes_without_reason`]: Is now available on stable, since the `lint_reasons` feature was stabilized
-->

---

Roses are red,
Violets are blue,
Let's expect lints,
With reason clues
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jun 27, 2024
…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

<!--
For Clippy:

changelog: [`allow_attributes`]: Is now available on stable, since the `lint_reasons` feature was stabilized
changelog: [`allow_attributes_without_reason`]: Is now available on stable, since the `lint_reasons` feature was stabilized
-->

---

Roses are red,
Violets are blue,
Let's expect lints,
With reason clues
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jun 28, 2024
…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

<!--
For Clippy:

changelog: [`allow_attributes`]: Is now available on stable, since the `lint_reasons` feature was stabilized
changelog: [`allow_attributes_without_reason`]: Is now available on stable, since the `lint_reasons` feature was stabilized
-->

---

Roses are red,
Violets are blue,
Let's expect lints,
With reason clues
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Jul 11, 2024
…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

<!--
For Clippy:

changelog: [`allow_attributes`]: Is now available on stable, since the `lint_reasons` feature was stabilized
changelog: [`allow_attributes_without_reason`]: Is now available on stable, since the `lint_reasons` feature was stabilized
-->

---

Roses are red,
Violets are blue,
Let's expect lints,
With reason clues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-lint_reasons `#![feature(lint_reasons)]` finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.