mutable_key_type false positives

[mimoo]

Hello!

We're getting false positives with the mutable_key_type rule: https://circleci.com/gh/libra/libra/41319?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link

It seems to be triggered by this:

HashSet<ProtocolId>

where ProtocolId is defined as

pub type ProtocolId = protocols::wire::messaging::v1::ProtocolId;

which is

#[repr(u8)]
#[derive(Clone, Copy, Debug, Hash, Eq, PartialEq, Deserialize_repr, Serialize_repr)]
pub enum ProtocolId {
    ConsensusRpc = 0,
    ConsensusDirectSend = 1,
    MempoolDirectSend = 2,
    StateSynchronizerDirectSend = 3,
    DiscoveryDirectSend = 4,
    HealthCheckerRpc = 5,
    IdentityDirectSend = 6,
}

I wasn't able to reproduce it with a simple example, so not sure why clippy is getting triggered here.

[llogiq]

This means that the is_mutable_type function returns true for the ProtocolID type, which is counter-intuitive, to say the least. What is the Deserialize_repr and Serialize_repr about? Does it change the lint if those aren't derived?

I do however think it may be because the type is likely in another crate, which may throw a wrench in the Rust compiler's Freeze trait inference (which we use to find out if a type is really immutable). Will try to reproduce in our test suite.

[llogiq]

I have rebuilt what you described within our test suite. And I get no warning. So there must be something else that you didn't show us (for example is there some re-export or pub use of a private modules' items or something going on?).

[mimoo]

we have mutable_key_type to the list of warnings we ignore for now, that's probably why you didn't get it: https://github.com/libra/libra/blob/master/x.toml#L11

[mimoo]

I'm not sure exactly what is missing, I wasn't able to reproduce this with a shorter example as well :/

[llogiq]

In that case, are you OK with closing the issue until we can find a reproducer?

[mimoo]

Up to you. We'll revisit when we have time, but I thought you should know about the issue now :)

[flip1995]

I also tried to reproduce this. I thought that it had something to do with (De)Serialize_repr or repr(u8). But I couldn't build an enum + use it in a function, so this lint would be triggered. That is a really tricky FP...

[carols10cents]

I think I have a reproducer for this-- I'm not 100% positive the key type isn't actually mutable, but it would be unexpected if it is...

• Playground
• Uses http 0.2.8's HeaderName type
• Rust 1.64.0, Clippy 0.1.66 (2022-10-18 a24a020)

Code:

use http::header::HeaderName;
use std::{collections::HashMap, str::FromStr};

fn main() {
    let mut foo = HashMap::new();
    foo.insert(HeaderName::from_str("hi").unwrap(), 3);
}

Clippy output:

warning: mutable key type
 --> src/main.rs:5:5
  |
5 |     let mut foo = 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

EDIT: Ooooh, HeaderName uses Bytes eventually, so I think my case is actually #5812.

[llogiq]

@rustbot claim