Add lint against (some) interior mutable consts

[Urgau]

interior_mutable_consts

warn-by-default

The interior_mutable_consts lint detects instance where const-items have a interior mutable type, which silently does nothing.

Example

use std::sync::Once;

// SAFETY: should only be call once
unsafe extern "C" fn ffi_init() { /* ... */ }

const A: Once = Once::new(); // using `A` will always creates temporaries and
                             // never modify it-self on use, should be a
                             // static-item instead

fn init() {
    A.call_once(|| unsafe {
        ffi_init();  // unsound, as the `call_once` is on a temporary
                     // and not on a shared variable
    })
}

warning: interior mutability in `const` item have no effect to the `const` item it-self
  --> $DIR/interior-mutable-types-in-consts.rs:10:1
   |
LL | const A: Once = Once::new();
   | ^^^^^^^^^----^^^^^^^^^^^^^^^
   |          |
   |          `Once` is an interior mutable type
   |
   = note: each usage of a `const` item creates a new temporary
   = note: only the temporaries and never the original `const` item `A` will be modified
   = help: for use as an initializer, consider using an inline-const `const { /* ... */ }` at the usage site instead
   = note: `#[warn(interior_mutable_consts)]` on by default
help: for a shared instance of `A`, consider using a `static` item instead
   |
LL | static A: Once = Once::new();
   | ~~~~~~
help: alternatively consider allowing the lint
   |
LL | #[allow(interior_mutable_consts)] const A: Once = Once::new();
   | +++++++++++++++++++++++++++++++++

Explanation

Using a const-item with an interior mutable type has no effect as const-item are essentially inlined wherever they are used, meaning that they are copied directly into the relevant context when used rendering modification through interior mutability ineffective across usage of that const-item.

The current implementation of this lint only warns on significant std and core interior mutable types, like Once, AtomicI32, ... this is done out of prudence and may be extended in the future.

---

It should be noted that the lint in not immunized against false-positives in particular when those immutable const-items are used as "init" const for const arrays (i.e. [INIT; 10]), which is why the lint recommends inline-const instead.

It should also be noted that this is NOT an uplift of the more general clippy::declare_interior_mutable_const lint, which works for all interior mutable types, but has even more false-positives than the currently proposed lint.

A simple Github Search reveals many instance where the user probably wanted to use a static-item instead.

---

@rustbot labels +I-lang-nominated +T-lang -S-waiting-on-review
cc @AngelicosPhosphoros @kupiakos
r? compiler

Fixes IRLO - Forbidding creation of constant mutexes, etc
Fixes #132028
Fixes #40543

[taiki-e]

This lint seems to be the same as (or limited version of) clippy::declare_interior_mutable_const?

