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]

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]

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.

[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.

I don't think this actually ever got fixed, but whatever.

@bors r+

[bors]

📌 Commit 3b0ce1b has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit 3b0ce1b with merge 5ba6db1...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 5ba6db1 to master...

[rust-timer]

Finished benchmarking commit (5ba6db1): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary -1.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 7.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	7.1%	[7.1%, 7.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 767.803s -> 768.593s (0.10%)
Artifact size: 341.28 MiB -> 341.36 MiB (0.02%)