Lint at deny-by-default against references to static mut

[traviscross]

We had earlier decided to disallow references to static mut in:

• Disallow *references* to static mut [Edition Idea] #114447

Later, we decided to disallow hidden references to mutable statics also:

• static_mut_refs: Should the lint cover hidden references? #123060

Since then, some interesting cases have come up in:

• Disallow hidden references to mutable static #124895 (comment)

These interesting cases revolve around how we desugar syntax for equality, comparison, compound assignment, and indexing. There are two aspects.

One, though PartialEq::eq, PartialOrd::lt, AddAssign::add_assign, Index::index, and IndexMut::index_mut all take references, for primitive types, as implemented today in lowering to MIR, these operations don't generate references at all. E.g., no references at all are created here:

fn f(x: u8) {
    static mut S: u8 = 1;
    static mut T: [u8; 1] = [0; 1];
    unsafe {
        _ = S == S;
        _ = S < S;
        S += x;
        S += T[0];
        T[0] = x;
    }
}

This behavior is documented for compound assignment expressions, but not otherwise (as far as I can tell).

Two, even if acting on wrapper types where a reference would be generated, the signatures of these methods guarantee that the reference is not leaked and is short-lived. That is, there's no possible sound implementation of AddAssign::add_assign, for any type, that would cause this:

static mut S: W<u8> = W(1);
unsafe { S += W(x); };

...to have any additional UB as compared with:

static mut S: W<u8> = W(1);
unsafe { S = S + W(x); }

...which raises the question of why we would want to treat these differently, and why we would want to encourage people to write the latter rather than the former.

---

In the 2024-08-07 triage meeting, we discussed whether making references to mutable statics a hard error actually met our bar for hard errors. In other cases, we've chosen that only when doing so would make the language definition in a future edition simpler. Otherwise, we've stopped at a deny-by-default lint.

We discussed how, if we made this a lint rather than a hard error, we might have more room to maneuver.

This is what the comment by @scottmcm below is in reply to. However, we did not reach an actual consensus in the meeting.

Also, we did not discuss in the meeting the second aspect described above about how short-lived references are UB-equivalent to accesses.

---

Proposal

On the table in this issue is the following proposal:

1. Upgrade the static_mut_ref lint to deny-by-default in Rust 2024.
   • That would replace and remove the current hard error for references to mutable statics in Rust 2024.
   • Doing this as a lint would give us more room to maneuver.
   • This case does not meet our bar for doing a hard error since it does not make the language definition any simpler in the new edition, and we've stopped at deny-by-default in other cases like this.
2. Extend, in all editions, static_mut_ref to lint against "hidden references" (that is, references created by virtue of autoref).
   • This follows the decision in #123060 except that a deny-by-default lint rather than a hard error would be used in Rust 2024.
   • This extension is justified because a reference that is created by autoref and then leaked is just as dangerous as any other reference to a static mut.
3. As feasible, do not lint in cases (whether explicit or due to autoref) where the compiler can prove that the reference is not created or does not leak.
   • This resolves how to handle the open questions in #124895.
   • An operation that does not create a reference, or a use where we can prove that the reference does not leak, has no additional UB as compared with any other access to the static mut. There's no reason for us to push people away from these patterns and into other patterns that have no more or less potential for UB.
   • We'd start by not linting in the cases where we can tell from the MIR that no reference is created, then later work could do more to avoid linting against other cases where such a proof is feasible. This later work would not be required to ship this lint or the edition.

cc @obeis @RalfJung @rust-lang/lang

Tracking:

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

[scottmcm]

As someone who was previously pushing for some hard errors here, this makes sense to me.¹ So long as static muts exist at all, we have to define the semantics if someone does &mut*addr_of_mut!(MY_STATIC_MUT), so it makes sense to me to pivot to treating this as a "these are known footguns that we'd prefer you do differently" lint -- especially since that leaves us flexibility to fix edge cases later if needed.

Footnotes

1. [Note by TC: This comment predated the full proposal above, and would have only been referring to what are now items 1 and 2.] ↩

[traviscross]

Over here, @RalfJung brought up an interesting point:

One could even make an argument that this shouldn't lint when a custom PartialEq is involved -- the signature of the function, fn eq(&self, other: &Rhs) -> bool, makes it impossible to smuggle out a long-lived reference (the function is generic in the lifetime). References that only exist for the duration of the comparison operation are no more dangerous than direct read/write accesses.

Following that line of argument, it seems in that world we might want to go beyond just the known cases like PartialEq and then not lint, to the degree feasible, on any case where we can prove the reference to be short-lived.

I find myself drawn to that as being probably the consistent and right thing to do.

That could suggest that we start by not linting against these cases where references aren't created at all in the MIR. Then later work could do more to avoid linting against other cases where such a proof is feasible.

What do people think?

[traviscross]

@rustbot labels +I-lang-nominated

Nominating to discuss that matter in case we don't manage to fully work it out asynchronously.

[traviscross]

The issue description has now been updated with a concrete proposal for how to proceed here. We can discuss that next week, and if appropriate, decide on that or on some other proposal via FCP.

That proposal is:

1. Upgrade the static_mut_ref lint to deny-by-default in Rust 2024.
   • That would replace and remove the current hard error for references to mutable statics in Rust 2024.
   • Doing this as a lint would give us more room to maneuver.
   • This case does not meet our bar for doing a hard error since it does not make the language definition any simpler in the new edition, and we've stopped at deny-by-default in other cases like this.
2. Extend, in all editions, static_mut_ref to lint against "hidden references" (that is, references created by virtue of autoref).
   • This follows the decision in #123060 except that a deny-by-default lint rather than a hard error would be used in Rust 2024.
   • This extension is justified because a reference that is created by autoref and then leaked is just as dangerous as any other reference to a static mut.
3. As feasible, do not lint in cases (whether explicit or due to autoref) where the compiler can prove that the reference is not created or does not leak.
   • This resolves how to handle the open questions in #124895.
   • An operation that does not create a reference, or a use where we can prove that the reference does not leak, has no additional UB as compared with any other access to the static mut. There's no reason for us to push people away from these patterns and into other patterns that have no more or less potential for UB.
   • We'd start by not linting in the cases where we can tell from the MIR that no reference is created, then later work could do more to avoid linting against other cases where such a proof is feasible. This later work would not be required to ship this lint or the edition.

---

We had ended the 2024-08-07 meeting with an apparent meeting consensus, but later discussion has indicated that there was not in fact a meeting of the minds among all members, so please disregard any earlier discussion to that effect. Lang is still undecided on the proper handling of #124895.

[traviscross]

@rfcbot fcp merge

We discussed this in the meeting today; we had consensus on the plan above. We're going to FCP this, but please take our meeting consensus as enough to proceed with implementation here.

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[scottmcm]

I still think as I did above (#128794 (comment)), so good by me. We're essentially approving the "intent to lint on references to static mut as footguns", and I don't think we need to be too strict about exactly all the details -- a huge advantage of it being a lint is that we can always tweak it around the edges later.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[traviscross]

This change was made in:

• Disallow hidden references to mutable static #124895