AFAIK clippy::declare_interior_mutable_const is usually useless (It is not a lint that is triggered for code that may actually encounter problems, e.g., it is not triggered when a constant defined in the external crate is used incorrectly. clippy::borrow_interior_mutable_const covers actual footguns about interior_mutable_const and triggered for code that may actually encounter problems. see also rust-lang/rust-clippy#7665)

[Urgau]

This lint seems to be the same as (or limited version of) clippy::declare_interior_mutable_const?

Forgot to mentioned it (fixed), but yes it's a very-limited form of the clippy lint.

AFAIK clippy::declare_interior_mutable_const is usually useless (It is not a lint that is triggered for code that may actually encounter problems, e.g., it is not triggered when a constant defined in the external crate is used incorrectly.

I disagree that it is useless. It detects suspicious constants item, suspicious enough that they are probably a mistake and IMO warrants a warning.

In other words I think this lint detects potential issues at the root cause and not at the usage site. I would find it weird if we warned when using a const-item from an external crate, where the user can't do anything about it (except maybe creating an issue upstream) but I also think I could be convinced otherwise.

clippy::borrow_interior_mutable_const covers actual footguns about interior_mutable_const and triggered for code that may actually encounter problems.

I did not actually see this lint before, it's interesting and tackles the issue at another level; but IMO declaring a interior mutable const is a foot-gun whenever used or not, in particular if exposed and used from an external crate.

see also rust-lang/rust-clippy#7665)

This should use inline const, which is suggested by this lint.

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[taiki-e]

@Urgau

In other words I think this lint detects potential issues at the root cause and not at the usage site. I would find it weird if we warned when using a const-item from an external crate, where the user can't do anything about it (except maybe creating an issue upstream).

Ah, we each seem to have a different case in mind. I was thinking of a case where const is correct and the usage referring to it is incorrect, and you seem to be thinking of a case where defining it as const is wrong and the usage referring to it is supposedly correct. I agree that this lint is indeed not necessarily useless in such a case.

This should use inline const, which is suggested by this lint.

It seems unfortunate that lint, for which the only two ways to deal with warnings are to ignore the warning or increase the MSRV, will be added as warn-by-default.

---

After reading your comment, I think it would be better to apply this lint only to public constants, and uplift clippy::borrow_interior_mutable_const to rustc to address other cases:

• In cases where const is not correct (i.e., static is correct):
  • if const is private:
    • borrow_interior_mutable_const warns of a reference to const and the user can correct it on the fly.
    • interior_mutable_consts(declare_interior_mutable_const) doesn't need to do anything since borrow_interior_mutable_const can catch all problems.
  • if const is public:
    • borrow_interior_mutable_const warns of a reference to const. If it is defined in the external crate, the user cannot fix the underlying problem, but at least the problem can be avoided.
    • interior_mutable_consts(declare_interior_mutable_const) warns of a suspect definition of const in the crate that defines that const.
• In cases where const is correct:
  • if const is private:
    • borrow_interior_mutable_const warns misuses of it.
    • interior_mutable_consts(declare_interior_mutable_const) doesn't need to do anything since borrow_interior_mutable_const can catch all misuses.
  • if const is public:
    • borrow_interior_mutable_const warns misuses of it.
    • interior_mutable_consts(declare_interior_mutable_const) warns of (correct) definition of const (false positive!).
      • However, in cases where const is correct, such as array initialization helpers, const is basically private, so the chances of encountering this false positive should be quite rare...

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[Urgau]

[..] you seem to be thinking of a case where defining it as const is wrong and the usage referring to it is supposedly correct.

Yes, there seems to be some of cases in the wildly were it's the case. (both of the linked issues are example of that)

Example of a soundness issue caused by using a const-item instead of a static-item

use std::sync::Once;

// SAFETY: should only be call once
unsafe extern "C" fn ffi_init() { /* ... */ }

const INIT_ONCE: Once = Once::new(); // using `B` will always creates temporaries and
                                     // never modify it-self on use, should be a
                                     // static-item instead

fn init() {
    INIT_ONCE.call_once(|| unsafe {
        ffi_init();  // unsound, as the `call_once` is on a temporary
                     // and not on a shared variable
    })
}

Updated the lint example with this one, to better describe the usefulness of the lint

After reading your comment, I think it would be better to apply this lint only to public constants, and uplift clippy::borrow_interior_mutable_const to rustc to address other cases

Interesting idea.

I'm not sure I like having the lint be dependant of the visibility of the const-item since it has nothing to do with the issue it-self, see my example above or #132028, where the issue is not the usage but the declaration, with your proposal we IMO would be linting at the wrong place. (We could lint at the declaration only if we found a usage, but that seems hacky and I rather avoid that)

[flip1995]

How do those Clippy lints interact with the new rustc lint?

[Urgau]

This proposed lint is similar to clippy::declare_interior_mutable_const, except that contrary to the Clippy lint which recurse into the type and can lint on arbitrary type that have interior mutability, this lint only checks if the type is marked with #[rustc_significant_interior_mutable_type] (which only concerns a few core/std types).

So for these types (and only these types) we would report a similar warning. (Preventing this duplicated warning should be a matter of checking if the type as the attribute).

[flip1995]

I would prefer adding this check to the Clippy lints, over just allowing the rustc lint in the tests.

The tests could then be adapted to remove all test cases that get linted by rustc, except for maybe 1 to check that the Clippy lint doesn't actually trigger on those core/std types.

[bors]

☔ The latest upstream changes (presumably #135726) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

[traviscross]

Suggested change

	/// // never modify it-self on use, should be a
	/// // never modify itself on use, should be a

[traviscross]

Suggested change

	lint_interior_mutable_consts = interior mutability in `const` item have no effect to the `const` item itself
	lint_interior_mutable_consts = interior mutability in `const` items have no effect on the `const` item itself

[traviscross]

I'm trying to decide whether I like this or not. What I like about it is where it might help catch errors, of course. What I don't like about it is that probably I actually think it's fine to use a const item as a kind of "const alias" (or like a thunk const function), and so I'm hesitant to discourage that as much as this lint probably would.

That is, it seems maybe fine to use a const item to set up some interior mutable type how you want it, and then to use that in initializers, e.g. [INIT; 128], rather than having to e.g. wrap the logic in a const fn and then call that in a const block in the initializers.

Regarding this bit:

The current implementation of this lint only warns on significant std and core interior mutable types, like Once, AtomicI32, ... this is done out of prudence and may be extended in the future.

This could be troublesome for us. Each time we'd want to extend this, it could be a significant lint expansion in terms of the amount of code affected, but the fix is not machine-applicable.

[jamesmunns]

In terms of "experience report", an almost identical repro of #132028 occurred on reddit today.

[Urgau]

That is, it seems maybe fine to use a const item to set up some interior mutable type how you want it, and then to use that in initializers, e.g. [INIT; 128], rather than having to e.g. wrap the logic in a const fn and then call that in a const block in the initializers.

In the case of array initializer we should maybe recommend inline const [const { ... }; 128] instead of the const INIT pattern.

This could be troublesome for us. Each time we'd want to extend this, it could be a significant lint expansion in terms of the amount of code affected, but the fix is not machine-applicable.

If we only recommend the const to static suggestion we could make it a machine-applicable suggestion. The only reason it's not is because the lint currently produces a second suggestion which makes the two suggestions exclusive.

[traviscross]

In the case of array initializer we should maybe recommend inline const [const { ... }; 128] instead of the const INIT pattern.

The point is that we could have:

const INIT: .. = {
    ..
    ..
    ..
    ..
};

let (x, y, z) = ([INIT; 128], [INIT; 128], [INIT; 128]);

Clearly we wouldn't just want to inline the body of the const initializer into each array initializer in const { .. } blocks. So we would need to refactor as, e.g.:

const fn make_init() -> .. {
    ..
    ..
    ..
    ..
}

let (x, y, z) = ([const { make_init() }; 128], [const { make_init() }; 128], [const { make_init() }; 128]);

That's what I'm not really sure it'd be great for us to impose.

Then, going a step further, it seems like anywhere we have const { make_init() } that we should be able to instead do:

const INIT: .. = make_init();
let (x, y, z) = ([INIT; 128], [INIT; 128], [INIT; 128]);

And I could see it being surprising if we warned against this kind of natural refactoring in this one case.

[Urgau]

Hum, I see, thanks for the bigger example.

We could maybe have exception for local (not pub) interior mutable const used only as initializer (only array initializer?) and nothing else.