const validation: remove check for mutable refs in final value of const

[RalfJung]

Reference PR:

• get rid of const.no-mut-refs reference#2080

This check rejects code that is not necessarily UB, e.g. a mutable ref to a static mut that is very carefully used correctly. That led to us having to describe it in the Reference, which uncovered just how ad-hoc this check is (rust-lang/reference#2074).

Even without this check, we still reject things like

const C: &mut i32 = &mut 0;

This is rejected by const checking -- the part of the frontend that looks at the source code and says whether it is allowed in const context. In the Reference, this restriction is explained here.

So, the check during validation is just a safety net. And it is already a safety net with gaping holes since we only check &mut T, not &UnsafeCell<T>, due to the fact that we promote some immutable values that have !Freeze type so &!Freeze actually can occur in the final value of a const.

So... it may be time for me to acknowledge that the "mutable ref in final value of const" check is a cure that's worth than the disease. Nobody asked for that check, I just added it because I was worried about soundness issues when we allow mutable references in constants. Originally it was much stricter, but I had to slowly relax it to its current form to prevent t from firing on code we intend to allow. In the end there are only 3 tests left that trigger this error, and they are all just constants containing references to mutable statics -- not the safest code in the world, but also not so bad that we have to spend a lot of time devising a core language limitation and associated Reference wording to prevent it from ever happening.

So... @rust-lang/wg-const-eval @rust-lang/lang I propose that we allow code like this

static mut S: i32 = 3;
const C2: &'static mut i32 = unsafe { &mut * &raw mut S };

@theemathas would be great if you could try to poke a hole into this. ;)

[rustbot]

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[theemathas]

In the end there are only 3 tests left that trigger this error, and they are all just constants containing references to mutable statics

All code that this PR newly allows involves &mut pointing to statics, right?

[theemathas]

With this PR, Miri detects no UB in this code. Is this intended?

use std::ptr;
static mut X: i32 = 1;
const Y: &mut i32 = unsafe { &mut *&raw mut X };
fn main() {
    let a = &Y;
    let b = &Y;
    println!("{a}");
    println!("{b}");
    unsafe {
        *ptr::from_ref(a).read() = 2;
        *ptr::from_ref(b).read() = 3;
        *ptr::from_ref(a).read() = 4;
        *ptr::from_ref(b).read() = 5;
    }
}

[theemathas]

However, Miri detects UB in the following code:

use std::ptr;
static mut X: i32 = 1;
const Y: &mut i32 = unsafe { &mut *&raw mut X };
fn main() {
    let a = &std::convert::identity(Y);
    let b = &Y;
    unsafe {
        *ptr::from_ref(b).read() = 3;
        *ptr::from_ref(a).read() = 4;
    }
}

error: Undefined Behavior: trying to retag from <287> for Unique permission at alloc1[0x0], but that tag does not exist in the borrow stack for this location       
  --> src\main.rs:10:10
   |
10 |         *ptr::from_ref(a).read() = 4;
   |          ^^^^^^^^^^^^^^^^^^^^^^^ this error occurs as part of retag at alloc1[0x0..0x4]
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental       
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <287> was created by a Unique retag at offsets [0x0..0x4]
  --> src\main.rs:5:14
   |
 5 |     let a = &std::convert::identity(Y);
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^
help: <287> was later invalidated at offsets [0x0..0x4] by a Unique retag
  --> src\main.rs:9:10
   |
 9 |         *ptr::from_ref(b).read() = 3;
   |          ^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

[RalfJung]

All code that this PR newly allows involves &mut pointing to statics, right?

That should be the case, yes. The only other thing an &mut could point to is a temporary, and then either that gets lifetime extended and we should reject it during const checking, or it does not get lifetime extended and then this becomes a dangling reference that we detect.

With this PR, Miri detects no UB in this code. Is this intended?

We can't run the aliasing model when evaluating constants, so all pointers coming from constants are "equivalent" to Miri. These &Y get promoted so the values of a and b come from constants and Miri can't tell them apart.

The 2nd example passes Y around by-value, which leads to it being retagged, and then Miri's aliasing checks kick in.

[theemathas]

all pointers coming from constants are "equivalent" to Miri

Even if those are two aliasing &mut references being stored at different addresses?

[RalfJung]

If the address they are stored at is "global" memory (i.e. the memory backing a const or static), then yes. All pointers stored in global memory are considered to be "root" pointers for the aliasing model in Miri.

[theemathas]

I couldn't find any other unusual behavior with this PR.

(I tried to use a const containing a &mut in a pattern, but for &mut pointing to 1 or more bytes of memory, rust won't let me use that pattern, due to the const referencing mutable memory.)

[traviscross]

Sounds right to me. Always good when we can delete hairy text from the Reference.

@rfcbot fcp merge

[traviscross]

@rfcbot fcp merge

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @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.

[davidtwco]

Implementation LGTM, r=me once t-lang FCP finishes

View changes since this review

[nikomatsakis]

@rfcbot reviewed

[rust-rfcbot]

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

[tmandry]

@rfcbot reviewed

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

[RalfJung]

@bors r=davidtwco

[bors]

📌 Commit 3d7c9bd has been approved by davidtwco

It is now in the queue for this repository.