The Hashtable Problem: A Litmus Test for External Impl Proposals

The Hashtable Problem: A Litmus Test for External Impl Proposals

Any proposal aiming to relax the orphan rule or enable external trait implementations must address the hashtable problem. I met the problem when I wrote my own Named Impl Draft. I selected a solution but it breaked by the indirect implementation sample.

Explain

The earliest article is The "Hashtable Problem" from nikomatsakis at 2011-12:

There is a danger with typeclasses only on functions because they permit a lack of coherence. This danger is particularly acute with collections, where the definition of a particular interface---such as hashable or ord---may be used in structuring the data structure. This renders the data structure corrupt when used with a different implementation.

Also there is a comment from @nikomatsakis 2017-2:

We called it the hashtable problem because – imagine you had a hashtable with keys to type K that is built up using one impl of Hash, but then you pass that hashtable to another module, where a distinct Hash impl is in scope. It’s going to be pandemonium. What that e-mail proposed was to solve this by making the impl “part of the type”, in a sense. At the end of the day it seemed like a lot of complexity and we opted against it – but there are real costs. I’m not sure of the best fix here.

The problem affects:

  • struct HashMap with trait Hash
  • struct BTreeMap with trait Ord

Sample codes

  1. example in typical scoped impl
let mut map = HashMap::<i32, &'static str>::new();
map.insert(42, "the answer to the universe"); // default hash of i32
{
     use AnotherHashForI32 as impl Hash for i32;
     let found = map.get(&42); // a different hash of i32, can we find the answer?
}
  1. example in typical named impl
let mut map = HashMap::<i32, &'static str>::new();
map.insert(42, "the answer to the universe"); // default hash of i32

let altmap = &map as &HashMap<i32 + Hash use AnotherHashForI32, &'static str>;
let found = altmap.get(&(42 as (i32 + Hash use AnotherHashForI32))); // a different hash of i32, can we find the answer?

Where is the problem from?

let key = 42;
let mut map = HashMap::<i32, &'static str>::new();
map.insert(key, "the answer to the universe"); // default hash of i32
let found = map.get(&key); // ok

let altkey = key as (i32 + Hash use AnotherHashForI32);
let mut altmap = HashMap::<i32 + Hash use AnotherHashForI32, &'static str>::new();
altmap.insert(altkey, "the answer to the universe"); // a different hash of i32
let found = altmap.get(&altkey);  // ok

The code above works fine.

The problem is from the type casting of the container:

let altmap = &map as &HashMap<i32 + Hash use AnotherHashForI32, &'static str>;

With scoped impl, there are also implicitly type casting:

let mut map = HashMap::<i32, &'static str>::new();
map.insert(42, "the answer to the universe");
{
     use AnotherHashForI32 as impl Hash for i32;
     let found = map.get(&42); // `map` is casted, and `&42` is casted
}

Solutions (partly)

  1. the strictest way
    Only allow casting of T <-> T + Impl, &T <-> &(T + Impl), and &mut T <-> &mut(T + Impl)
    This is just a better newtype pattern.

  2. the most detailed way
    Disable implementation casting from HashMap<K + Hash use HashImpl1, _> to HashMap<K + Hash use HashImpl2, _>

    pub struct HashMap<#[disable_impl_casting(Hash)]K: Hash, V> {}
    

    Unsolved problems:

    • inner different implementation HashMap<(i32 + Hash use HashImpl1, i32), _>
    • indirect implements
  3. disable implementation casting on HashMap

    #[disable_impl_casting]
    pub struct HashMap<K, V> {}
    
  4. disable implementation casting on HashMap with K

    pub struct HashMap<#[disable_impl_casting(all)]K: Hash, V> {}
    
  5. disable external impl of Hash In My Named impl draft, this is selected:

    #[disable_named_impl(exist_default)]
    pub trait Hash {}
    

    Unsolved problems:

    • indirect implementation

