Clarify some interning details

`Interned` does pointer equality/hashing, which is valid in two cases.
- The values are guaranteed to be unique (e.g. via interning, or
  construction). This is how `rustc_middle` uses `Interned`.
- The type has "identity" and different values should be considered
  distinct even if they are identical. This is how `rustc_resolve` uses
  `Interned`.

PR 137202 tried to clarify things by adding a `T: Hash` constraint to
`Interned<'a, T>`. This constraint isn't actually used, because
`Interned` is hashed based on pointer value, not contents. But it was
intended to communicate the idea that a type stored in `Interned` is
actually interned, which is likely to be done with hashing. Panicking
impls of `Hash` were added for the relevant `rustc_resolve` types to
work around the fact that it doesn't use hashing-based interning.

In my opinion PR 137202 didn't improve things. The `T: Hash` constraint
is only aimed at the interning case, and even for that case it's not
quite right because you could use a `BTreeMap` to intern instead of a
`HashMap`.

This commit does several things.
- Removes the `T: Hash` constraint and the `Hash` impls for
  `rustc_resolve` types added in PR 137202.
- Improves the comments on `Interned` to cover the non-interning cases.
- Removes the `PartialOrd`/`Ord` impls on `Interned` because (a) they're
  not used, and (b) their meaning is unclear for the "identity" case.
- Improves the documentation in `rustc_resolve` to explain how
  `Interned` usage is valid there.

compiler/rustc_data_structures/src/intern.rs

@@ -1,4 +1,3 @@
-use std::cmp::Ordering;
 use std::fmt::{self, Debug};
 use std::hash::{Hash, Hasher};
 use std::ops::Deref;
@@ -11,26 +10,40 @@ mod private {
     pub struct PrivateZst;
 }
 
-/// A reference to a value that is interned, and is known to be unique.
+/// This type is a reference with one special behaviour: the reference pointer (i.e. the address of
+/// the value referred to) is used for equality and hashing, rather than the value's contents, as
+/// would occur with a vanilla reference. There are two cases when this is useful.
 ///
-/// Note that it is possible to have a `T` and a `Interned<T>` that are (or
-/// refer to) equal but different values. But if you have two different
-/// `Interned<T>`s, they both refer to the same value, at a single location in
-/// memory. This means that equality and hashing can be done on the value's
-/// address rather than the value's contents, which can improve performance.
+/// - Types where uniqueness is guaranteed. This is most commonly achieved via interning -- hence
+///   the name `Interned` -- though it may also be possible via other means. In this case, the use
+///   of `Interned` is primarily a performance optimization, because pointer equality/hashing gives
+///   the same results as value equality/hashing, but is faster. (The use of the `Interned` type
+///   also provides documentation about the interned-ness.)
 ///
