Produce a warning when using `const` with interior mutability

[matklad]

Originally reported in https://users.rust-lang.org/t/broken-atomics-puzzle/9533

Consider this code

use std::sync::atomic::{ATOMIC_BOOL_INIT, AtomicBool, Ordering};

pub const A: AtomicBool = ATOMIC_BOOL_INIT;

fn main() {
    A.compare_and_swap(false, true, Ordering::SeqCst);
    println!("A = {}", A.load(Ordering::SeqCst));
}

Playground

It compiles and runs cleanly, but produces unexpected results because const is used instead of static.

It would be nice to somehow give a warning for .compare_and_swap call, but I am not sure it is possible.

[petrochenkov]

cc rust-lang/rfcs#885 rust-lang/rust-clippy#829

[matklad]

Note that compare_and_swap uses &self:

compare_and_swap(&self, current: bool, new: bool, order: Ordering) -> bool

[petrochenkov]

The underlying issue is the same - A is a rvalue and copied on use and this becomes observable when any mutability is involved, interior or exterior.

[steveklabnik]

New warnings generally require an RFC, but I'll let @rust-lang/compiler make the call

[nikomatsakis]

I'm not sure -- precisely what conditions would trigger the warning? I agree there is a subtle footgun at play here, if you don't understand the rules.

[matklad]

I'm not sure -- precisely what conditions would trigger the warning?

I think "calling a method exploiting interior mutability on const" is the right condition, but I don't think we can really check it because we don't know which methods involve interior mutability.

[nikomatsakis]

Seems like it would have to "calling any method on const if the type of that const involves UnsafeCell", roughly speaking. Right?

[matklad]

Seems like it would have to "calling any method on const if the type of that const involves UnsafeCell", roughly speaking. Right?

Yep. Perhaps this won't add false positives in practice?

I wonder if we need to warn about creating constants with interior mutability. This is the root of the problem. Currently we need such constants for ATOMIC_X_INIT, but it is only because there are no constant functions in Rust yet.

[Diggsey]

I'm not sure that's quite right:

• Methods which consume self are probably OK even on values with an UnsafeCell, since any side-effects wouldn't be visible anyway.
• Passing borrows of constants to functions also suffers the same issue: http://play.integer32.com/?gist=427fafb8cf47fd5b629d1fc6a91bf162&version=stable

Seems to me like this behaviour occurs whenever an internally mutable rvalue is borrowed. However, it's only surprising when that rvalue comes from a constant, because constants don't look like rvalues, and because they can be used multiple times, whereas most rvalues can only be used once.

Putting that together, it looks like the compiler should issue a warning whenever a constant containing an UnsafeCell is borrowed.

[burdges]

Another autotrait would be required for this, no? All atomics have this problem, but maybe not all usages of UnsafeCell? Also variance matters, like a &AtomicBool or &Arc<T> sounds okay.

Also, there are usages for const fn with smart pointer, like Mutex/Arc::new() should become const fn so that statics containing Arcs can be initialized at compile time, meaning you'll transiently have a const with an Atomic and/or UnsafeCell during compilation.

[pedrohjordao]

Just got hit by this unexpected behavior as well and thought I was going crazy. Reading the explanations made everything make sense, but some warning would be ideal.

[dhardy]

@burdges has a point. Items like the following should be illegal, not merely illegal to try mutating.

const X: Cell<u32> = Cell::new(0);

[eddyb]

@dhardy Why so? That's not a static, there's no Cell<u32> place in memory, it's just a constant value, a "template" for creating the same value.

[dhardy]

But are there any legitimate uses for this? Possibly as part of a compound type I guess, but that would be unusual. A lint may be better then.

Okay, the problematic bit is that one can call X.set(5); even though consts do not permit mutation.

[eddyb]

@dhardy Yeah. IMO, this is in the same category as this:

struct Foo<T>(T);
const X: Foo<i32> = Foo(0);
fn main() {
    X.0 += 1;
}

Clippy should be linting both this and your example, saying that you're mutating a (field of a) temporary and you probably wanted to do something else.
Although it's maybe harder for Cell::set (assuming we'd rather not hardcode it in clippy).

[dhardy]

Why rely on Clippy here? As I understand it, Clippy is primarily about style and succinctness, not correctness. If someone is assigning to a temporary field that is very likely incorrect code.