Progressive Hashtable Violations via External Impls

  1. different implementation the Key with a different Hash implementation

    impl Hash for i32 use AlterHash {
        fn hash<H: Hasher>(&self, state: &mut H) {
            self.hash(state);
            8.hash(state);
        }
    }
    
    let map = HashMap::<i32, i32>::new();
    map.insert(42, 42);
    
    type AltKey = i32 + Hash use AlterHash;
    let altmap = &map as &HashMap<AltKey, i32>;
    let altkey = 42 as AltKey;
    let found = altmap.get(&altkey);
    
  2. inner different implementation the inner type of Key with a different Hash implementation

    impl Hash for i32 use AlterHash {
        fn hash<H: Hasher>(&self, state: &mut H) {
            self.hash(state);
            8.hash(state);
        }
    }
    
    let map = HashMap::<(i32,), i32>::new();
    map.insert((42,), 42);
    
    type AltKey = (i32 + Hash use AlterHash,);
    let altmap = &map as &HashMap<AltKey, i32>;
    let altkey = (42,) as AltKey;
    let found = altmap.get(&altkey);
    
  3. indirect implementation the inner type of Key with a different Display implementation but affects Hash of Key

    struct Key<T: Display>(T);
    impl<T: Display> Hash for Key<T> {
        fn hash<H: Hasher>(&self, state: &mut H) {
            format!("{}", self.0).hash(state);
        }
    }
    
    impl Display for i32 use AlterDisplay {
        fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
            write!(f, "I32AlterDisplay")
        }
    }
    
    let map = HashMap::<Key<i32>, i32>::new();
    map.insert(Key(42), 42);
    
    type AltKey = Key<i32 + Display use AlterDisplay>;
    let altmap = &map as &HashMap<AltKey, i32>;
    let altkey = Key(42) as AltKey;
    let found = altmap.get(&altkey);
    

Analyzing Whether Existing Forum Proposals Can Solve the Three Issues Above

1 Like

Because it was not obvious to me the first time I encountered this problem, the requirement of K: Hash is not on the HashMap type in Rust; it's on the impl block containing operations that hash. Therefore "the Hash implementation is part of the type that depends on Hash" is not a solution for the HashMap in std.

4 Likes

All solutions from 2 to 5 are in my opinion unacceptable, as they require extensive changes to existing code and hence break any code that is not updated. You're basically answering the hashtable problem by saying "let's instead consider a hashtable that doesn't have this problem", but this doesn't solve the issue for the original hashtable.

4 Likes

There's another wrinkle to this which is type-erasing/type-keyed hashtables, which a "plain" solution over static typing may not catch. You can best read my attempt (not independently reviewed) at solving both here: rust-rfcs/text/3634-scoped-impl-trait-for-type.md at 251e4c443757ebe7c079a8a3627e32f9b79d4d0e · Tamschi/rust-rfcs · GitHub (version-agnostic link, in case I update it, in case I update it).

There's another common problem with orphan-rule relaxations, which is external-global implementation mixing in multi-trait bounds breaking cross-impl proofs valid for existing code: https://github.com/rust-lang/rfcs/pull/3634#issuecomment-2153325753
This can be worked around without changes, but in my opinion that would break ergonomics badly for what in practice is a very rare problem. I think it's feasible to change what impl blocks can assume across an edition boundary though, along with an attribute to apply the previous mixing constraint explicitly. I haven't (yet, hopefully) found the time to revise for that, though, so my RFC is unsound in that regard. You can find a sketch of a possible solution in the GitHub comments, starting here: Scoped `impl Trait for Type` by Tamschi · Pull Request #3634 · rust-lang/rfcs · GitHub

This is only true for proposals that intend to allow overlapping implementations.

I'd love to be able to declare "I depend on the two unrelated crates miniserde and jiff" in my Cargo.toml[1]. By unrelated I mean that miniserde does not depend on jiff nor does jiff depend on miniserde. Having ruled that out, and assuming that neither adds any blanket impls or a dependency on the other[2], I ought to be able to define impls of miniserde::Serialize for jiff structs without risk of overlap due to changes upstream.

I don't remember instances of running into the orphan rule that wouldn't be solved by such a construct.


  1. This would need to be enforced by cargo. ↩︎

  2. This would be a new "possibly-breaking change" to be listed in the SemVer Compatibility chapter. This rule may seem dubious, but in practice it seems defensible. Assuming a new crate such as jiff-miniserde or miniserde-jiff, I think such breakage would be highly unlikely in practice. ↩︎

