Make LintExpectationId stable between compilation sessions (RFC-2383)

xFrednet · xFrednet · commit 33a5945069e2 · 2022-03-02T17:46:08.000+01:00
diff --git a/Cargo.lock b/Cargo.lock
@@ -3882,7 +3882,7 @@ version = "0.0.0"
 dependencies = [
  "rustc_ast",
  "rustc_data_structures",
- "rustc_index",
+ "rustc_hir",
  "rustc_macros",
  "rustc_serialize",
  "rustc_span",
diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs
@@ -25,7 +25,7 @@ use Level::*;
 
 use emitter::{is_case_difference, Emitter, EmitterWriter};
 use registry::Registry;
-use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
+use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap};
 use rustc_data_structures::stable_hasher::StableHasher;
 use rustc_data_structures::sync::{self, Lock, Lrc};
 use rustc_data_structures::AtomicRef;
@@ -452,7 +452,14 @@ struct HandlerInner {
 
     future_breakage_diagnostics: Vec<Diagnostic>,
 
-    /// Lint [`Diagnostic`]s can be expected as described in [RFC-2383]. An
+    /// Expected [`Diagnostic`]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 [`Diagnostic`]s are the
+    /// submitted for storage and added to the list of fulfilled expectations.
+    unstable_expect_diagnostics: Vec<Diagnostic>,
+
     /// 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
@@ -580,6 +587,7 @@ impl Handler {
                 emitted_diagnostics: Default::default(),
                 stashed_diagnostics: Default::default(),
                 future_breakage_diagnostics: Vec::new(),
+                unstable_expect_diagnostics: Vec::new(),
                 fulfilled_expectations: Default::default(),
             }),
         }
@@ -923,6 +931,28 @@ impl Handler {
         self.inner.borrow_mut().emit_unused_externs(lint_level, unused_externs)
     }
 
