Skip to content

Improve expect impl and handle #[expect(unfulfilled_lint_expectations)] (RFC 2383) #94670

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

Merged
merged 4 commits into from
Mar 15, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,7 @@ impl Handler {

/// This methods steals all [`LintExpectationId`]s that are stored inside
/// [`HandlerInner`] and indicate that the linked expectation has been fulfilled.
#[must_use]
pub fn steal_fulfilled_expectation_ids(&self) -> FxHashSet<LintExpectationId> {
assert!(
self.inner.borrow().unstable_expect_diagnostics.is_empty(),
Expand Down
9 changes: 5 additions & 4 deletions compiler/rustc_lint/src/expect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ fn emit_unfulfilled_expectation_lint(
hir_id: HirId,
expectation: &LintExpectation,
) {
// FIXME: The current implementation doesn't cover cases where the
// `unfulfilled_lint_expectations` is actually expected by another lint
// expectation. This can be added here by checking the lint level and
// retrieving the `LintExpectationId` if it was expected.
tcx.struct_span_lint_hir(
builtin::UNFULFILLED_LINT_EXPECTATIONS,
hir_id,
Expand All @@ -43,6 +39,11 @@ fn emit_unfulfilled_expectation_lint(
if let Some(rationale) = expectation.reason {
diag.note(&rationale.as_str());
}

if expectation.is_unfulfilled_lint_expectations {
diag.note("the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message");
}

diag.emit();
},
);
Expand Down
34 changes: 26 additions & 8 deletions compiler/rustc_lint/src/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use rustc_middle::lint::{
use rustc_middle::ty::query::Providers;
use rustc_middle::ty::{RegisteredTools, TyCtxt};
use rustc_session::lint::{
builtin::{self, FORBIDDEN_LINT_GROUPS},
builtin::{self, FORBIDDEN_LINT_GROUPS, UNFULFILLED_LINT_EXPECTATIONS},
Level, Lint, LintExpectationId, LintId,
};
use rustc_session::parse::feature_err;
Expand Down Expand Up @@ -212,6 +212,14 @@ impl<'s> LintLevelsBuilder<'s> {
}
}
}

// The lint `unfulfilled_lint_expectations` can't be expected, as it would suppress itself.
// Handling expectations of this lint would add additional complexity with little to no
// benefit. The expect level for this lint will therefore be ignored.
if let Level::Expect(_) = level && id == LintId::of(UNFULFILLED_LINT_EXPECTATIONS) {
return;
}

if let Level::ForceWarn = old_level {
specs.insert(id, (old_level, old_src));
} else {
Expand Down Expand Up @@ -344,6 +352,20 @@ impl<'s> LintLevelsBuilder<'s> {
self.store.check_lint_name(&name, tool_name, self.registered_tools);
match &lint_result {
CheckLintNameResult::Ok(ids) => {
// This checks for instances where the user writes `#[expect(unfulfilled_lint_expectations)]`
// in that case we want to avoid overriding the lint level but instead add an expectation that
// can't be fulfilled. The lint message will include an explanation, that the
// `unfulfilled_lint_expectations` lint can't be expected.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior makes sense to me (as we discussed in the last PR) but before stabilization, we should decide if this is the correct approach. After stabilization, changing the behavior here will be potentially breaking. If we make this a hard error, then we can always relax that later backwards-compatibly.

if let Level::Expect(expect_id) = level {
let is_unfulfilled_lint_expectations = match ids {
[lint] => *lint == LintId::of(UNFULFILLED_LINT_EXPECTATIONS),
_ => false,
};
self.lint_expectations.push((
expect_id,
LintExpectation::new(reason, sp, is_unfulfilled_lint_expectations),
));
}
let src = LintLevelSource::Node(
meta_item.path.segments.last().expect("empty lint name").ident.name,
sp,
Expand All @@ -353,10 +375,6 @@ impl<'s> LintLevelsBuilder<'s> {
self.check_gated_lint(id, attr.span);
self.insert_spec(&mut specs, id, (level, src));
}
if let Level::Expect(expect_id) = level {
self.lint_expectations
.push((expect_id, LintExpectation::new(reason, sp)));
}
}

CheckLintNameResult::Tool(result) => {
Expand All @@ -374,7 +392,7 @@ impl<'s> LintLevelsBuilder<'s> {
}
if let Level::Expect(expect_id) = level {
self.lint_expectations
.push((expect_id, LintExpectation::new(reason, sp)));
.push((expect_id, LintExpectation::new(reason, sp, false)));
}
}
Err((Some(ids), ref new_lint_name)) => {
Expand Down Expand Up @@ -414,7 +432,7 @@ impl<'s> LintLevelsBuilder<'s> {
}
if let Level::Expect(expect_id) = level {
self.lint_expectations
.push((expect_id, LintExpectation::new(reason, sp)));
.push((expect_id, LintExpectation::new(reason, sp, false)));
}
}
Err((None, _)) => {
Expand Down Expand Up @@ -511,7 +529,7 @@ impl<'s> LintLevelsBuilder<'s> {
}
if let Level::Expect(expect_id) = level {
self.lint_expectations
.push((expect_id, LintExpectation::new(reason, sp)));
.push((expect_id, LintExpectation::new(reason, sp, false)));
}
} else {
panic!("renamed lint does not exist: {}", new_name);
Expand Down
12 changes: 10 additions & 2 deletions compiler/rustc_middle/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,19 @@ pub struct LintExpectation {
pub reason: Option<Symbol>,
/// The [`Span`] of the attribute that this expectation originated from.
pub emission_span: Span,
/// Lint messages for the `unfulfilled_lint_expectations` lint will be
/// adjusted to include an additional note. Therefore, we have to track if
/// the expectation is for the lint.
pub is_unfulfilled_lint_expectations: bool,
}

impl LintExpectation {
pub fn new(reason: Option<Symbol>, attr_span: Span) -> Self {
Self { reason, emission_span: attr_span }
pub fn new(
reason: Option<Symbol>,
emission_span: Span,
is_unfulfilled_lint_expectations: bool,
) -> Self {
Self { reason, emission_span, is_unfulfilled_lint_expectations }
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// check-pass
// ignore-tidy-linelength

#![feature(lint_reasons)]
#![warn(unused_mut)]

#![expect(unfulfilled_lint_expectations, reason = "idk why you would expect this")]
//~^ WARNING this lint expectation is unfulfilled
//~| NOTE `#[warn(unfulfilled_lint_expectations)]` on by default
//~| NOTE idk why you would expect this
//~| NOTE the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message

#[expect(unfulfilled_lint_expectations, reason = "a local: idk why you would expect this")]
//~^ WARNING this lint expectation is unfulfilled
//~| NOTE a local: idk why you would expect this
//~| NOTE the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
pub fn normal_test_fn() {
#[expect(unused_mut, reason = "this expectation will create a diagnostic with the default lint level")]
//~^ WARNING this lint expectation is unfulfilled
//~| NOTE this expectation will create a diagnostic with the default lint level
let mut v = vec![1, 1, 2, 3, 5];
v.sort();

// Check that lint lists including `unfulfilled_lint_expectations` are also handled correctly
#[expect(unused, unfulfilled_lint_expectations, reason = "the expectation for `unused` should be fulfilled")]
//~^ WARNING this lint expectation is unfulfilled
//~| NOTE the expectation for `unused` should be fulfilled
//~| NOTE the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message
let value = "I'm unused";
}

#[expect(warnings, reason = "this suppresses all warnings and also suppresses itself. No warning will be issued")]
pub fn expect_warnings() {
// This lint trigger will be suppressed
#[warn(unused_mut)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wondered why this doesn't warn here. It should override theexpect/allow of the outer scope. But it seems like warnings is somehow treated differently to other lint groups. It behaves the same as with allow, see Playground, so this is not a bug in the expect attribute.

But I would argue, that this is generally surprising behavior. Is this documented somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the warnings name seems to behave a bit differently. It looks like it sets the level for all warnings in a given scope. I only could find this documentation:

image

source and this. But both say that it behaves like a lint group and not overrides the level. I think this is fine for now as it behaves like the allow attribute 🙃

Copy link
Member

@flip1995 flip1995 Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine for now

Agreed. Nothing to do in this PR. I just wanted to point out this behavior, because I thought it is generally unexpected/surprising.

let mut v = vec![1, 1, 2, 3, 5];
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
warning: this lint expectation is unfulfilled
--> $DIR/expect_unfulfilled_expectation.rs:7:11
|
LL | #![expect(unfulfilled_lint_expectations, reason = "idk why you would expect this")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(unfulfilled_lint_expectations)]` on by default
= note: idk why you would expect this
= note: the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message

warning: this lint expectation is unfulfilled
--> $DIR/expect_unfulfilled_expectation.rs:13:10
|
LL | #[expect(unfulfilled_lint_expectations, reason = "a local: idk why you would expect this")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: a local: idk why you would expect this
= note: the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message

warning: this lint expectation is unfulfilled
--> $DIR/expect_unfulfilled_expectation.rs:18:14
|
LL | #[expect(unused_mut, reason = "this expectation will create a diagnostic with the default lint level")]
| ^^^^^^^^^^
|
= note: this expectation will create a diagnostic with the default lint level

warning: this lint expectation is unfulfilled
--> $DIR/expect_unfulfilled_expectation.rs:25:22
|
LL | #[expect(unused, unfulfilled_lint_expectations, reason = "the expectation for `unused` should be fulfilled")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: the expectation for `unused` should be fulfilled
= note: the `unfulfilled_lint_expectations` lint can't be expected and will always produce this message

warning: 4 warnings emitted