Without further additions, at least. It's afaict okay as long as the impl identity constraint is enforced for any function calls where both implementing and depending type appear in the signature bounds-linked (even indirectly, so basically any case where those types are linked by relevant type parameter bounds).

Right. The main problem there is that disallowing them would make most added trait implementations a breaking change for any library crates.

This would not be (much of) an issue for executable-only crates, which would already cover a lot of ground in practice. There may be some ecosystem tradeoffs in terms of fewer stand-alone binaries being made also usable as libraries.

  • Crate A defines struct Key
  • Crate B defines: impl Hash for Key use AlterHashB
  • Crate C defines: impl Hash for Key use AlterHashC
  • The bin depends A,B,C, there is also casting from HashMap<Key + Hash use AlterHashB, _> to HashMap<Key + Hash use AlterHashC, _>

This works if only one crate can do this (the binary crate?)

But if two crates do this, then those impls can overlap.

My preferred solution for those kinds of global constraints is to just fail compilation in that case (the problem is, fail where? in the linker?). And more importantly, disallow uploading such crates with non-local impls to crates.io (make it require a field in Cargo.toml that says the crate is not for consumption of the wide ecosystem)

So you can have some odd, non-composable crate in your own project, and it will be perpetually fragile (what if a new version of jiff depends on miniserde), and that's okay because it's contained. But you absolutely shouldn't be able to publish this duct-taped crate to the wider ecosystem and allow other crates depend on it.

2 Likes

My core point was supposed to be that, in my experience, overlapping impls aren't necessary and that the hashtable problem may be a bad and/or overly strict test.

(The rest of this post is going somewhat off-topic.)

Absolutely. Only one crate in the crate graph can be allowed to exploit the independence between miniserde and jiff.

Yes, compilation should fail. In Ben's world of fantasy and wonder, modules and crates could be generic. Importantly, that would allow adding (trivial) bounds. Any crate could then declare that jiff::Timestamp: miniserde::Serialize must hold. These constraints would "bubble up" to the binary crate which would have to ensure they hold.

I think this is letting the perfect be the enemy of the good. It's unlikely that jiff will start to depend on miniserde. So the expected cost is low and the expected utility is huge: Fewer implementations means less effort spent, fewer sources of bugs, and increased compatibility.

That is true. For maximum flexibility, there would have to be separate packages for the library and the binary.

I think the best answer is instead to add a level of indirection, not unlike we have with the buildhasher already.

If I use a CustomizedBinaryHeap<MyCustomStrategy, T> (where BinaryHeap<T> = CustomizedBinaryHeap<DefaultOrdStrategy, T>), then you can put any trait impl you want on your local MyCustomStrategy type. So even if the T is some external type, that's fine.

That way the instances carry around the necessarily "impls" without needing all these extra features for adding new ways to specify things.

It's a good pattern for e.g. ordered containers I think, but it wouldn't solve the newtype pain points that external trait impls would cover: rust-rfcs/text/3634-scoped-impl-trait-for-type.md at 251e4c443757ebe7c079a8a3627e32f9b79d4d0e · Tamschi/rust-rfcs · GitHub

Implementing it for everything that takes bounded generics also seems very noisy to me in terms of API surface.

After thinking about this for ages, I've realised that the problem isn't really a problem with respect to types like HashMap for which the implemented trait is not unsafe. Not including K: Hash on the HashMap type itself means that it's impossible for external-impl proposals to ensure that HashMap is coherent in the sense of always hashing the same keys to the same values (because a different Hash impl could be used for each call); but even in current Rust, HashMap has numerous ways in which a programmer could cause incoherent hashing in a way that won't be caught by the compiler:

  • (edit: I corrected this in response to pitaj's correction) Hash is a safe trait, so nothing forces K: Hash and &K: Hash Q: Hash (where Q: Borrow<K> is used to do hash lookups) to have any relationship to each other, but it is possible to add keys to a hash as K and then retrieve them using a Q;
  • Hash is a safe trait, so nothing is forcing it to always hash the same key to the same hash (it could, e.g., return a random number)
  • Even if Hash is deterministic, it is possible to use interior mutability to modify the keys stored in a hash table (while they are being stored) in order to change the value they would hash to.

HashMap's documentation contains plenty of warnings against doing this sort of thing, but it is just warnings in the documentation (and in particular its memory safety is not allowed to depend on the programmer doing this). As such, a change to Rust to allow external impls wouldn't actually break HashMap in any ways that it isn't already broken: it would allow people to try to retrieve keys using a different hashing function from the one used to add them (which of course doesn't work), but there are plenty of ways to do that even without external impls.

