Rollup merge of #163148 - nnethercote:improve-Diag-hashing, r=oli-obk

Clean up diagnostic hashing

`DiagInner` impls `PartialEq` and `Hash`, as you'd expect for storing it in a hash table. But there's a couple of strange things.

- We only store the hash value of the `DiagInner` to do deduplication, not the `DiagInner` itself, which means the `PartialEq` impl is unused.

- The `Hash` impl only considers some of the fields. Some of the ignored fields are clearly deliberate (there are comments) but for some it is unclear if it is deliberate.

This commit:

- Removes the unused `PartialEq` impl.

- Inlines and removes `keys` now that it's not needed for `PartialEq`.

- Uses struct deconstruction to ensure no fields can be accidentally ignored. I have preserved existing behaviour by assuming that all the ignored fields are supposed to be ignored.

- Renames `hash` as an inherent method `dedup_hash` to indicate that it's not a typical hash function, and simplifies it to just return `Hash128` instead of being generic.

- Replaces the unnecessary `collect` on `args` with `as_slice`.

- Improves the comment on `emitted_diagnostics`.

r? @oli-obk

Author: JonathanBrouwer

compiler/rustc_errors/src/diagnostic.rs

@@ -1,13 +1,15 @@
 use std::borrow::Cow;
 use std::fmt::{self, Debug};
-use std::hash::{Hash, Hasher};
+use std::hash::Hash;
 use std::ops::{Deref, DerefMut};
 use std::panic;
 use std::path::PathBuf;
 use std::thread::panicking;
 
 use rustc_ast::attr::version::RustcVersion;
-use rustc_error_messages::{DiagArgMap, DiagArgName, DiagArgValue, IntoDiagArg};
+use rustc_data_structures::stable_hash::StableHasher;
+use rustc_error_messages::{DiagArgMap, DiagArgName, IntoDiagArg};
+use rustc_hashes::Hash128;
 use rustc_lint_defs::{Applicability, LintExpectationId};
 use rustc_macros::{Decodable, Encodable};
 use rustc_span::{DUMMY_SP, Span, Spanned, Symbol};
@@ -305,46 +307,31 @@ impl DiagInner {
         }
     }
 
-    /// Fields used for Hash, and PartialEq trait.
-    fn keys(
-        &self,
-    ) -> (
-        &Level,
-        &[(DiagMessage, Style)],
-        &Option<ErrCode>,
-        &MultiSpan,
-        &[Subdiag],
-        &Suggestions,
-        Vec<(&DiagArgName, &DiagArgValue)>,
-        &Option<IsLint>,
-    ) {
-        (
-            &self.level,
-            &self.messages,
-            &self.code,
-            &self.span,
-            &self.children,
-            &self.suggestions,
-            self.args.iter().collect(),
-            // omit self.sort_span
-            &self.is_lint,
-            // omit self.emitted_at
-        )
-    }
-}
-
-impl Hash for DiagInner {
-    fn hash<H>(&self, state: &mut H)
-    where
-        H: Hasher,
-    {
-        self.keys().hash(state);
-    }
-}
-
-impl PartialEq for DiagInner {
-    fn eq(&self, other: &Self) -> bool {
-        self.keys() == other.keys()
+    /// Hash used to determine if two diagnostics are the same. Used by
+    /// `DiagCtxtInner::emitted_diagnostics`. Some fields are ignored for the hash.
+    pub(crate) fn dedup_hash(&self) -> Hash128 {
+        // Deconstruct to ensure all fields are considered.
+        let DiagInner {
+            level,
+            messages,
+            code,
+            lint_id: _, // ignore
+            span,
+            children,
+            suggestions,
+            args,
+            sort_span: _, // ignore
+            is_lint,
+            long_ty_path: _, // ignore
+            emitted_at: _,   // ignore
+        } = self;
+
+        let hashed_parts =
+            (level, messages, code, span, children, suggestions, args.as_slice(), is_lint);
+
+        let mut hasher = StableHasher::new();
+        hashed_parts.hash(&mut hasher);
+        hasher.finish()
     }
 }
 

compiler/rustc_errors/src/lib.rs

@@ -325,8 +325,10 @@ struct DiagCtxtInner {
     emitted_diagnostic_codes: FxIndexSet<ErrCode>,
 
     /// This set contains a hash of every diagnostic that has been emitted by
-    /// this `DiagCtxt`. These hashes is used to avoid emitting the same error
-    /// twice.
+    /// this `DiagCtxt`. These hashes are used to avoid emitting the same error
+    /// twice. (Because we don't store the diagnostics themselves, two
+    /// different diagnostics with the same hash value will be considered
+    /// equivalent. Such collisions should be vanishingly rare...)
     emitted_diagnostics: FxHashSet<Hash128>,
 
     /// We only want to emit `recursion_depth_exceeding_limit` once per
@@ -1301,12 +1303,7 @@ impl DiagCtxtInner {
                 self.emitted_diagnostic_codes.insert(code);
             }
 
-            let already_emitted = {
-                let mut hasher = StableHasher::new();
-                diagnostic.hash(&mut hasher);
-                let diagnostic_hash = hasher.finish();
-                !self.emitted_diagnostics.insert(diagnostic_hash)
-            };
+            let already_emitted = !self.emitted_diagnostics.insert(diagnostic.dedup_hash());
 
             let is_error = diagnostic.is_error();
             let is_lint = diagnostic.is_lint.is_some();