Clippy interprets Bytes type incorrectly as interior mutable which triggers errors on several lints

[Matthias247]

I tried this code:

use bytes::Bytes;

const MY_BYTES: Bytes = Bytes::from_static(b"asdf");

Playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=dec0c4a3d84023fdc3e8b27c6ed90c88

I expected to see this happen: Everything compiles fine and clippy checks succeed

Instead, this happened: Clippy emits a bunch of errors which abort compilation. For this particular piece of code it reports

error: a `const` item should never be interior mutable
 --> src/lib.rs:3:1
  |
3 | const MY_BYTES: Bytes = Bytes::from_static(b"asdf");
  | -----^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  | |
  | make this a static item (maybe with lazy_static)
  |
  = note: `#[deny(clippy::declare_interior_mutable_const)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#declare_interior_mutable_const

However in other contexts - e.g. when I use Bytes as a key type in a map - I also get the mutable_key_type and borrow_interior_mutable_const errors.

I guess clippy doesn't understand the layout of Bytes, which uses interior raw pointers and a vtable: https://github.com/tokio-rs/bytes/blob/6fdb7391ce83dc71ccaeda6a54211ce723e5d9a5/src/bytes.rs#L71-L77

While it is true that some data could be modified even from an immutable reference - e.g. due to a refcount change - the changes have no impact on the actual payload of the Bytes type - it always is an immutable byte array. Therefore I think the lints are wrongly applied here.

I'm however not sure if this is something that would need to be fixed in clippy or could be fixed in Bytes

Meta

• cargo clippy -V: e.g. 0.0.212 (2020-07-15 7e11379)
• rustc -Vv:

  rustc 1.44.1 stable
  

[flip1995]

The problem here is, that Clippy detects the AtomicPtr field in the Bytes struct, which has interior mutability. I'm not sure, why the Bytes struct has this field. I guess because of syncing if Bytes is used in a shared context?

From the lint documentation:

every time you refer to the const a fresh instance of the Cell or Mutex or AtomicXxxx will be created, which defeats the whole purpose of using these types in the first place.

I think this applies here. But I'm not sure, since I'm not familiar with the Tokio code base and what the intention behind this struct is.

[rail-rain]

Bytes is very strange. It’s like a sliceable version of Arc<str>, but it can also point directly to the static slice without any allocation nor copying. It looks like, when constructed from a static slice, no code reach the AtomicPtr; and the Hash impl never touch the atomic too (what’s more, this AtomicPtr is not the refcount. I’m not certain the purpose of it). Thus, it’s safe to use as a key of a hash map; and a constant of Bytes does make some sense, in my opinion.

For mutable_key_type, there’s a couple of examples where Bytes are keys of hash sets in the wild: here (Key is a type alias of Bytes) and there (RsyncModule is a struct of two Bytes). I consider it’s a false positive of the lint, but not sure if it’s fixable. However, checking Hash implementations as this issue describes (#732), instead of keys of sets would be more resistant to this FP. I think implementing #732 actually and downgrading mutable_key_type in favour of it would be one option.

Regarding declare/borrow_interior_mutable_const, having said “make some sense”, it would be rare. If an interface is generic enough, one can just pass a byte slice; e.g. HashSet::<Bytes>::new().contains(b"foo".as_ref()) compiles (playground). actix-web have a bunch of suppression of these lints, but it’s due to #3825. Downgrading these and documenting this issue might be enough to close this (#6012).

[coolreader18]

I was thinking about implementing a #[clippy::allow_as_interior_mutable_const], similar to #[clippy::has_significant_drop], to allow for marking a type as OK to use as a const, even though it's got interior mutability. Would that be a reasonable way to resolve this kind of issue?

[theli-ua]

Wouldn't it make more sense to simply make this lint use ignore-interior-mutability configuration option?

[rail-rain]

It looks like the lints already do since #11678. This issue is effectively complete.