Disallow hidden references to mutable static

[obeis]

Closes #123060

Tracking:

• Tracking Issue for Rust 2024: Deny references to static mut #123758

[RalfJung]

Sorry, I can only review interpreter and Miri PRs.

r? compiler

[obeis]

r? @davidtwco

[davidtwco]

Marking as waiting on author since you've made this a draft.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[obeis]

@rustbot ready

[michaelwoerister]

That error can be solved by pub re-exporting the Edition type here:

// `pub use` because the re-export is referred to by the declare_lint! macro
pub use rustc_span::edition::Edition;

Otherwise the code instantiated when declare_lint! is called from other crates won't have access to the rustc_lint_defs::Edition re-export.

[michaelwoerister]

Thanks, @traviscross!

What would be an acceptable heuristic for determining from the method signature that the self reference might leak? If all other parameter types and the return type don't contain references -- or is that too simplistic?

[traviscross]

What would be an acceptable heuristic for determining from the method signature that the self reference [doesn't] leak? If all other parameter types and the return type don't contain references -- or is that too simplistic?

Sounds like a start. For PartialEq::eq, though, both arguments are references.

cc @compiler-errors

[bors]

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

[traviscross]

In terms of the heuristic, I'm trying to think up the maximally-pessimistic example of how the reference could leak. Perhaps it's something like this:

use core::cell::Cell;

struct W<'w>(Cell<Option<&'w W<'w>>>);
impl<'w> W<'w> {
    fn leak<'a: 'w>(&'a self) {
        self.0.set(Some(self));
    }
}


fn main() {
    static mut X: W = W(Cell::new(None));
    unsafe { X.leak() };
    let leaked = unsafe { X.0.get().unwrap() };
    _ = [leaked];
}

Playground link

[traviscross]

The changes I asked for were addressed.

[compiler-errors]

Please change all of the FIXME(obeis) to not reference a specific username, since this is probably grouped under something better like FIXME(static_mut_refs).

Also, are all of the test changes still necessary? Do we really need to change STATIC += 1 to STATIC = STATIC + 1? If we're motivating users to make somewhat churn-y noop changes to their code, I really don't think this lint is complete yet. If not, then those should be reverted.

Same with x.is_some() to let Some(_) = X, and most of the other changes to tests.

Existing tests should not be changing static mut to static or to use atomics. In general, tests should be left alone, and have allow(static_mut_refs) added to the top. There's no reason to add FIXMEs to those -- some of these tests are explicitly using static muts to exercise their behavior in the compiler.

[compiler-errors]

I was perhaps a bit too knee-jerky on the test changes, but they're really hard to separate out the changes from an initial glance.

I would prefe us to separately in a follow-up PR (or maybe even a PR that lands before this one) that we could tweak some of the test to use Atomics over static mut if they're run-pass tests. In the mean time, would prefer if we tag them FIXME(static_mut_refs): this could use an atomic rather than also trying to change them at the same time as implementing this lint.

[compiler-errors]

There are also some random stray files that need to be deleted:

• compiler/rustc_lint_defs/src/lib.rs:11:5
• compiler/rustc_lint_defs/src/lib.rs:887:48

[compiler-errors]

You should use cx.typeck_results here instead of calling the query: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/struct.LateContext.html#method.typeck_results

[compiler-errors]

This can be simplified into something like let Some(receiver) = inputs.get(0) && let ty::Ref(_, _, m) = receiver.kind().

[compiler-errors]

There's never a case where this isn't Some on the caller side, so this could be simplified to just Mutability.

[compiler-errors]

I'd personally rather we try to make this lint a bit less trigger-prone if possible. I'd be fine even if we implement it only for totally unconstrained late-bound receiver lifetimes where the lifetime does not show up in the return type or something like that.

But that's my personal opinion; I guess if T-lang is fine with this amount of churn then whatever.

[michaelwoerister]

r? @compiler-errors -- I'm not a good reviewer for this other than the basics.

[bors]

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

[obeis]

@compiler-errors Done.