Auto merge of #108312 - michaelwoerister:hash-set-not-hash-stable, r=…

…eholk Do not implement HashStable for HashSet (MCP 533) This PR removes all occurrences of `HashSet` in query results, replacing it either with `FxIndexSet` or with `UnordSet`, and then removes the `HashStable` implementation of `HashSet`. This is part of implementing [MCP 533](rust-lang/compiler-team#533), that is, removing the `HashStable` implementations of all collection types with unstable iteration order. The changes are mostly mechanical. The only place where additional sorting is happening is in Miri's override implementation of the `exported_symbols` query.
rust-lang · Mar 8, 2023 · 9b60e6c · 9b60e6c
2 parents 38b9655 + b79f026
commit 9b60e6c
Show file tree

Hide file tree

Showing 23 changed files with 127 additions and 103 deletions.
diff --git a/compiler/rustc_codegen_ssa/src/back/symbol_export.rs b/compiler/rustc_codegen_ssa/src/back/symbol_export.rs
@@ -58,7 +58,7 @@ fn reachable_non_generics_provider(tcx: TyCtxt<'_>, cnum: CrateNum) -> DefIdMap<
 
     let mut reachable_non_generics: DefIdMap<_> = tcx
         .reachable_set(())
-        .iter()
+        .items()
         .filter_map(|&def_id| {
             // We want to ignore some FFI functions that are not exposed from
             // this crate. Reachable FFI functions can be lumped into two
@@ -136,7 +136,7 @@ fn reachable_non_generics_provider(tcx: TyCtxt<'_>, cnum: CrateNum) -> DefIdMap<
             };
             (def_id.to_def_id(), info)
         })
-        .collect();
+        .into();
 
     if let Some(id) = tcx.proc_macro_decls_static(()) {
         reachable_non_generics.insert(

diff --git a/compiler/rustc_codegen_ssa/src/target_features.rs b/compiler/rustc_codegen_ssa/src/target_features.rs
@@ -1,7 +1,7 @@
 use rustc_ast::ast;
 use rustc_attr::InstructionSetAttr;
 use rustc_data_structures::fx::FxHashMap;
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::fx::FxIndexSet;
 use rustc_errors::Applicability;
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::DefId;
@@ -418,7 +418,7 @@ pub fn from_target_feature(
 
 /// Computes the set of target features used in a function for the purposes of
 /// inline assembly.
-fn asm_target_features(tcx: TyCtxt<'_>, did: DefId) -> &FxHashSet<Symbol> {
+fn asm_target_features(tcx: TyCtxt<'_>, did: DefId) -> &FxIndexSet<Symbol> {
     let mut target_features = tcx.sess.unstable_target_features.clone();
     if tcx.def_kind(did).has_codegen_attrs() {
         let attrs = tcx.codegen_fn_attrs(did);

diff --git a/compiler/rustc_data_structures/src/stable_hasher.rs b/compiler/rustc_data_structures/src/stable_hasher.rs
@@ -617,18 +617,10 @@ where
     }
 }
 
-impl<K, R, HCX> HashStable<HCX> for ::std::collections::HashSet<K, R>
-where
-    K: ToStableHashKey<HCX> + Eq,
-    R: BuildHasher,
-{
-    fn hash_stable(&self, hcx: &mut HCX, hasher: &mut StableHasher) {
-        stable_hash_reduce(hcx, hasher, self.iter(), self.len(), |hasher, hcx, key| {
-            let key = key.to_stable_hash_key(hcx);
-            key.hash_stable(hcx, hasher);
-        });
-    }
-}
+// It is not safe to implement HashStable for HashSet or any other collection type
+// with unstable but observable iteration order.
+// See https://github.com/rust-lang/compiler-team/issues/533 for further information.
+impl<V, HCX> !HashStable<HCX> for std::collections::HashSet<V> {}
 
 impl<K, V, HCX> HashStable<HCX> for ::std::collections::BTreeMap<K, V>
 where

diff --git a/compiler/rustc_data_structures/src/unord.rs b/compiler/rustc_data_structures/src/unord.rs
@@ -109,6 +109,12 @@ impl<T, I: Iterator<Item = T>> UnordItems<T, I> {
     }
 }
 
+impl<T> UnordItems<T, std::iter::Empty<T>> {
+    pub fn empty() -> Self {
+        UnordItems(std::iter::empty())
+    }
+}
+
 impl<'a, T: Clone + 'a, I: Iterator<Item = &'a T>> UnordItems<&'a T, I> {
     #[inline]
     pub fn cloned(self) -> UnordItems<T, impl Iterator<Item = T>> {
@@ -133,6 +139,20 @@ impl<T: Ord, I: Iterator<Item = T>> UnordItems<T, I> {
         items
     }
 
+    #[inline]
+    pub fn into_sorted_stable_ord(self, use_stable_sort: bool) -> Vec<T>
+    where
+        T: Ord + StableOrd,
+    {
+        let mut items: Vec<T> = self.0.collect();
+        if use_stable_sort {
+            items.sort();
+        } else {
+            items.sort_unstable()
+        }
+        items
+    }
+
     pub fn into_sorted_small_vec<HCX, const LEN: usize>(self, hcx: &HCX) -> SmallVec<[T; LEN]>
     where
         T: ToStableHashKey<HCX>,
@@ -175,6 +195,11 @@ impl<V: Eq + Hash> UnordSet<V> {
         self.inner.len()
     }
 
+    #[inline]
+    pub fn is_empty(&self) -> bool {
+        self.inner.is_empty()
+    }
+
     #[inline]
     pub fn insert(&mut self, v: V) -> bool {
         self.inner.insert(v)
@@ -253,7 +278,7 @@ impl<V: Eq + Hash> UnordSet<V> {
     // We can safely extend this UnordSet from a set of unordered values because that
     // won't expose the internal ordering anywhere.
     #[inline]
-    pub fn extend<I: Iterator<Item = V>>(&mut self, items: UnordItems<V, I>) {
+    pub fn extend_unord<I: Iterator<Item = V>>(&mut self, items: UnordItems<V, I>) {
         self.inner.extend(items.0)
     }
 
@@ -277,6 +302,12 @@ impl<V: Hash + Eq> FromIterator<V> for UnordSet<V> {
     }
 }
 
+impl<V: Hash + Eq> From<FxHashSet<V>> for UnordSet<V> {
+    fn from(value: FxHashSet<V>) -> Self {
+        UnordSet { inner: value }
+    }
+}
+
 impl<HCX, V: Hash + Eq + HashStable<HCX>> HashStable<HCX> for UnordSet<V> {
     #[inline]
     fn hash_stable(&self, hcx: &mut HCX, hasher: &mut StableHasher) {

diff --git a/compiler/rustc_hir_analysis/src/check/intrinsicck.rs b/compiler/rustc_hir_analysis/src/check/intrinsicck.rs
@@ -1,5 +1,5 @@
 use rustc_ast::InlineAsmTemplatePiece;
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::fx::FxIndexSet;
 use rustc_hir as hir;
 use rustc_middle::ty::{self, Article, FloatTy, IntTy, Ty, TyCtxt, TypeVisitableExt, UintTy};
 use rustc_session::lint;
@@ -51,7 +51,7 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> {
         template: &[InlineAsmTemplatePiece],
         is_input: bool,
         tied_input: Option<(&'tcx hir::Expr<'tcx>, Option<InlineAsmType>)>,
-        target_features: &FxHashSet<Symbol>,
+        target_features: &FxIndexSet<Symbol>,
     ) -> Option<InlineAsmType> {
         let ty = (self.get_operand_ty)(expr);
         if ty.has_non_region_infer() {
@@ -201,7 +201,7 @@ impl<'a, 'tcx> InlineAsmCtxt<'a, 'tcx> {
         // (!). In that case we still need the earlier check to verify that the
         // register class is usable at all.
         if let Some(feature) = feature {
-            if !target_features.contains(&feature) {
+            if !target_features.contains(feature) {
                 let msg = &format!("`{}` target feature is not enabled", feature);
                 let mut err = self.tcx.sess.struct_span_err(expr.span, msg);
                 err.note(&format!(

diff --git a/compiler/rustc_hir_analysis/src/check_unused.rs b/compiler/rustc_hir_analysis/src/check_unused.rs
@@ -10,7 +10,7 @@ pub fn check_crate(tcx: TyCtxt<'_>) {
     for item_def_id in tcx.hir().body_owners() {
         let imports = tcx.used_trait_imports(item_def_id);
         debug!("GatherVisitor: item_def_id={:?} with imports {:#?}", item_def_id, imports);
-        used_trait_imports.extend(imports.items().copied());
+        used_trait_imports.extend_unord(imports.items().copied());
     }
 
     for &id in tcx.maybe_unused_trait_imports(()) {

diff --git a/compiler/rustc_middle/src/arena.rs b/compiler/rustc_middle/src/arena.rs
@@ -94,7 +94,8 @@ macro_rules! arena_types {
             [] object_safety_violations: rustc_middle::traits::ObjectSafetyViolation,
             [] codegen_unit: rustc_middle::mir::mono::CodegenUnit<'tcx>,
             [decode] attribute: rustc_ast::Attribute,
-            [] name_set: rustc_data_structures::fx::FxHashSet<rustc_span::symbol::Symbol>,
+            [] name_set: rustc_data_structures::unord::UnordSet<rustc_span::symbol::Symbol>,
+            [] ordered_name_set: rustc_data_structures::fx::FxIndexSet<rustc_span::symbol::Symbol>,
             [] hir_id_set: rustc_hir::HirIdSet,
 
             // Interned types

diff --git a/compiler/rustc_middle/src/mir/query.rs b/compiler/rustc_middle/src/mir/query.rs
@@ -2,7 +2,7 @@
 
 use crate::mir::{Body, ConstantKind, Promoted};
 use crate::ty::{self, OpaqueHiddenType, Ty, TyCtxt};
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::unord::UnordSet;
 use rustc_data_structures::vec_map::VecMap;
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir as hir;
@@ -123,7 +123,7 @@ pub struct UnsafetyCheckResult {
     pub violations: Vec<UnsafetyViolation>,
 
     /// Used `unsafe` blocks in this function. This is used for the "unused_unsafe" lint.
-    pub used_unsafe_blocks: FxHashSet<hir::HirId>,
+    pub used_unsafe_blocks: UnordSet<hir::HirId>,
 
     /// This is `Some` iff the item is not a closure.
     pub unused_unsafes: Option<Vec<(hir::HirId, UnusedUnsafe)>>,

diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs
@@ -764,7 +764,7 @@ rustc_queries! {
     ///
     /// The map returned for `tcx.impl_item_implementor_ids(impl_id)` would be
     ///`{ trait_f: impl_f, trait_g: impl_g }`
-    query impl_item_implementor_ids(impl_id: DefId) -> &'tcx FxHashMap<DefId, DefId> {
+    query impl_item_implementor_ids(impl_id: DefId) -> &'tcx DefIdMap<DefId> {
         arena_cache
         desc { |tcx| "comparing impl items against trait for `{}`", tcx.def_path_str(impl_id) }
     }
@@ -906,8 +906,8 @@ rustc_queries! {
     /// The second return value maps from ADTs to ignored derived traits (e.g. Debug and Clone) and
     /// their respective impl (i.e., part of the derive macro)
     query live_symbols_and_ignored_derived_traits(_: ()) -> &'tcx (
-        FxHashSet<LocalDefId>,
-        FxHashMap<LocalDefId, Vec<(DefId, DefId)>>
+        LocalDefIdSet,
+        LocalDefIdMap<Vec<(DefId, DefId)>>
     ) {
         arena_cache
         desc { "finding live symbols in crate" }
@@ -1120,7 +1120,7 @@ rustc_queries! {
         desc { "checking for private elements in public interfaces" }
     }
 
-    query reachable_set(_: ()) -> &'tcx FxHashSet<LocalDefId> {
+    query reachable_set(_: ()) -> &'tcx LocalDefIdSet {
         arena_cache
         desc { "reachability" }
     }
@@ -1229,7 +1229,7 @@ rustc_queries! {
         separate_provide_extern
     }
 
-    query asm_target_features(def_id: DefId) -> &'tcx FxHashSet<Symbol> {
+    query asm_target_features(def_id: DefId) -> &'tcx FxIndexSet<Symbol> {
         desc { |tcx| "computing target features for inline asm of `{}`", tcx.def_path_str(def_id) }
     }
 
@@ -1845,7 +1845,7 @@ rustc_queries! {
     query maybe_unused_trait_imports(_: ()) -> &'tcx FxIndexSet<LocalDefId> {
         desc { "fetching potentially unused trait imports" }
     }
-    query names_imported_by_glob_use(def_id: LocalDefId) -> &'tcx FxHashSet<Symbol> {
+    query names_imported_by_glob_use(def_id: LocalDefId) -> &'tcx UnordSet<Symbol> {
         desc { |tcx| "finding names imported by glob use for `{}`", tcx.def_path_str(def_id.to_def_id()) }
     }
 

diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs
@@ -36,6 +36,7 @@ use rustc_data_structures::sharded::{IntoPointer, ShardedHashMap};
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_data_structures::steal::Steal;
 use rustc_data_structures::sync::{self, Lock, Lrc, MappedReadGuard, ReadGuard, WorkerLocal};
+use rustc_data_structures::unord::UnordSet;
 use rustc_errors::{
     DecorateLint, DiagnosticBuilder, DiagnosticMessage, ErrorGuaranteed, MultiSpan,
 };
@@ -2486,7 +2487,9 @@ pub fn provide(providers: &mut ty::query::Providers) {
     providers.maybe_unused_trait_imports =
         |tcx, ()| &tcx.resolutions(()).maybe_unused_trait_imports;
     providers.names_imported_by_glob_use = |tcx, id| {
-        tcx.arena.alloc(tcx.resolutions(()).glob_map.get(&id).cloned().unwrap_or_default())
+        tcx.arena.alloc(UnordSet::from(
+            tcx.resolutions(()).glob_map.get(&id).cloned().unwrap_or_default(),
+        ))
     };
 
     providers.extern_mod_stmt_cnum =

diff --git a/compiler/rustc_middle/src/ty/query.rs b/compiler/rustc_middle/src/ty/query.rs
@@ -41,7 +41,7 @@ use rustc_arena::TypedArena;
 use rustc_ast as ast;
 use rustc_ast::expand::allocator::AllocatorKind;
 use rustc_attr as attr;
-use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap, FxIndexSet};
+use rustc_data_structures::fx::{FxHashMap, FxIndexMap, FxIndexSet};
 use rustc_data_structures::steal::Steal;
 use rustc_data_structures::svh::Svh;
 use rustc_data_structures::sync::Lrc;
@@ -50,7 +50,9 @@ use rustc_data_structures::unord::UnordSet;
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir as hir;
 use rustc_hir::def::{DefKind, DocLinkResMap};
-use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, DefIdSet, LocalDefId};
+use rustc_hir::def_id::{
+    CrateNum, DefId, DefIdMap, DefIdSet, LocalDefId, LocalDefIdMap, LocalDefIdSet,
+};
 use rustc_hir::hir_id::OwnerId;
 use rustc_hir::lang_items::{LangItem, LanguageItems};
 use rustc_hir::{Crate, ItemLocalId, TraitCandidate};

diff --git a/compiler/rustc_mir_transform/src/check_unsafety.rs b/compiler/rustc_mir_transform/src/check_unsafety.rs
@@ -1,4 +1,4 @@
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::unord::{UnordItems, UnordSet};
 use rustc_errors::struct_span_err;
 use rustc_hir as hir;
 use rustc_hir::def::DefKind;
@@ -24,7 +24,7 @@ pub struct UnsafetyChecker<'a, 'tcx> {
     param_env: ty::ParamEnv<'tcx>,
 
     /// Used `unsafe` blocks in this function. This is used for the "unused_unsafe" lint.
-    used_unsafe_blocks: FxHashSet<HirId>,
+    used_unsafe_blocks: UnordSet<HirId>,
 }
 
 impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
@@ -129,7 +129,7 @@ impl<'tcx> Visitor<'tcx> for UnsafetyChecker<'_, 'tcx> {
                     let def_id = def_id.expect_local();
                     let UnsafetyCheckResult { violations, used_unsafe_blocks, .. } =
                         self.tcx.unsafety_check_result(def_id);
-                    self.register_violations(violations, used_unsafe_blocks.iter().copied());
+                    self.register_violations(violations, used_unsafe_blocks.items().copied());
                 }
             },
             _ => {}
@@ -151,7 +151,7 @@ impl<'tcx> Visitor<'tcx> for UnsafetyChecker<'_, 'tcx> {
                         let local_def_id = def_id.expect_local();
                         let UnsafetyCheckResult { violations, used_unsafe_blocks, .. } =
                             self.tcx.unsafety_check_result(local_def_id);
-                        self.register_violations(violations, used_unsafe_blocks.iter().copied());
+                        self.register_violations(violations, used_unsafe_blocks.items().copied());
                     }
                 }
             }
@@ -268,14 +268,14 @@ impl<'tcx> UnsafetyChecker<'_, 'tcx> {
             .lint_root;
         self.register_violations(
             [&UnsafetyViolation { source_info, lint_root, kind, details }],
-            [],
+            UnordItems::empty(),
         );
     }
 
     fn register_violations<'a>(
         &mut self,
         violations: impl IntoIterator<Item = &'a UnsafetyViolation>,
-        new_used_unsafe_blocks: impl IntoIterator<Item = HirId>,
+        new_used_unsafe_blocks: UnordItems<HirId, impl Iterator<Item = HirId>>,
     ) {
         let safety = self.body.source_scopes[self.source_info.scope]
             .local_data
@@ -308,9 +308,7 @@ impl<'tcx> UnsafetyChecker<'_, 'tcx> {
             }),
         };
 
-        new_used_unsafe_blocks.into_iter().for_each(|hir_id| {
-            self.used_unsafe_blocks.insert(hir_id);
-        });
+        self.used_unsafe_blocks.extend_unord(new_used_unsafe_blocks);
     }
     fn check_mut_borrowing_layout_constrained_field(
         &mut self,
@@ -407,7 +405,7 @@ enum Context {
 
 struct UnusedUnsafeVisitor<'a, 'tcx> {
     tcx: TyCtxt<'tcx>,
-    used_unsafe_blocks: &'a FxHashSet<HirId>,
+    used_unsafe_blocks: &'a UnordSet<HirId>,
     context: Context,
     unused_unsafes: &'a mut Vec<(HirId, UnusedUnsafe)>,
 }
@@ -458,7 +456,7 @@ impl<'tcx> intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'_, 'tcx> {
 fn check_unused_unsafe(
     tcx: TyCtxt<'_>,
     def_id: LocalDefId,
-    used_unsafe_blocks: &FxHashSet<HirId>,
+    used_unsafe_blocks: &UnordSet<HirId>,
 ) -> Vec<(HirId, UnusedUnsafe)> {
     let body_id = tcx.hir().maybe_body_owned_by(def_id);
 
@@ -505,7 +503,7 @@ fn unsafety_check_result(
     if body.is_custom_mir() {
         return tcx.arena.alloc(UnsafetyCheckResult {
             violations: Vec::new(),
-            used_unsafe_blocks: FxHashSet::default(),
+            used_unsafe_blocks: Default::default(),
             unused_unsafes: Some(Vec::new()),
         });
     }