-/// The `PrivateZst` field means you can pattern match with `Interned(v, _)`
-/// but you can only construct a `Interned` with `new_unchecked`, and not
-/// directly.
+///   Note that in this case it is possible to have a `T` and a `Interned<T>` that are (or refer
+///   to) equal but different values. But if you have two different `Interned<T>`s, they both refer
+///   to the same value, at a single location in memory.
+///
+/// - Types with identity, where distinct values should always be considered unequal, even if they
+///   have equal values. These are rare in Rust, but do occur sometimes. In this case, the use of
+///   `Interned` gives different behaviour, because pointer equality/hashing gives different result
+///   to value equality/hashing, and is also faster.
+///
+/// The `PrivateZst` field means you can pattern match with `Interned(v, _)` but you can only
+/// construct a `Interned` with `new_unchecked`, and not directly. This means that all creation
+/// points can be audited easily.
 #[rustc_pass_by_value]
 pub struct Interned<'a, T>(pub &'a T, pub private::PrivateZst);
 
 impl<'a, T> Interned<'a, T> {
-    /// Create a new `Interned` value. The value referred to *must* be interned
-    /// and thus be unique, and it *must* remain unique in the future. This
-    /// function has `_unchecked` in the name but is not `unsafe`, because if
-    /// the uniqueness condition is violated condition it will cause incorrect
-    /// behaviour but will not affect memory safety.
+    /// Create a new `Interned` value. The value referred to *must* satisfy one of the following
+    /// two conditions.
+    /// - It must be unique and it must remain unique in the future.
+    /// - It must be of a type with "identity" such that distinct values should always be
+    ///   considered unequal.
+    ///
+    /// This function has `_unchecked` in the name but is not `unsafe`, because if neither of these
+    /// conditions is met it will cause incorrect behaviour but will not affect memory safety.
     #[inline]
     pub const fn new_unchecked(t: &'a T) -> Self {
         Interned(t, private::PrivateZst)
@@ -64,41 +77,10 @@ impl<'a, T> PartialEq for Interned<'a, T> {
 
 impl<'a, T> Eq for Interned<'a, T> {}
 
-impl<'a, T: PartialOrd> PartialOrd for Interned<'a, T> {
-    fn partial_cmp(&self, other: &Interned<'a, T>) -> Option<Ordering> {
-        // Pointer equality implies equality, due to the uniqueness constraint,
-        // but the contents must be compared otherwise.
-        if ptr::eq(self.0, other.0) {
-            Some(Ordering::Equal)
-        } else {
-            let res = self.0.partial_cmp(other.0);
-            debug_assert_ne!(res, Some(Ordering::Equal));
-            res
-        }
-    }
-}
-
-impl<'a, T: Ord> Ord for Interned<'a, T> {
-    fn cmp(&self, other: &Interned<'a, T>) -> Ordering {
-        // Pointer equality implies equality, due to the uniqueness constraint,
-        // but the contents must be compared otherwise.
-        if ptr::eq(self.0, other.0) {
-            Ordering::Equal
-        } else {
-            let res = self.0.cmp(other.0);
-            debug_assert_ne!(res, Ordering::Equal);
-            res
-        }
-    }
-}
-
-impl<'a, T> Hash for Interned<'a, T>
-where
-    T: Hash,
-{
+impl<'a, T> Hash for Interned<'a, T> {
     #[inline]
     fn hash<H: Hasher>(&self, s: &mut H) {
-        // Pointer hashing is sufficient, due to the uniqueness constraint.
+        // Pointer hashing is sufficient.
         ptr::hash(self.0, s)
     }
 }

compiler/rustc_data_structures/src/intern/tests.rs

@@ -1,5 +1,6 @@
 use super::*;
 
+#[allow(unused)]
 #[derive(Debug)]
 struct S(u32);
 
@@ -11,22 +12,6 @@ impl PartialEq for S {
 
 impl Eq for S {}
 
-impl PartialOrd for S {
-    fn partial_cmp(&self, other: &S) -> Option<Ordering> {
-        // The `==` case should be handled by `Interned`.
-        assert_ne!(self.0, other.0);
-        self.0.partial_cmp(&other.0)
-    }
-}
-
-impl Ord for S {
-    fn cmp(&self, other: &S) -> Ordering {
-        // The `==` case should be handled by `Interned`.
-        assert_ne!(self.0, other.0);
-        self.0.cmp(&other.0)
-    }
-}
-
 #[test]
 fn test_uniq() {
     let s1 = S(1);
@@ -45,14 +30,4 @@ fn test_uniq() {
     assert_eq!(v1, v1);
     assert_eq!(v3a, v3b);
     assert_ne!(v1, v4); // same content but different addresses: not equal
-
-    assert_eq!(v1.cmp(&v2), Ordering::Less);
-    assert_eq!(v3a.cmp(&v2), Ordering::Greater);
-    assert_eq!(v1.cmp(&v1), Ordering::Equal); // only uses Interned::eq, not S::cmp
-    assert_eq!(v3a.cmp(&v3b), Ordering::Equal); // only uses Interned::eq, not S::cmp
-
-    assert_eq!(v1.partial_cmp(&v2), Some(Ordering::Less));
-    assert_eq!(v3a.partial_cmp(&v2), Some(Ordering::Greater));
-    assert_eq!(v1.partial_cmp(&v1), Some(Ordering::Equal)); // only uses Interned::eq, not S::cmp
-    assert_eq!(v3a.partial_cmp(&v3b), Some(Ordering::Equal)); // only uses Interned::eq, not S::cmp
 }

compiler/rustc_resolve/src/imports.rs

@@ -210,23 +210,10 @@ pub(crate) struct ImportData<'ra> {
     pub on_unknown_attr: Option<OnUnknownData>,
 }
 
-/// All imports are unique and allocated on a same arena,
-/// so we can use referential equality to compare them.
+/// `Interned` is used because values of this type have "identity" and compare as unequal even if
+/// they have the same contents.
 pub(crate) type Import<'ra> = Interned<'ra, ImportData<'ra>>;
 
-// Allows us to use Interned without actually enforcing (via Hash/PartialEq/...) uniqueness of the
-// contained data.
-// FIXME: We may wish to actually have at least debug-level assertions that Interned's guarantees
-// are upheld.
-impl std::hash::Hash for ImportData<'_> {
-    fn hash<H>(&self, _: &mut H)
-    where
-        H: std::hash::Hasher,
-    {
-        unreachable!()
-    }
-}
-
 impl<'ra> ImportData<'ra> {
     pub(crate) fn is_glob(&self) -> bool {
         matches!(self.kind, ImportKind::Glob { .. })
@@ -291,6 +278,8 @@ pub(crate) struct NameResolution<'ra> {
     pub orig_ident_span: Span,
 }
 
+/// `Interned` is used because values of this type have "identity" and compare as unequal even if
+/// they have the same contents.
 pub(crate) type NameResolutionRef<'ra> = Interned<'ra, CmRefCell<NameResolution<'ra>>>;
 
 impl<'ra> NameResolution<'ra> {

compiler/rustc_resolve/src/lib.rs

@@ -682,8 +682,8 @@ struct ModuleData<'ra> {
     self_decl: Option<Decl<'ra>>,
 }
 
-/// All modules are unique and allocated on a same arena,
-/// so we can use referential equality to compare them.
+/// `Interned` is used because values of this type have "identity" and compare as unequal even if
+/// they have the same contents.
 #[derive(Clone, Copy, PartialEq, Eq, Hash)]
 #[rustc_pass_by_value]
 struct Module<'ra>(Interned<'ra, ModuleData<'ra>>);
@@ -698,19 +698,6 @@ struct LocalModule<'ra>(Interned<'ra, ModuleData<'ra>>);
 #[rustc_pass_by_value]
 struct ExternModule<'ra>(Interned<'ra, ModuleData<'ra>>);
 
-// Allows us to use Interned without actually enforcing (via Hash/PartialEq/...) uniqueness of the
-// contained data.
-// FIXME: We may wish to actually have at least debug-level assertions that Interned's guarantees
-// are upheld.
-impl std::hash::Hash for ModuleData<'_> {
-    fn hash<H>(&self, _: &mut H)
-    where
-        H: std::hash::Hasher,
-    {
-        unreachable!()
-    }
-}
-
 impl<'ra> ModuleData<'ra> {
     fn new(
         parent: Option<Module<'ra>>,
@@ -915,6 +902,7 @@ impl<'ra> LocalModule<'ra> {
         assert!(kind.is_local());
         let parent = parent.map(|m| m.to_module());
         let data = ModuleData::new(parent, kind, expn_id, span, no_implicit_prelude, vis, arenas);
+        // SAFETY: `Interned` is valid because values of this type have "identity".
         LocalModule(Interned::new_unchecked(arenas.modules.alloc(data)))
     }
 
@@ -936,6 +924,7 @@ impl<'ra> ExternModule<'ra> {
         assert!(!kind.is_local());
         let parent = parent.map(|m| m.to_module());
         let data = ModuleData::new(parent, kind, expn_id, span, no_implicit_prelude, vis, arenas);
+        // SAFETY: `Interned` is valid because values of this type have "identity".
         ExternModule(Interned::new_unchecked(arenas.modules.alloc(data)))
     }
 
@@ -1000,23 +989,10 @@ struct DeclData<'ra> {
     parent_module: Option<Module<'ra>>,
 }
 
-/// All name declarations are unique and allocated on a same arena,
-/// so we can use referential equality to compare them.
+/// `Interned` is used because values of this type have "identity" and compare as unequal even if
+/// they have the same contents.
 type Decl<'ra> = Interned<'ra, DeclData<'ra>>;
 
-// Allows us to use Interned without actually enforcing (via Hash/PartialEq/...) uniqueness of the
-// contained data.
-// FIXME: We may wish to actually have at least debug-level assertions that Interned's guarantees
-// are upheld.
-impl std::hash::Hash for DeclData<'_> {
-    fn hash<H>(&self, _: &mut H)
-    where
-        H: std::hash::Hasher,
-    {
-        unreachable!()
-    }
-}
-
 /// Name declaration kind.
 #[derive(Debug)]
 enum DeclKind<'ra> {
@@ -1584,12 +1560,15 @@ impl<'ra> ResolverArenas<'ra> {
     }
 
     fn alloc_decl(&'ra self, data: DeclData<'ra>) -> Decl<'ra> {
+        // SAFETY: `Interned` is valid because values of this type have "identity".
         Interned::new_unchecked(self.dropless.alloc(data))
     }
     fn alloc_import(&'ra self, import: ImportData<'ra>) -> Import<'ra> {
+        // SAFETY: `Interned` is valid because values of this type have "identity".
         Interned::new_unchecked(self.imports.alloc(import))
     }
     fn alloc_name_resolution(&'ra self, orig_ident_span: Span) -> NameResolutionRef<'ra> {
+        // SAFETY: `Interned` is valid because values of this type have "identity".
         Interned::new_unchecked(
             self.name_resolutions.alloc(CmRefCell::new(NameResolution::new(orig_ident_span))),
         )