mutable_key_type false positive for raw pointers

[YaLTeR]

Lint name: mutable_key_type

I tried this code:

use std::collections::HashSet;

fn main() {
   let s: HashSet<*mut ()> = HashSet::new(); 
}

I expected to see this happen: lint not triggered because raw pointers are compared and hashed by pointer value rather than by pointed-to value

Instead, this happened: lint triggered

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=601da4e6cf6eb0b2c178c32fcab35390

Meta

• cargo clippy -V: clippy 0.1.52 (d1206f9 2021-02-15)
• rustc -Vv:

  rustc 1.52.0-nightly (d1206f950 2021-02-15)
  binary: rustc
  commit-hash: d1206f950ffb76c76e1b74a19ae33c2b7d949454
  commit-date: 2021-02-15
  host: x86_64-unknown-linux-gnu
  release: 1.52.0-nightly
  LLVM version: 11.0.1
  

[kneasle]

I'd be happy to have a go at fixing this - it'd be my first contribution, so if someone could mentor/point me in the right directions then that would be great 😁.

[Enselic]

@kneasle 👍 Looks to me like these lines are wrong, and your task would be to figure out what the code should look like instead: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/mut_key.rs#L118-L120

[xFrednet]

Welcome to the project @kneasle,

@Enselic has already linked the correct file and most likely location of the fix (Thank you for this). For bug fixes like these, it's usual helpful to check why they happen (In this case due to the use of a mut pointer) and to test some examples to determine what we want to happen. (I've also tested that the Hash trait is only called for the pointer and not the value in this playground just to make sure)

In this case, we want to allow *mut but not things like mut* etc. A possible solution would be to pass the information somehow along, or check if the recursive call is even needed in the referenced file location.

If you like to work on this, please claim it with @rustbot claim. You can also always unassign yourself later. If you have any further questions, feel free to ask 🙃

[Enselic]

(I've also tested that the Hash trait is only called for the pointer and not the value in this playground just to make sure)

Both of you might also find it worthwhile to take a look at the implementation of Hash for raw pointers (*mut T, *const T), which also helps to convince oneself of that only the pointer is involved in hashing, and not what is pointed to: https://github.com/rust-lang/rust/blob/1.54.0/library/core/src/hash/mod.rs#L717-L736

[xFrednet]

You're right, that's an excellent hint! 👍

[kneasle]

Welcome to the project @kneasle,

Thanks!

(In this case due to the use of a mut pointer)

Also btw this false positive still happens for *const pointers (which happened in the code I was writing).

If you like to work on this, please claim it with @rustbot claim. You can also always unassign yourself later. If you have any further questions, feel free to ask upside_down_face

Righto does this need to be a separate message or can I just do @rustbot claim in a message (I guess not but let's test it anyway...)

[kneasle]

@rustbot claim

[xFrednet]

Also btw this false positive still happens for *const pointers (which happened in the code I was writing).

Good to know, then you can try fixing it for both cases 🙃

Righto does this need to be a separate message or can I just do @rustbot claim in a message

It doesn't have to be in its own comment, but the bot seems to be down right now 😅. In that case, I'll assign you

[kneasle]

In that case, I'll assign you

Thanks 😄

[Enselic]

Also btw this false positive still happens for *const pointers (which happened in the code I was writing).

Just to save you from some confusion later, I would like to point out that this is not quite accurate. The false positive will only trigger for *const T if T has interior mutability. Demonstration:

#![allow(dead_code)]
#![allow(unused_imports)]
#![allow(unused_variables)]

use std::cell::Cell;
use std::collections::HashSet;

struct SomeStruct {
    regular: u64,
    // Uncomment the next line to "fix" the clippy::mutable_key_type false positive
    interior_mutability: Cell<u64>,
}

fn main() {
    let map: HashSet<*const SomeStruct> = HashSet::new();
}

[kneasle]

... The false positive will only trigger for *const T if T has interior mutability.

Oooh! So does that mean that the pattern which matches raw pointers is just wrong? And by extension, removing it will fix the bug? I'll probably replace it with a cheeky comment linking to this issue and the impl of Hash. If that's the case, then it'll probably take me more time to get clippy to compile than it will to actually fix the bug 😆.

[xFrednet]

Oooh! So does that mean that the pattern which matches raw pointers is just wrong?

Not quite, it can make sense if you actually want to compare them. The question is really what you want to have hashed and also why. 🙃

This lint however shouldn't care about it and only check if we have a type that is mutable in a way that could change the hash. For *mut T and *const T this is not the case as both implementations take the address and ignore the value of T. This is also what this false positive is about. However, if we would have a type than those could matter if the type has its own hashing implementation. Basically

//             vvvv Okay as only this get's hashed
let s: HashSet<*mut ()> = HashSet::new(); 

struct Penguin {
    inner: *mut Inner
}
//             Not okay as the implementation of hash for Penguin
//             could depend on inner which can be mutated
//             vvvvvvv 
let s: HashSet<Penguin> = HashSet::new(); 

[kneasle]

Not quite, it can make sense if you actually want to compare them. The question is really what you want to have hashed and also why. 🙃

Ooooh - good point! So only the top-level Hash impl needs to change... I'll make some cheeky test cases once I start working on this (not quite sure when it'll be, but hopefully soon 😃).