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

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

Merged
merged 4 commits into from
Mar 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
25 changes: 24 additions & 1 deletion compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ use crate::Substitution;
use crate::SubstitutionPart;
use crate::SuggestionStyle;
use crate::ToolMetadata;
use rustc_lint_defs::Applicability;
use rustc_data_structures::stable_map::FxHashMap;
use rustc_lint_defs::{Applicability, LintExpectationId};
use rustc_serialize::json::Json;
use rustc_span::{MultiSpan, Span, DUMMY_SP};
use std::fmt;
Expand Down Expand Up @@ -137,6 +138,28 @@ impl Diagnostic {
}
}

pub fn update_unstable_expectation_id(
&mut self,
unstable_to_stable: &FxHashMap<LintExpectationId, LintExpectationId>,
) {
if let Level::Expect(expectation_id) = &mut self.level {
if expectation_id.is_stable() {
return;
}

// The unstable to stable map only maps the unstable `AttrId` to a stable `HirId` with an attribute index.
// The lint index inside the attribute is manually transferred here.
let lint_index = expectation_id.get_lint_index();
expectation_id.set_lint_index(None);
let mut stable_id = *unstable_to_stable
.get(&expectation_id)
.expect("each unstable `LintExpectationId` must have a matching stable id");

stable_id.set_lint_index(lint_index);
*expectation_id = stable_id;
}
}

pub fn has_future_breakage(&self) -> bool {
match self.code {
Some(DiagnosticId::Lint { has_future_breakage, .. }) => has_future_breakage,
Expand Down
30 changes: 18 additions & 12 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,11 @@ impl Drop for HandlerInner {
"no warnings or errors encountered even though `delayed_good_path_bugs` issued",
);
}

assert!(
self.unstable_expect_diagnostics.is_empty(),
"all diagnostics with unstable expectations should have been converted",
);
}
}

Expand Down Expand Up @@ -942,29 +947,30 @@ impl Handler {

let mut inner = self.inner.borrow_mut();
for mut diag in diags.into_iter() {
let mut unstable_id = diag
diag.update_unstable_expectation_id(unstable_to_stable);

let stable_id = diag
.level
.get_expectation_id()
.expect("all diagnostics inside `unstable_expect_diagnostics` must have a `LintExpectationId`");

// The unstable to stable map only maps the unstable `AttrId` to a stable `HirId` with an attribute index.
// The lint index inside the attribute is manually transferred here.
let lint_index = unstable_id.get_lint_index();
unstable_id.set_lint_index(None);
let mut stable_id = *unstable_to_stable
.get(&unstable_id)
.expect("each unstable `LintExpectationId` must have a matching stable id");

stable_id.set_lint_index(lint_index);
diag.level = Level::Expect(stable_id);
inner.fulfilled_expectations.insert(stable_id);

(*TRACK_DIAGNOSTICS)(&diag);
}

inner
.stashed_diagnostics
.values_mut()
.for_each(|diag| diag.update_unstable_expectation_id(unstable_to_stable));
inner
.future_breakage_diagnostics
.iter_mut()
.for_each(|diag| diag.update_unstable_expectation_id(unstable_to_stable));
}

/// 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
36 changes: 28 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,22 @@ 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 {
// The `unfulfilled_lint_expectations` lint is not part of any lint groups. Therefore. we
// only need to check the slice if it contains a single lint.
let is_unfulfilled_lint_expectations = match ids {
xFrednet marked this conversation as resolved.
Show resolved Hide resolved
[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 +377,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 +394,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 +434,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 +531,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
8 changes: 5 additions & 3 deletions compiler/rustc_lint_defs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ pub enum Applicability {
/// Expected `Diagnostic`s get the lint level `Expect` which stores the `LintExpectationId`
/// to match it with the actual expectation later on.
///
/// The `LintExpectationId` has to be has stable between compilations, as diagnostic
/// The `LintExpectationId` has to be stable between compilations, as diagnostic
/// instances might be loaded from cache. Lint messages can be emitted during an
/// `EarlyLintPass` operating on the AST and during a `LateLintPass` traversing the
/// HIR tree. The AST doesn't have enough information to create a stable id. The
Expand All @@ -71,7 +71,7 @@ pub enum Applicability {
#[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash, Encodable, Decodable)]
pub enum LintExpectationId {
/// Used for lints emitted during the `EarlyLintPass`. This id is not
/// has stable and should not be cached.
/// hash stable and should not be cached.
Unstable { attr_id: AttrId, lint_index: Option<u16> },
/// The [`HirId`] that the lint expectation is attached to. This id is
/// stable and can be cached. The additional index ensures that nodes with
Expand Down Expand Up @@ -113,7 +113,9 @@ impl<HCX: rustc_hir::HashStableContext> HashStable<HCX> for LintExpectationId {
lint_index.hash_stable(hcx, hasher);
}
_ => {
unreachable!("HashStable should only be called for a filled `LintExpectationId`")
unreachable!(
"HashStable should only be called for filled and stable `LintExpectationId`"
)
}
}
}
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