From 94f8347bae7404d894f4e6dc57fb64e63e8efb73 Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Sat, 31 Aug 2024 14:32:15 +0000 Subject: [PATCH] Check AttrId for expectations. --- compiler/rustc_errors/src/diagnostic.rs | 24 +---- compiler/rustc_errors/src/lib.rs | 101 +++--------------- compiler/rustc_lint/src/expect.rs | 90 +++++++--------- tests/ui/lint/expect-unused-imports.rs | 9 ++ ...force_warn_expected_lints_fulfilled.stderr | 16 +-- 5 files changed, 73 insertions(+), 167 deletions(-) create mode 100644 tests/ui/lint/expect-unused-imports.rs diff --git a/compiler/rustc_errors/src/diagnostic.rs b/compiler/rustc_errors/src/diagnostic.rs index 1c39840207c..412a2c17081 100644 --- a/compiler/rustc_errors/src/diagnostic.rs +++ b/compiler/rustc_errors/src/diagnostic.rs @@ -8,11 +8,11 @@ use rustc_data_structures::fx::FxIndexMap; use rustc_error_messages::{fluent_value_from_str_list_sep_by_and, FluentValue}; -use rustc_lint_defs::{Applicability, LintExpectationId}; +use rustc_lint_defs::Applicability; use rustc_macros::{Decodable, Encodable}; use rustc_span::source_map::Spanned; use rustc_span::symbol::Symbol; -use rustc_span::{AttrId, Span, DUMMY_SP}; +use rustc_span::{Span, DUMMY_SP}; use tracing::debug; use crate::snippet::Style; @@ -354,26 +354,6 @@ pub fn is_error(&self) -> bool { } } - pub(crate) fn update_unstable_expectation_id( - &mut self, - unstable_to_stable: &FxIndexMap, - ) { - if let Level::Expect(expectation_id) | Level::ForceWarning(Some(expectation_id)) = - &mut self.level - && let LintExpectationId::Unstable { attr_id, lint_index } = *expectation_id - { - // 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 Some(stable_id) = unstable_to_stable.get(&attr_id) else { - panic!("{expectation_id:?} must have a matching stable id") - }; - - let mut stable_id = *stable_id; - stable_id.set_lint_index(lint_index); - *expectation_id = stable_id; - } - } - /// Indicates whether this diagnostic should show up in cargo's future breakage report. pub(crate) fn has_future_breakage(&self) -> bool { matches!(self.is_lint, Some(IsLint { has_future_breakage: true, .. })) diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 02ead41fc68..26e3a63c9d6 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -69,7 +69,7 @@ pub use rustc_span::fatal_error::{FatalError, FatalErrorMarker}; use rustc_span::source_map::SourceMap; pub use rustc_span::ErrorGuaranteed; -use rustc_span::{AttrId, Loc, Span, DUMMY_SP}; +use rustc_span::{Loc, Span, DUMMY_SP}; pub use snippet::Style; // Used by external projects such as `rust-gpu`. // See https://github.com/rust-lang/rust/pull/115393. @@ -497,28 +497,18 @@ struct DiagCtxtInner { future_breakage_diagnostics: Vec, - /// The [`Self::unstable_expect_diagnostics`] should be empty when this struct is - /// dropped. However, it can have values if the compilation is stopped early - /// or is only partially executed. To avoid ICEs, like in rust#94953 we only - /// check if [`Self::unstable_expect_diagnostics`] is empty, if the expectation ids - /// have been converted. - check_unstable_expect_diagnostics: bool, - - /// Expected [`DiagInner`][struct@diagnostic::DiagInner]s store a [`LintExpectationId`] as part - /// of the lint level. [`LintExpectationId`]s created early during the compilation - /// (before `HirId`s have been defined) are not stable and can therefore not be - /// stored on disk. This buffer stores these diagnostics until the ID has been - /// replaced by a stable [`LintExpectationId`]. The [`DiagInner`][struct@diagnostic::DiagInner]s - /// are submitted for storage and added to the list of fulfilled expectations. - unstable_expect_diagnostics: Vec, - /// expected diagnostic will have the level `Expect` which additionally /// carries the [`LintExpectationId`] of the expectation that can be /// marked as fulfilled. This is a collection of all [`LintExpectationId`]s /// that have been marked as fulfilled this way. /// /// [RFC-2383]: https://rust-lang.github.io/rfcs/2383-lint-reasons.html - fulfilled_expectations: FxHashSet, + fulfilled_expectations: FxIndexSet, + + /// Whether `fulfilled_expectations` has been stolen. This is used to ICE in case we emit + /// an expectation diagnostic after stealing it, which means that expectation would not be + /// correctly handled. + stolen_fulfilled_expectations: bool, /// The file where the ICE information is stored. This allows delayed_span_bug backtraces to be /// stored along side the main panic backtrace. @@ -605,13 +595,6 @@ fn drop(&mut self) { ); } } - - if self.check_unstable_expect_diagnostics { - assert!( - self.unstable_expect_diagnostics.is_empty(), - "all diagnostics with unstable expectations should have been converted", - ); - } } } @@ -740,9 +723,8 @@ pub fn reset_err_count(&self) { emitted_diagnostics, stashed_diagnostics, future_breakage_diagnostics, - check_unstable_expect_diagnostics, - unstable_expect_diagnostics, fulfilled_expectations, + stolen_fulfilled_expectations: _, ice_file: _, } = inner.deref_mut(); @@ -761,8 +743,6 @@ pub fn reset_err_count(&self) { *emitted_diagnostics = Default::default(); *stashed_diagnostics = Default::default(); *future_breakage_diagnostics = Default::default(); - *check_unstable_expect_diagnostics = false; - *unstable_expect_diagnostics = Default::default(); *fulfilled_expectations = Default::default(); } @@ -1094,44 +1074,11 @@ pub fn emit_unused_externs( inner.emitter.emit_unused_externs(lint_level, unused_externs) } - pub fn update_unstable_expectation_id( - &self, - unstable_to_stable: FxIndexMap, - ) { - let mut inner = self.inner.borrow_mut(); - let diags = std::mem::take(&mut inner.unstable_expect_diagnostics); - inner.check_unstable_expect_diagnostics = true; - - if !diags.is_empty() { - inner.suppressed_expected_diag = true; - for mut diag in diags.into_iter() { - diag.update_unstable_expectation_id(&unstable_to_stable); - - // Here the diagnostic is given back to `emit_diagnostic` where it was first - // intercepted. Now it should be processed as usual, since the unstable expectation - // id is now stable. - inner.emit_diagnostic(diag, self.tainted_with_errors); - } - } - - inner - .stashed_diagnostics - .values_mut() - .for_each(|(diag, _guar)| 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 /// [`DiagCtxtInner`] and indicate that the linked expectation has been fulfilled. #[must_use] - pub fn steal_fulfilled_expectation_ids(&self) -> FxHashSet { - assert!( - self.inner.borrow().unstable_expect_diagnostics.is_empty(), - "`DiagCtxtInner::unstable_expect_diagnostics` should be empty at this point", - ); + pub fn steal_fulfilled_expectation_ids(&self) -> FxIndexSet { + self.inner.borrow_mut().stolen_fulfilled_expectations = true; std::mem::take(&mut self.inner.borrow_mut().fulfilled_expectations) } @@ -1440,9 +1387,8 @@ fn new(emitter: Box) -> Self { emitted_diagnostics: Default::default(), stashed_diagnostics: Default::default(), future_breakage_diagnostics: Vec::new(), - check_unstable_expect_diagnostics: false, - unstable_expect_diagnostics: Vec::new(), fulfilled_expectations: Default::default(), + stolen_fulfilled_expectations: false, ice_file: None, } } @@ -1471,24 +1417,6 @@ fn emit_diagnostic( mut diagnostic: DiagInner, taint: Option<&Cell>>, ) -> Option { - match diagnostic.level { - Expect(expect_id) | ForceWarning(Some(expect_id)) => { - // The `LintExpectationId` can be stable or unstable depending on when it was - // created. Diagnostics created before the definition of `HirId`s are unstable and - // can not yet be stored. Instead, they are buffered until the `LintExpectationId` - // is replaced by a stable one by the `LintLevelsBuilder`. - if let LintExpectationId::Unstable { .. } = expect_id { - // We don't call TRACK_DIAGNOSTIC because we wait for the - // unstable ID to be updated, whereupon the diagnostic will be - // passed into this method again. - self.unstable_expect_diagnostics.push(diagnostic); - return None; - } - // Continue through to the `Expect`/`ForceWarning` case below. - } - _ => {} - } - if diagnostic.has_future_breakage() { // Future breakages aren't emitted if they're `Level::Allow` or // `Level::Expect`, but they still need to be constructed and @@ -1564,9 +1492,10 @@ fn emit_diagnostic( return None; } Expect(expect_id) | ForceWarning(Some(expect_id)) => { - if let LintExpectationId::Unstable { .. } = expect_id { - unreachable!(); // this case was handled at the top of this function - } + assert!( + !self.stolen_fulfilled_expectations, + "Attempting to emit an expected diagnostic after `check_expectations`.", + ); self.fulfilled_expectations.insert(expect_id); if let Expect(_) = diagnostic.level { // Nothing emitted here for expected lints. diff --git a/compiler/rustc_lint/src/expect.rs b/compiler/rustc_lint/src/expect.rs index d8afba3d505..2450afbca06 100644 --- a/compiler/rustc_lint/src/expect.rs +++ b/compiler/rustc_lint/src/expect.rs @@ -1,10 +1,10 @@ -use rustc_data_structures::fx::FxIndexMap; -use rustc_hir::{HirId, CRATE_OWNER_ID}; +use rustc_data_structures::fx::FxHashSet; +use rustc_hir::CRATE_OWNER_ID; use rustc_middle::lint::LintExpectation; use rustc_middle::query::Providers; use rustc_middle::ty::TyCtxt; use rustc_session::lint::builtin::UNFULFILLED_LINT_EXPECTATIONS; -use rustc_session::lint::{Level, LintExpectationId}; +use rustc_session::lint::LintExpectationId; use rustc_span::Symbol; use crate::lints::{Expectation, ExpectationNote}; @@ -17,43 +17,12 @@ fn lint_expectations(tcx: TyCtxt<'_>, (): ()) -> Vec<(LintExpectationId, LintExp let krate = tcx.hir_crate_items(()); let mut expectations = Vec::new(); - let mut unstable_to_stable_ids = FxIndexMap::default(); - let mut record_stable = |attr_id, hir_id, attr_index| { - let expect_id = LintExpectationId::Stable { hir_id, attr_index, lint_index: None }; - unstable_to_stable_ids.entry(attr_id).or_insert(expect_id); - }; - let mut push_expectations = |owner| { + for owner in std::iter::once(CRATE_OWNER_ID).chain(krate.owners()) { let lints = tcx.shallow_lint_levels_on(owner); - if lints.expectations.is_empty() { - return; - } - expectations.extend_from_slice(&lints.expectations); - - let attrs = tcx.hir_attrs(owner); - for &(local_id, attrs) in attrs.map.iter() { - // Some attributes appear multiple times in HIR, to ensure they are correctly taken - // into account where they matter. This means we cannot just associate the AttrId to - // the first HirId where we see it, but need to check it actually appears in a lint - // level. - // FIXME(cjgillot): Can this cause an attribute to appear in multiple expectation ids? - if !lints.specs.contains_key(&local_id) { - continue; - } - for (attr_index, attr) in attrs.iter().enumerate() { - let Some(Level::Expect(_)) = Level::from_attr(attr) else { continue }; - record_stable(attr.id, HirId { owner, local_id }, attr_index.try_into().unwrap()); - } - } - }; - - push_expectations(CRATE_OWNER_ID); - for owner in krate.owners() { - push_expectations(owner); } - tcx.dcx().update_unstable_expectation_id(unstable_to_stable_ids); expectations } @@ -61,24 +30,43 @@ fn check_expectations(tcx: TyCtxt<'_>, tool_filter: Option) { let lint_expectations = tcx.lint_expectations(()); let fulfilled_expectations = tcx.dcx().steal_fulfilled_expectation_ids(); - for (id, expectation) in lint_expectations { - // This check will always be true, since `lint_expectations` only - // holds stable ids - if let LintExpectationId::Stable { hir_id, .. } = id { - if !fulfilled_expectations.contains(id) - && tool_filter.map_or(true, |filter| expectation.lint_tool == Some(filter)) - { - let rationale = expectation.reason.map(|rationale| ExpectationNote { rationale }); - let note = expectation.is_unfulfilled_lint_expectations; - tcx.emit_node_span_lint( - UNFULFILLED_LINT_EXPECTATIONS, - *hir_id, - expectation.emission_span, - Expectation { rationale, note }, - ); + // Turn a `LintExpectationId` into a `(AttrId, lint_index)` pair. + let canonicalize_id = |expect_id: &LintExpectationId| { + match *expect_id { + LintExpectationId::Unstable { attr_id, lint_index: Some(lint_index) } => { + (attr_id, lint_index) } - } else { + LintExpectationId::Stable { hir_id, attr_index, lint_index: Some(lint_index) } => { + // We are an `eval_always` query, so looking at the attribute's `AttrId` is ok. + let attr_id = tcx.hir().attrs(hir_id)[attr_index as usize].id; + (attr_id, lint_index) + } + _ => panic!("fulfilled expectations must have a lint index"), + } + }; + + let fulfilled_expectations: FxHashSet<_> = + fulfilled_expectations.iter().map(canonicalize_id).collect(); + + for (expect_id, expectation) in lint_expectations { + // This check will always be true, since `lint_expectations` only holds stable ids + let LintExpectationId::Stable { hir_id, .. } = expect_id else { unreachable!("at this stage all `LintExpectationId`s are stable"); + }; + + let expect_id = canonicalize_id(expect_id); + + if !fulfilled_expectations.contains(&expect_id) + && tool_filter.map_or(true, |filter| expectation.lint_tool == Some(filter)) + { + let rationale = expectation.reason.map(|rationale| ExpectationNote { rationale }); + let note = expectation.is_unfulfilled_lint_expectations; + tcx.emit_node_span_lint( + UNFULFILLED_LINT_EXPECTATIONS, + *hir_id, + expectation.emission_span, + Expectation { rationale, note }, + ); } } } diff --git a/tests/ui/lint/expect-unused-imports.rs b/tests/ui/lint/expect-unused-imports.rs new file mode 100644 index 00000000000..fc5b1bf2a0f --- /dev/null +++ b/tests/ui/lint/expect-unused-imports.rs @@ -0,0 +1,9 @@ +//@ check-pass + +#![deny(unused_imports)] +#![deny(unfulfilled_lint_expectations)] + +#[expect(unused_imports)] +use std::{io, fs}; + +fn main() {} diff --git a/tests/ui/lint/rfc-2383-lint-reason/force_warn_expected_lints_fulfilled.stderr b/tests/ui/lint/rfc-2383-lint-reason/force_warn_expected_lints_fulfilled.stderr index 29e579464c8..eaf9edd8e8f 100644 --- a/tests/ui/lint/rfc-2383-lint-reason/force_warn_expected_lints_fulfilled.stderr +++ b/tests/ui/lint/rfc-2383-lint-reason/force_warn_expected_lints_fulfilled.stderr @@ -1,3 +1,11 @@ +warning: denote infinite loops with `loop { ... }` + --> $DIR/force_warn_expected_lints_fulfilled.rs:8:5 + | +LL | while true { + | ^^^^^^^^^^ help: use `loop` + | + = note: requested on the command line with `--force-warn while-true` + warning: unused variable: `x` --> $DIR/force_warn_expected_lints_fulfilled.rs:18:9 | @@ -28,13 +36,5 @@ warning: unused variable: `this_should_fulfill_the_expectation` LL | let this_should_fulfill_the_expectation = "The `#[allow]` has no power here"; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_this_should_fulfill_the_expectation` -warning: denote infinite loops with `loop { ... }` - --> $DIR/force_warn_expected_lints_fulfilled.rs:8:5 - | -LL | while true { - | ^^^^^^^^^^ help: use `loop` - | - = note: requested on the command line with `--force-warn while-true` - warning: 5 warnings emitted