`mutable_key_type` doesn't account for custom `Hash` impl

[kchibisov]

Summary

With the custom Hash implementation for the struct the said lint often fails because it sees the mutable field inside the struct. However the way how this field is used could be very different and such field could be not used at all.

Lint Name

mutable_key_type

Reproducer

I tried this code:

use std::sync::atomic::AtomicBool;
use std::sync::Arc;
use std::collections::HashMap;

pub struct ObjectId {
    id: u32,
    alive: Option<Arc<AtomicBool>>,
}

impl std::hash::Hash for ObjectId {
    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
        self.id.hash(state);
        self.alive
            .as_ref()
            .map(|arc| &**arc as *const AtomicBool)
            .unwrap_or(std::ptr::null())
            .hash(state);
    }
}

fn main() {
    let _set : HashMap<ObjectId, i32> = HashMap::new();
}

I saw this happen:

warning: mutable key type
  --> src/main.rs:24:5
   |
24 |     let _set : HashMap<ObjectId, i32> = HashMap::new();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mutable_key_type
   = note: `#[warn(clippy::mutable_key_type)]` on by default

I expected to see this happen:

It shouldn't trigger because the unmutable part of the key is used, as in the pointer to the Arc which can't change.

Also, if you completely remove the self.alive part of the Hash implementation it'll still warn, even though the object won't be a part of the hash meaning that it's mutability can not affect anything.

Version

clippy 0.1.81 (d9284af 2024-07-14)
rustc 1.81.0-nightly (d9284afea 2024-07-14)
cargo 1.81.0-nightly (154fdac39 2024-07-07)

Additional Labels

No response

[lukaslueg]

In the general case, there is no way to determine if a impl Hash is stable. Consider fn hash(&self, state: &mut Hasher) { if const { riemann_hypothesis() } { self.value.hash(state) } }. Taking into account "accidental mutability", imho this is non-goal for this lint.

[kchibisov]

Yeah, it's really hard to figure this stuff out if you have a hash, though, general AST matching can detect if you use value the at all inside the hash implementation.

Maybe for the rest it's fine to force the user to annotate or just not throw if you have custom hash impl...

[cbiffle]

By "force the user to annotate," is there a way to annotate the type as being safe for use as a hash key (because e.g. the interior mutable field is explicitly excluded for eq/hash), or are you suggesting that every user of the type for the rest of time needs to add a Clippy annotation to suppress this false positive?

[kchibisov]

I mean to suppress the warning on this particular struct, etc, so to suppress the false positive. The implementation is edgy, so probably it's not a bad idea, it's just the issue is that you can only see the issue once you try to hash it from what I've seen.