From 94f8347bae7404d894f4e6dc57fb64e63e8efb73 Mon Sep 17 00:00:00 2001
From: Camille GILLOT <gillot.camille@gmail.com>
Date: Sat, 31 Aug 2024 14:32:15 +0000
Subject: [PATCH 1/2] 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 1c39840207ca6..412a2c17081a9 100644
--- a/compiler/rustc_errors/src/diagnostic.rs
+++ b/compiler/rustc_errors/src/diagnostic.rs
@@ -8,11 +8,11 @@ use std::thread::panicking;
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 @@ impl DiagInner {
}
}
- pub(crate) fn update_unstable_expectation_id(
- &mut self,
- unstable_to_stable: &FxIndexMap<AttrId, LintExpectationId>,
- ) {
- 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 02ead41fc68f1..26e3a63c9d6d0 100644
--- a/compiler/rustc_errors/src/lib.rs
+++ b/compiler/rustc_errors/src/lib.rs
@@ -69,7 +69,7 @@ use rustc_macros::{Decodable, Encodable};
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<DiagInner>,
- /// 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<DiagInner>,
-
/// 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<LintExpectationId>,
+ fulfilled_expectations: FxIndexSet<LintExpectationId>,
+
+ /// 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 @@ impl Drop for DiagCtxtInner {
);
}
}
-
- 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 @@ impl DiagCtxt {
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 @@ impl DiagCtxt {
*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 @@ impl<'a> DiagCtxtHandle<'a> {
inner.emitter.emit_unused_externs(lint_level, unused_externs)
}
- pub fn update_unstable_expectation_id(
- &self,
- unstable_to_stable: FxIndexMap<AttrId, LintExpectationId>,
- ) {
- 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<LintExpectationId> {
- 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<LintExpectationId> {
+ self.inner.borrow_mut().stolen_fulfilled_expectations = true;
std::mem::take(&mut self.inner.borrow_mut().fulfilled_expectations)
}
@@ -1440,9 +1387,8 @@ impl DiagCtxtInner {
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 @@ impl DiagCtxtInner {
mut diagnostic: DiagInner,
taint: Option<&Cell<Option<ErrorGuaranteed>>>,
) -> Option<ErrorGuaranteed> {
- 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 @@ impl DiagCtxtInner {
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 d8afba3d5053c..2450afbca064a 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<Symbol>) {
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)
+ }
+ 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)
}
- } else {
+ _ => 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 0000000000000..fc5b1bf2a0fe6
--- /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 29e579464c8de..eaf9edd8e8fee 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
From 6dfc4033be7c7d9f6551b2db093fd3bdbbc45cc2 Mon Sep 17 00:00:00 2001
From: Camille GILLOT <gillot.camille@gmail.com>
Date: Sat, 7 Sep 2024 01:34:22 +0000
Subject: [PATCH 2/2] Do not ICE on expect(warnings).
---
compiler/rustc_errors/src/lib.rs | 17 +++++------------
.../rfc-2383-lint-reason/expect_warnings.rs | 6 ++++++
2 files changed, 11 insertions(+), 12 deletions(-)
create mode 100644 tests/ui/lint/rfc-2383-lint-reason/expect_warnings.rs
diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs
index 26e3a63c9d6d0..13da1721a4a32 100644
--- a/compiler/rustc_errors/src/lib.rs
+++ b/compiler/rustc_errors/src/lib.rs
@@ -502,14 +502,14 @@ struct DiagCtxtInner {
/// marked as fulfilled. This is a collection of all [`LintExpectationId`]s
/// that have been marked as fulfilled this way.
///
+ /// Emitting expectations after having stolen this field can happen. In particular, an
+ /// `#[expect(warnings)]` can easily make the `UNFULFILLED_LINT_EXPECTATIONS` lint expect
+ /// itself. To avoid needless complexity in this corner case, we tolerate failing to track
+ /// those expectations.
+ ///
/// [RFC-2383]: https://rust-lang.github.io/rfcs/2383-lint-reasons.html
fulfilled_expectations: FxIndexSet<LintExpectationId>,
- /// 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.
ice_file: Option<PathBuf>,
@@ -724,7 +724,6 @@ impl DiagCtxt {
stashed_diagnostics,
future_breakage_diagnostics,
fulfilled_expectations,
- stolen_fulfilled_expectations: _,
ice_file: _,
} = inner.deref_mut();
@@ -1078,7 +1077,6 @@ impl<'a> DiagCtxtHandle<'a> {
/// [`DiagCtxtInner`] and indicate that the linked expectation has been fulfilled.
#[must_use]
pub fn steal_fulfilled_expectation_ids(&self) -> FxIndexSet<LintExpectationId> {
- self.inner.borrow_mut().stolen_fulfilled_expectations = true;
std::mem::take(&mut self.inner.borrow_mut().fulfilled_expectations)
}
@@ -1388,7 +1386,6 @@ impl DiagCtxtInner {
stashed_diagnostics: Default::default(),
future_breakage_diagnostics: Vec::new(),
fulfilled_expectations: Default::default(),
- stolen_fulfilled_expectations: false,
ice_file: None,
}
}
@@ -1492,10 +1489,6 @@ impl DiagCtxtInner {
return None;
}
Expect(expect_id) | ForceWarning(Some(expect_id)) => {
- 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/tests/ui/lint/rfc-2383-lint-reason/expect_warnings.rs b/tests/ui/lint/rfc-2383-lint-reason/expect_warnings.rs
new file mode 100644
index 0000000000000..35d9e02d3c3d9
--- /dev/null
+++ b/tests/ui/lint/rfc-2383-lint-reason/expect_warnings.rs
@@ -0,0 +1,6 @@
+//@ check-pass
+
+#![expect(warnings)]
+
+#[expect(unused)]
+fn main() {}