In general, it seems impossible for incoherent implementations of a safe trait to break existing code, as long as all the trait's associated items are methods (i.e. it has no associated types (including RPITIT as a special case of associated types) or associated constants). That's because code like this, which is the case that breaks (stealing the syntax from the OP):

let g: Generic<T> = Generic::new();
(&g as &Generic<T + Trait use Impl1>).method();
(&g as &Generic<T + Trait use Impl2>).method();

could be replicated without external impls by doing something like this:

thread_local! { static USE_IMPL_2: Cell<bool> = const { Cell::new(false); }; }
impl Trait for T {
    // for each method in the trait, you write a small wrapper like this
    fn f(&self) {
        if USE_IMPL_2.get() { self.f2(); } else { self.f1(); }
    }
}

let g: Generic<T> = Generic::new();
USE_IMPL_2.set(false);
g.method();
USE_IMPL_2.set(true);
g.method();

This specifically only works for methods, because associated types or constants can't depend on the value of a global variable. (It also doesn't work for unsafe traits, because they may have safety requirements that prohibit this sort of implementation.) It strikes me that this is very similar for the rules of dyn compatibility (the only exception is that dyn compatibility requires the ability to make a vtable, whereas this doesn't), which is probably not entirely a coincidence.

All this is making me think that external impls should probably be designed so that putting a bound on the type itself (as opposed to the impl block) is the correct way to say "this trait must always be implemented the same way whenever a method of this type is called": for safe, dyn-compatible traits, this seems to be entirely backwards compatible with current Rust, and it also fits my expectation of how trait bounds should work (and is consistent with how type generics are specified: if the type parameter needs to always be the same, it's placed on the type itself, if it can differ between calls to a method it's placed on the method).

Further evidence that this is the correct approach is that there's already a place where existing Rust has what in effect is an external implementation, and it works like this already. I realised recently that a lifetime 'a is equivalent to an external impl of impl<T> Deref for &'a T (and likewise for Deref and DerefMut on &'a mut T). (That is, you can view &'a T as a type that implements Deref conditionally on being inside the lifetime 'a, and thus the lifetime itself can be viewed as an external impl of Deref (providing one possible view of what a lifetime actually is, theoretically).) This means that any problem with externally implemented traits should also have an equivalent problem that shows up with lifetimes.

In the case of lifetimes, Rust prevents the equivalent of the hashtable problem arising by requiring any lifetime generics that exist within the types of struct/enum fields to also be lifetime generics of the type itself (thus forcing them to be the same every time). It seems that the fact that the generics exist on the type itself is important for soundness: there is at least one case in current Rust where a type involves types with a lifetime but they aren't generics on the type itself (closures, which can have a non-generic type even if they capture a value whose type has a lifetime), which creates a soundness hole in the type system. This is part of what makes me expect that the same solution would be used for external implementations other than lifetimes.

Doesn't really make a difference, but this isn't true. The standard library itself provides

impl<T> Hash for &T
where
    T: Hash + ?Sized,

So nobody else can provide different impls for K and &K, that would conflict.

Ah right – I was remembering a real problem but it's with Borrow implementations that are more complicated than just &T.

But those all depend on the type being something for which they can define Hash.

It's really important to me that for HashSet<TrustedType>, if someone gives me one it actually works. I don't want to ever let someone give me a garbage HashSet<u32>, for example. I want to be able to trust it because I trust u32 and I trust HashSet.

(This is also why I was opposed to the "raw API": the current plan of having a raw table type is great, but a safe way to violate the invariants on a HashSet<u32> would, IMHO, have blown a huge hole in the whole "Aiming for correctness with types" thing.)

8 Likes