+    pub fn update_unstable_expectation_id(
+        &self,
+        unstable_to_stable: &FxHashMap<LintExpectationId, LintExpectationId>,
+    ) {
+        let diags = std::mem::take(&mut self.inner.borrow_mut().unstable_expect_diagnostics);
+        if diags.is_empty() {
+            return;
+        }
+
+        let mut inner = self.inner.borrow_mut();
+        for mut diag in diags.into_iter() {
+            if let Some(unstable_id) = diag.level.get_expectation_id() {
+                if let Some(stable_id) = unstable_to_stable.get(&unstable_id) {
+                    diag.level = Level::Expect(*stable_id);
+                    inner.fulfilled_expectations.insert(*stable_id);
+                }
+            }
+
+            (*TRACK_DIAGNOSTICS)(&diag);
+        }
+    }
+
     /// This methods steals all [`LintExpectationId`]s that are stored inside
     /// [`HandlerInner`] and indicate that the linked expectation has been fulfilled.
     pub fn steal_fulfilled_expectation_ids(&self) -> FxHashSet<LintExpectationId> {
@@ -973,6 +1003,15 @@ impl HandlerInner {
             return;
         }
 
+        // 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 Level::Expect(LintExpectationId::Unstable(_)) = diagnostic.level {
+            self.unstable_expect_diagnostics.push(diagnostic.clone());
+            return;
+        }
+
         (*TRACK_DIAGNOSTICS)(diagnostic);
 
         if let Level::Expect(expectation_id) = diagnostic.level {
@@ -1322,6 +1361,13 @@ impl Level {
     pub fn is_failure_note(&self) -> bool {
         matches!(*self, FailureNote)
     }
+
+    pub fn get_expectation_id(&self) -> Option<LintExpectationId> {
+        match self {
+            Level::Expect(id) => Some(*id),
+            _ => None,
+        }
+    }
 }
 
 // FIXME(eddyb) this doesn't belong here AFAICT, should be moved to callsite.
diff --git a/compiler/rustc_lint/src/early.rs b/compiler/rustc_lint/src/early.rs
@@ -59,7 +59,8 @@ impl<'a, T: EarlyLintPass> EarlyContextAndPass<'a, T> {
         F: FnOnce(&mut Self),
     {
         let is_crate_node = id == ast::CRATE_NODE_ID;
-        let push = self.context.builder.push(attrs, is_crate_node);
+        let push = self.context.builder.push(attrs, is_crate_node, None);
+
         self.check_id(id);
         self.enter_attrs(attrs);
         f(self);
diff --git a/compiler/rustc_lint/src/expect.rs b/compiler/rustc_lint/src/expect.rs
@@ -25,7 +25,7 @@ pub fn check_expectations(tcx: TyCtxt<'_>) {
 }
 
 fn emit_unfulfilled_expectation_lint(tcx: TyCtxt<'_>, expectation: &LintExpectation) {
-    // FIXME  The current implementation doesn't cover cases where the
+    // 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 as we have the lint level of this
     // expectation, and we can also mark the lint expectation it would fulfill
diff --git a/compiler/rustc_lint/src/levels.rs b/compiler/rustc_lint/src/levels.rs
@@ -32,17 +32,23 @@ fn lint_levels(tcx: TyCtxt<'_>, (): ()) -> LintLevelMap {
 
     builder.levels.id_to_set.reserve(krate.owners.len() + 1);
 
-    let push = builder.levels.push(tcx.hir().attrs(hir::CRATE_HIR_ID), true);
+    let push =
+        builder.levels.push(tcx.hir().attrs(hir::CRATE_HIR_ID), true, Some(hir::CRATE_HIR_ID));
+
     builder.levels.register_id(hir::CRATE_HIR_ID);
     tcx.hir().walk_toplevel_module(&mut builder);
     builder.levels.pop(push);
 
+    builder.levels.update_unstable_expectation_ids();
     builder.levels.build_map()
 }
 
 pub struct LintLevelsBuilder<'s> {
     sess: &'s Session,
     lint_expectations: FxHashMap<LintExpectationId, LintExpectation>,
+    /// Each expectation has a stable and an unstable identifier. This map
+    /// is used to map from unstable to stable [`LintExpectationId`]s.
+    expectation_id_map: FxHashMap<LintExpectationId, LintExpectationId>,
     sets: LintLevelSets,
     id_to_set: FxHashMap<HirId, LintStackIndex>,
     cur: LintStackIndex,
@@ -66,6 +72,7 @@ impl<'s> LintLevelsBuilder<'s> {
         let mut builder = LintLevelsBuilder {
             sess,
             lint_expectations: Default::default(),
+            expectation_id_map: Default::default(),
             sets: LintLevelSets::new(),
             cur: COMMAND_LINE,
             id_to_set: Default::default(),
@@ -226,13 +233,26 @@ impl<'s> LintLevelsBuilder<'s> {
     ///   `#[allow]`
     ///
     /// Don't forget to call `pop`!
-    pub(crate) fn push(&mut self, attrs: &[ast::Attribute], is_crate_node: bool) -> BuilderPush {
+    pub(crate) fn push(
+        &mut self,
+        attrs: &[ast::Attribute],
+        is_crate_node: bool,
+        source_hir_id: Option<HirId>,
+    ) -> BuilderPush {
         let mut specs = FxHashMap::default();
         let sess = self.sess;
         let bad_attr = |span| struct_span_err!(sess, span, E0452, "malformed lint attribute input");
-        for attr in attrs {
-            let Some(level) = Level::from_symbol(attr.name_or_empty(), attr.id.as_u32()) else {
-                continue
+        for (attr_index, attr) in attrs.iter().enumerate() {
+            let level = match Level::from_symbol(attr.name_or_empty(), || {
+                LintExpectationId::Unstable(attr.id)
+            }) {
+                None => continue,
+                Some(Level::Expect(unstable_id)) if source_hir_id.is_some() => {
+                    let stable_id =
+                        self.create_stable_id(unstable_id, source_hir_id.unwrap(), attr_index);
+                    Level::Expect(stable_id)
+                }
+                Some(lvl) => lvl,
             };
 
             let Some(mut metas) = attr.meta_item_list() else {
@@ -539,6 +559,19 @@ impl<'s> LintLevelsBuilder<'s> {
         BuilderPush { prev, changed: prev != self.cur }
     }
 
+    fn create_stable_id(
+        &mut self,
+        unstable_id: LintExpectationId,
+        hir_id: HirId,
+        attr_index: usize,
+    ) -> LintExpectationId {
+        let stable_id = LintExpectationId::Stable { hir_id, attr_index };
+
+        self.expectation_id_map.insert(unstable_id, stable_id);
+
+        stable_id
+    }
+
     /// Checks if the lint is gated on a feature that is not enabled.
     fn check_gated_lint(&self, lint_id: LintId, span: Span) {
         if let Some(feature) = lint_id.lint.feature_gate {
@@ -582,6 +615,10 @@ impl<'s> LintLevelsBuilder<'s> {
         self.id_to_set.insert(id, self.cur);
     }
 
+    fn update_unstable_expectation_ids(&self) {
+        self.sess.diagnostic().update_unstable_expectation_id(&self.expectation_id_map);
+    }
+
     pub fn build_map(self) -> LintLevelMap {
         LintLevelMap {
             sets: self.sets,
@@ -603,7 +640,8 @@ impl LintLevelMapBuilder<'_> {
     {
         let is_crate_hir = id == hir::CRATE_HIR_ID;
         let attrs = self.tcx.hir().attrs(id);
-        let push = self.levels.push(attrs, is_crate_hir);
+        let push = self.levels.push(attrs, is_crate_hir, Some(id));
+
         if push.changed {
             self.levels.register_id(id);
         }
diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs
@@ -51,8 +51,8 @@ pub mod builtin;
 mod context;
 mod early;
 mod enum_intrinsics_non_enums;
-pub mod hidden_unicode_codepoints;
 mod expect;
+pub mod hidden_unicode_codepoints;
 mod internal;
 mod late;
 mod levels;
diff --git a/compiler/rustc_lint_defs/Cargo.toml b/compiler/rustc_lint_defs/Cargo.toml
@@ -10,4 +10,4 @@ rustc_span = { path = "../rustc_span" }
 rustc_serialize = { path = "../rustc_serialize" }
 rustc_macros = { path = "../rustc_macros" }
 rustc_target = { path = "../rustc_target" }
-rustc_index = { path = "../rustc_index" }
+rustc_hir = { path = "../rustc_hir" }
diff --git a/compiler/rustc_lint_defs/src/lib.rs b/compiler/rustc_lint_defs/src/lib.rs
@@ -5,7 +5,9 @@ extern crate rustc_macros;
 
 pub use self::Level::*;
 use rustc_ast::node_id::{NodeId, NodeMap};
+use rustc_ast::AttrId;
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher, ToStableHashKey};
+use rustc_hir::HirId;
 use rustc_serialize::json::Json;
 use rustc_span::edition::Edition;
 use rustc_span::{sym, symbol::Ident, MultiSpan, Span, Symbol};
@@ -48,29 +50,70 @@ pub enum Applicability {
     Unspecified,
 }
 
-rustc_index::newtype_index! {
-    /// FIXME: The lint expectation ID is currently a simple copy of the `AttrId`
-    /// that the expectation originated from. In the future it should be generated
-    /// by other means. This is for one to keep the IDs independent of each other
-    /// and also to ensure that it is actually stable between compilation sessions.
-    /// (The `AttrId` for instance, is not stable).
-    ///
-    /// Additionally, it would be nice if this generation could be moved into
-    /// [`Level::from_symbol`] to have it all contained in one module and to
-    /// make it simpler to use.
-    pub struct LintExpectationId {
-        DEBUG_FORMAT = "LintExpectationId({})"
+/// Each lint expectation has a `LintExpectationId` assigned by the
+/// [`LintLevelsBuilder`][`rustc_lint::levels::LintLevelsBuilder`]. Expected
+/// [`Diagnostic`][`rustc_errors::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
+/// 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
+/// `LintExpectationId` will instead store the [`AttrId`] defining the expectation.
+/// These `LintExpectationId` will be updated to use the stable [`HirId`] once the
+/// AST has been lowered. The transformation is done by the
+/// [`LintLevelsBuilder`][`rustc_lint::levels::LintLevelsBuilder`]
+#[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.
+    Unstable(AttrId),
+    /// The [`HirId`] that the lint expectation is attached to. This id is
+    /// stable and can be cached. The additional index ensures that nodes with
+    /// several expectations can correctly match diagnostics to the individual
+    /// expectation.
+    Stable { hir_id: HirId, attr_index: usize },
+}
+
+impl LintExpectationId {
+    pub fn is_stable(&self) -> bool {
+        match self {
+            LintExpectationId::Unstable(_) => false,
+            LintExpectationId::Stable { .. } => true,
+        }
     }
 }
 
-rustc_data_structures::impl_stable_hash_via_hash!(LintExpectationId);
+impl<HCX: rustc_hir::HashStableContext> HashStable<HCX> for LintExpectationId {
+    #[inline]
+    fn hash_stable(&self, hcx: &mut HCX, hasher: &mut StableHasher) {
+        match self {
+            LintExpectationId::Unstable(_) => {
+                unreachable!(
+                    "HashStable should never be called for an unstable `LintExpectationId`"
+                )
+            }
+            LintExpectationId::Stable { hir_id, attr_index } => {
+                hir_id.hash_stable(hcx, hasher);
+                attr_index.hash_stable(hcx, hasher);
+            }
+        }
+    }
+}
 
-impl<HCX> ToStableHashKey<HCX> for LintExpectationId {
-    type KeyType = u32;
+impl<HCX: rustc_hir::HashStableContext> ToStableHashKey<HCX> for LintExpectationId {
+    type KeyType = (HirId, usize);
 
     #[inline]
     fn to_stable_hash_key(&self, _: &HCX) -> Self::KeyType {
-        self.as_u32()
+        match self {
+            LintExpectationId::Unstable(_) => {
+                unreachable!(
+                    "HashStable should never be called for an unstable `LintExpectationId`"
+                )
+            }
+            LintExpectationId::Stable { hir_id, attr_index } => (*hir_id, *attr_index),
+        }
     }
 }
 
@@ -133,10 +176,13 @@ impl Level {
     }
 
     /// Converts a symbol to a level.
-    pub fn from_symbol(x: Symbol, possible_lint_expect_id: u32) -> Option<Level> {
+    pub fn from_symbol<F>(x: Symbol, create_expectation_id: F) -> Option<Level>
+    where
+        F: FnOnce() -> LintExpectationId,
+    {
         match x {
             sym::allow => Some(Level::Allow),
-            sym::expect => Some(Level::Expect(LintExpectationId::from(possible_lint_expect_id))),
+            sym::expect => Some(Level::Expect(create_expectation_id())),
             sym::warn => Some(Level::Warn),
             sym::deny => Some(Level::Deny),
             sym::forbid => Some(Level::Forbid),
diff --git a/compiler/rustc_middle/src/lint.rs b/compiler/rustc_middle/src/lint.rs
@@ -6,10 +6,9 @@ use rustc_errors::{Diagnostic, DiagnosticBuilder, DiagnosticId};
 use rustc_hir::HirId;
 use rustc_index::vec::IndexVec;
 use rustc_query_system::ich::StableHashingContext;
-use rustc_session::lint::LintExpectationId;
 use rustc_session::lint::{
     builtin::{self, FORBIDDEN_LINT_GROUPS},
-    FutureIncompatibilityReason, Level, Lint, LintId,
+    FutureIncompatibilityReason, Level, Lint, LintExpectationId, LintId,
 };
 use rustc_session::{DiagnosticMessageId, Session};
 use rustc_span::hygiene::MacroKind;
diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs
@@ -49,7 +49,7 @@ use rustc_middle::mir::FakeReadCause;
 use rustc_query_system::ich::{NodeIdHashingMode, StableHashingContext};
 use rustc_serialize::opaque::{FileEncodeResult, FileEncoder};
 use rustc_session::config::{BorrowckMode, CrateType, OutputFilenames};
-use rustc_session::lint::{Level, Lint};
+use rustc_session::lint::{Level, Lint, LintExpectationId};
 use rustc_session::Limit;
 use rustc_session::Session;
 use rustc_span::def_id::{DefPathHash, StableCrateId};
@@ -2755,11 +2755,13 @@ impl<'tcx> TyCtxt<'tcx> {
                 return bound;
             }
 
-            if hir
-                .attrs(id)
-                .iter()
-                .any(|attr| Level::from_symbol(attr.name_or_empty(), attr.id.as_u32()).is_some())
-            {
+            if hir.attrs(id).iter().enumerate().any(|(attr_index, attr)| {
+                Level::from_symbol(attr.name_or_empty(), || LintExpectationId::Stable {
+                    hir_id: id,
+                    attr_index,
+                })
+                .is_some()
+            }) {
                 return id;
             }
             let next = hir.get_parent_node(id);

Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,7 @@ pub fn check_expectations(tcx: TyCtxt<'_>) {`
`25`	`25`	`}`
`26`	`26`
`27`	`27`	`fn emit_unfulfilled_expectation_lint(tcx: TyCtxt<'_>, expectation: &LintExpectation) {`
`28`		`- // FIXME The current implementation doesn't cover cases where the`
	`28`	`+ // FIXME: The current implementation doesn't cover cases where the`
`29`	`29`	// `unfulfilled_lint_expectations` is actually expected by another lint
`30`	`30`	`// expectation. This can be added here as we have the lint level of this`
`31`	`31`	`// expectation, and we can also mark the lint expectation it would fulfill`