declare_interior_mutable_const for Option

[Byter09]

Hello!

This trait

pub trait SomeTrait {
    const VAL: Option<Self>;
}

triggers

error: a const item should never be interior mutable
  |
6 |     const VAL: Option<Self>;
  |     ^^^^^^^^^^^------------^
  |                |
  |                consider requiring `std::option::Option<Self>` to be `Copy`
  |

which is this lint: https://rust-lang.github.io/rust-clippy/master/index.html#declare_interior_mutable_const

Apparently the corresponding test is this? https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/non_copy_const.rs (not sure!)

Thing is, it doesnt test Option<T> and additionally: is interior mutability for const Options even such a big deal?

I'm simply trying to figure out if I'm implementing a foot-gun right now if I allow this. I don't intent to ever manipulate this const and will only use it for PartialEq and Eq purposes.

EDIT: @frozolotl on Discord suggested removing Option from the trait and it should indeed still trigger. And it does. Note: The suggestion to require Copy is not possible in my case.

[flip1995]

When I try to compile

pub trait SomeTrait {
    const VAL: Option<Self>;
}

I get an compilation error that this isn't Sized and if I write dyn Self + Sized, I get the error, that Self is only available in impls, traits and type definitions. Which both makes sense.

Can you provide a minimal playground example, that produces this lint message?

[flip1995]

Here's my attempt at a SSCCE: Playground

[Byter09]

@flip1995 There you go: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9dfdb8ddad2227c8ca6a48235bb23630 (I also removed the Option as it is not required to trigger this)

What I simply don't understand is why this lint fires a const item should never be interior mutable. It makes no sense.

Adding any trait bounds like Sized is not relevant to this lint triggering for the above given example (Because it's not bad code. It compiles.). This is merely about the fact, that it fires without any discernible reason why.

[flip1995]

I guess it is because it could be interior mutable, when you implement this trait for a struct with a Cell or something.

But I agree, that this is a little bit too conservative. We should special case Self types of trait consts.

[ghost]

I'm not sure about this change. Will warnings continue to be fired if you impl the trait with something unsafe?

Also, can't the trait in the example just be changed to

pub trait SomeTrait {
    fn val() -> Option<Self>;
}

the way the Default trait does it?

[jyn514]

What I simply don't understand is why this lint fires a const item should never be interior mutable. It makes no sense.

I actually ran into a problem this would have warned for the other day. I had a global array of atomic booleans and marked const instead of static, so the changes were never seen (because each change update a different copy of the array).

const KILL_SWITCHES: &[AtomicBool] = [AtomicBool::new(false)];

[rail-rain]

I found a related case at #3962 (comment). In that case, a trait constant, whose type is an associated type, gets linted due to the same reason. However, both of cases have existed in the test since the first PR:

rust-clippy/tests/ui/non_copy_const.rs

Lines 36 to 63 in 656b26e

	trait Trait<T>: Copy {
	type NonCopyType;
	const ATOMIC: AtomicUsize; //~ ERROR interior mutable
	const INTEGER: u64;
	const STRING: String;
	const SELF: Self; // (no error)
	const INPUT: T;
	//~^ ERROR interior mutable
	//~| HELP consider requiring `T` to be `Copy`
	const ASSOC: Self::NonCopyType;
	//~^ ERROR interior mutable
	//~| HELP consider requiring `<Self as Trait<T>>::NonCopyType` to be `Copy`
	const AN_INPUT: T = Self::INPUT;
	//~^ ERROR interior mutable
	//~| ERROR consider requiring `T` to be `Copy`
	declare_const!(ANOTHER_INPUT: T = Self::INPUT); //~ ERROR interior mutable
	}
	trait Trait2 {
	type CopyType: Copy;
	const SELF_2: Self;
	//~^ ERROR interior mutable
	//~| HELP consider requiring `Self` to be `Copy`
	const ASSOC_2: Self::CopyType; // (no error)
	}

And the comments on it imply these behaviours were the author's conscious decision, ...which appears to turn out too restrictive.

Besides Self, I propose stop linting associated types and generic type parameters; and then triggering the lint for associated constants in trait impls whose corresponding definitions in traits are generic in place of it. Originally all trait impls are ignored since "an impl has no power to change the interface"; but, it shouldn't be a problem when definitions in traits are generic (I also think it's impossible to constrain a generic type to be a cell, which doesn't make much sense anyway, as it requires the Freeze trait and negative trait bounds). I have a seemingly working demo and am happy to finish it.