Remove ordering traits from chalk_ir::interner::DefId

Use `indexmap` to obviate need for ordering `DefId`s.

Author: pierwill

Cargo.lock

@@ -20,3 +20,4 @@ chalk-solve = { version = "0.76.0-dev.0", path = "../chalk-solve" }
 chalk-recursive = { version = "0.76.0-dev.0", path = "../chalk-recursive" }
 chalk-engine = { version = "0.76.0-dev.0", path = "../chalk-engine" }
 chalk-parse = { version = "0.76.0-dev.0", path = "../chalk-parse" }
+indexmap = "1.7.0"

chalk-integration/Cargo.toml

@@ -57,7 +57,7 @@ use std::sync::Arc;
 /// (e.g., `SourceI` and `TargetI`) -- even if those type parameters
 /// wind up being mapped to the same underlying type families in the
 /// end.
-pub trait Interner: Debug + Copy + Eq + Ord + Hash + Sized {
+pub trait Interner: Debug + Copy + Eq + Hash + Sized {
     /// "Interned" representation of types.  In normal user code,
     /// `Self::InternedType` is not referenced. Instead, we refer to
     /// `Ty<Self>`, which wraps this type.
@@ -188,10 +188,10 @@ pub trait Interner: Debug + Copy + Eq + Ord + Hash + Sized {
     type InternedVariances: Debug + Clone + Eq + Hash;
 
     /// The core "id" type used for trait-ids and the like.
-    type DefId: Debug + Copy + Eq + Ord + Hash;
+    type DefId: Debug + Copy + Eq + Hash;
 
     /// The ID type for ADTs
-    type InternedAdtId: Debug + Copy + Eq + Ord + Hash;
+    type InternedAdtId: Debug + Copy + Eq + Hash;
 
     /// Representation of identifiers.
     type Identifier: Debug + Clone + Eq + Hash;

chalk-ir/src/interner.rs

@@ -20,6 +20,7 @@ rustc-hash = { version = "1.1.0" }
 
 chalk-derive = { version = "0.76.0-dev.0", path = "../chalk-derive" }
 chalk-ir = { version = "0.76.0-dev.0", path = "../chalk-ir" }
+indexmap = "1.7.0"
 
 [dev-dependencies]
 chalk-integration = { path = "../chalk-integration" }

chalk-solve/Cargo.toml

@@ -4,10 +4,11 @@ use crate::solve::Solver;
 use crate::RustIrDatabase;
 use chalk_ir::interner::Interner;
 use chalk_ir::{self, ImplId, TraitId};
-use std::collections::BTreeMap;
 use std::fmt;
 use std::sync::Arc;
 
+use indexmap::IndexMap;
+
 pub mod orphan;
 mod solve;
 
@@ -42,13 +43,13 @@ impl<I: Interner> std::error::Error for CoherenceError<I> {}
 /// This basically encodes which impls specialize one another.
 #[derive(Clone, Debug, Default, PartialEq, Eq)]
 pub struct SpecializationPriorities<I: Interner> {
-    map: BTreeMap<ImplId<I>, SpecializationPriority>,
+    map: IndexMap<ImplId<I>, SpecializationPriority>,
 }
 
 impl<I: Interner> SpecializationPriorities<I> {
     pub fn new() -> Self {
         Self {
-            map: BTreeMap::new(),
+            map: IndexMap::new(),
         }
     }
 
@@ -60,8 +61,7 @@ impl<I: Interner> SpecializationPriorities<I> {
     /// Store the priority of an impl (used during construction).
     /// Panics if we have already stored the priority for this impl.
     fn insert(&mut self, impl_id: ImplId<I>, p: SpecializationPriority) {
-        let old_value = self.map.insert(impl_id, p);
-        assert!(old_value.is_none());
+        self.map.insert(impl_id, p);
     }
 }
 
@@ -106,18 +106,24 @@ where
 
     // Build the forest of specialization relationships.
     fn build_specialization_forest(&self) -> Result<Graph<ImplId<I>, ()>, CoherenceError<I>> {
-        // The forest is returned as a graph but built as a GraphMap; this is
-        // so that we never add multiple nodes with the same ItemId.
-        let mut forest = DiGraphMap::new();
+        let mut forest = DiGraph::new();
+
+        let node_impls: Vec<ImplId<_>> = forest.raw_nodes().iter().map(|x| x.weight).collect();
 
         // Find all specializations (implemented in coherence/solve)
         // Record them in the forest by adding an edge from the less special
         // to the more special.
         self.visit_specializations_of_trait(|less_special, more_special| {
-            forest.add_edge(less_special, more_special, ());
+            // Check so that we never add multiple nodes with the same ImplId.
+            if !node_impls.contains(&less_special) && !node_impls.contains(&more_special) {
+                let l = forest.add_node(less_special);
+                let m = forest.add_node(more_special);
+
+                forest.add_edge(l, m, ());
+            }
         })?;
 
-        Ok(forest.into_graph())
+        Ok(forest)
     }
 
     // Recursively set priorities for those node and all of its children.

chalk-solve/src/coherence.rs

@@ -1,4 +1,3 @@
-//! Persistent state passed down between writers.
 //!
 //! This is essentially `InternalWriterState` and other things supporting that.
 use std::{
@@ -12,6 +11,7 @@ use std::{
 
 use crate::RustIrDatabase;
 use chalk_ir::{interner::Interner, *};
+use indexmap::IndexMap;
 use itertools::Itertools;
 
 /// Like a BoundVar, but with the debrujin index inverted so as to create a
@@ -36,31 +36,31 @@ impl Display for InvertedBoundVar {
     }
 }
 
-#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
 enum UnifiedId<I: Interner> {
     AdtId(I::InternedAdtId),
     DefId(I::DefId),
 }
 
 #[derive(Debug)]
-pub struct IdAliasStore<T: Ord> {
+pub struct IdAliasStore<T> {
     /// Map from the DefIds we've encountered to a u32 alias id unique to all ids
     /// the same name.
-    aliases: BTreeMap<T, u32>,
+    aliases: IndexMap<T, u32>,
     /// Map from each name to the next unused u32 alias id.
     next_unused_for_name: BTreeMap<String, u32>,
 }
 
-impl<T: Ord> Default for IdAliasStore<T> {
+impl<T> Default for IdAliasStore<T> {
     fn default() -> Self {
         IdAliasStore {
-            aliases: BTreeMap::default(),
+            aliases: IndexMap::default(),
             next_unused_for_name: BTreeMap::default(),
         }
     }
 }
 
-impl<T: Copy + Ord> IdAliasStore<T> {
+impl<T: Copy + Eq + core::hash::Hash> IdAliasStore<T> {
     fn alias_for_id_name(&mut self, id: T, name: String) -> String {
         let next_unused_for_name = &mut self.next_unused_for_name;
         let alias = *self.aliases.entry(id).or_insert_with(|| {

chalk-solve/src/display/state.rs

@@ -2,8 +2,6 @@
 //! `.chalk` files containing those definitions.
 use std::{
     borrow::Borrow,
-    cmp::{Ord, Ordering},
-    collections::BTreeSet,
     fmt::{self, Debug, Display},
     io::Write,
     marker::PhantomData,
@@ -18,6 +16,8 @@ use crate::{
 };
 use chalk_ir::{interner::Interner, *};
 
+use indexmap::IndexSet;
+
 mod id_collector;
 
 /// Wraps another `RustIrDatabase` (`DB`) and records which definitions are
@@ -36,7 +36,7 @@ where
     I: Interner,
 {
     ws: WriterState<I, DB, P>,
-    def_ids: Mutex<BTreeSet<RecordedItemId<I>>>,
+    def_ids: Mutex<IndexSet<RecordedItemId<I>>>,
     _phantom: PhantomData<DB>,
 }
 
@@ -535,7 +535,7 @@ where
     }
 }
 
-#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
 pub enum RecordedItemId<I: Interner> {
     Adt(AdtId<I>),
     Trait(TraitId<I>),
@@ -580,38 +580,3 @@ impl<I: Interner> From<GeneratorId<I>> for RecordedItemId<I> {
         RecordedItemId::Generator(v)
     }
 }
-
-/// Utility for implementing Ord for RecordedItemId.
-#[derive(PartialEq, Eq, PartialOrd, Ord)]
-enum OrderedItemId<'a, DefId, AdtId> {
-    DefId(&'a DefId),
-    AdtId(&'a AdtId),
-}
-
-impl<I: Interner> RecordedItemId<I> {
-    /// Extract internal identifier. Allows for absolute ordering matching the
-    /// order in which chalk saw things (and thus reproducing that order in
-    /// printed programs)
-    fn ordered_item_id(&self) -> OrderedItemId<'_, I::DefId, I::InternedAdtId> {
-        match self {
-            RecordedItemId::Trait(TraitId(x))
-            | RecordedItemId::Impl(ImplId(x))
-            | RecordedItemId::OpaqueTy(OpaqueTyId(x))
-            | RecordedItemId::Generator(GeneratorId(x))
-            | RecordedItemId::FnDef(FnDefId(x)) => OrderedItemId::DefId(x),
-            RecordedItemId::Adt(AdtId(x)) => OrderedItemId::AdtId(x),
-        }
-    }
-}
-
-impl<I: Interner> PartialOrd for RecordedItemId<I> {
-    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
-        Some(self.cmp(other))
-    }
-}
-
-impl<I: Interner> Ord for RecordedItemId<I> {
-    fn cmp(&self, other: &Self) -> Ordering {
-        self.ordered_item_id().cmp(&other.ordered_item_id())
-    }
-}

chalk-solve/src/logging_db.rs

@@ -6,9 +6,10 @@ use chalk_ir::{
     visit::{SuperVisit, Visit},
     AliasTy, DebruijnIndex, TyKind, WhereClause,
 };
-use std::collections::BTreeSet;
 use std::ops::ControlFlow;
 
+use indexmap::IndexSet;
+
 /// Collects the identifiers needed to resolve all the names for a given
 /// set of identifers, excluding identifiers we already have.
 ///
@@ -34,11 +35,11 @@ use std::ops::ControlFlow;
 /// resolution is successful.
 pub fn collect_unrecorded_ids<'i, I: Interner, DB: RustIrDatabase<I>>(
     db: &'i DB,
-    identifiers: &'_ BTreeSet<RecordedItemId<I>>,
-) -> BTreeSet<RecordedItemId<I>> {
+    identifiers: &'_ IndexSet<RecordedItemId<I>>,
+) -> IndexSet<RecordedItemId<I>> {
     let mut collector = IdCollector {
         db,
-        found_identifiers: BTreeSet::new(),
+        found_identifiers: IndexSet::new(),
     };
     for id in identifiers {
         match *id {
@@ -96,7 +97,7 @@ pub fn collect_unrecorded_ids<'i, I: Interner, DB: RustIrDatabase<I>>(
 
 struct IdCollector<'i, I: Interner, DB: RustIrDatabase<I>> {
     db: &'i DB,
-    found_identifiers: BTreeSet<RecordedItemId<I>>,
+    found_identifiers: IndexSet<RecordedItemId<I>>,
 }
 
 impl<'i, I: Interner, DB: RustIrDatabase<I>> IdCollector<'i, I, DB> {