Skip to content

Commit

Permalink
Auto merge of #121786 - RalfJung:interior-mut-value-reasoning, r=<try>
Browse files Browse the repository at this point in the history
const checking: do not do value-based reasoning for interior mutability

This basically means a nicer error for #121610, see the new test in `tests/ui/consts/refs-to-cell-in-final.rs`.

Currently we have a mismatch between the dynamic semantics of `&` (things might be mutable when the pointee type is `!Freeze`, no matter the pointee value) and const-qualif (doing value-based reasoning). This removes that mismatch. I don't think this can break anything that builds on current nightly; any such code would already later run into the error that the interner complains about the mutable pointer (that's exactly #121610). So a crater run makes little sense.

Overall this peanuts compared to the *actual* problem with value-based reasoning for interior mutability in promotion (rust-lang/unsafe-code-guidelines#493). I have no idea how to resolve that. But at least now the use of the problematic qualif is down to 1...

The first commit is independent, I just got worried about promotion of unions and was happy to notice that at least there we don't do value-based reasoning.

r? `@oli-obk`
  • Loading branch information
bors committed Mar 2, 2024
2 parents 4cdd205 + ac1ed22 commit ca7d067
Show file tree
Hide file tree
Showing 8 changed files with 127 additions and 38 deletions.
8 changes: 4 additions & 4 deletions compiler/rustc_const_eval/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,10 +412,10 @@ pub fn const_validate_mplace<'mir, 'tcx>(
_ if cid.promoted.is_some() => CtfeValidationMode::Promoted,
Some(mutbl) => CtfeValidationMode::Static { mutbl }, // a `static`
None => {
// In normal `const` (not promoted), the outermost allocation is always only copied,
// so having `UnsafeCell` in there is okay despite them being in immutable memory.
let allow_immutable_unsafe_cell = cid.promoted.is_none() && !inner;
CtfeValidationMode::Const { allow_immutable_unsafe_cell }
// This is a normal `const` (not promoted).
// The outermost allocation is always only copied, so having `UnsafeCell` in there
// is okay despite them being in immutable memory.
CtfeValidationMode::Const { allow_immutable_unsafe_cell: !inner }
}
};
ecx.const_validate_operand(&mplace.into(), path, &mut ref_tracking, mode)?;
Expand Down
47 changes: 39 additions & 8 deletions compiler/rustc_const_eval/src/transform/check_consts/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type QualifResults<'mir, 'tcx, Q> =
rustc_mir_dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'mir, 'mir, 'tcx, Q>>;

#[derive(Default)]
pub struct Qualifs<'mir, 'tcx> {
pub(crate) struct Qualifs<'mir, 'tcx> {
has_mut_interior: Option<QualifResults<'mir, 'tcx, HasMutInterior>>,
needs_drop: Option<QualifResults<'mir, 'tcx, NeedsDrop>>,
needs_non_const_drop: Option<QualifResults<'mir, 'tcx, NeedsNonConstDrop>>,
Expand Down Expand Up @@ -373,6 +373,21 @@ impl<'mir, 'tcx> Checker<'mir, 'tcx> {
}
}
}

fn has_interior_mut(&self, ty: Ty<'tcx>) -> bool {
match ty.kind() {
// Empty arrays have no interior mutability no matter their element type.
ty::Array(_elem, count)
if count
.try_eval_target_usize(self.tcx, self.param_env)
.is_some_and(|v| v == 0) =>
{
false
}
// Fallback to checking `Freeze`.
_ => !ty.is_freeze(self.tcx, self.param_env),
}
}
}

impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
Expand Down Expand Up @@ -484,11 +499,10 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {

Rvalue::Ref(_, BorrowKind::Shared | BorrowKind::Fake, place)
| Rvalue::AddressOf(Mutability::Not, place) => {
let borrowed_place_has_mut_interior = qualifs::in_place::<HasMutInterior, _>(
self.ccx,
&mut |local| self.qualifs.has_mut_interior(self.ccx, local, location),
place.as_ref(),
);
// We don't do value-based reasoning here, since the rules for interior mutability
// are not finalized yet and they seem likely to not be full value-based in the end.
let borrowed_place_has_mut_interior =
self.has_interior_mut(place.ty(self.body, self.tcx).ty);

// If the place is indirect, this is basically a reborrow. We have a reborrow
// special case above, but for raw pointers and pointers/references to `static` and
Expand All @@ -498,6 +512,17 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
// `TransientMutBorrow`), but such reborrows got accidentally stabilized already and
// it is too much of a breaking change to take back.
if borrowed_place_has_mut_interior && !place.is_indirect() {
// We used to do a value-based check here, so when we changed to a purely
// type-based check we started rejecting code that used to work on stable. So
// for that reason we already stabilize "transient borrows of interior mutable
// borrows where value-based reasoning says that there actually is no interior
// mutability". We don't do anything like that for non-transient borrows since
// those are and will remain hard errors.
let allow_transient_on_stable = !qualifs::in_place::<HasMutInterior, _>(
self.ccx,
&mut |local| self.qualifs.has_mut_interior(self.ccx, local, location),
place.as_ref(),
);
match self.const_kind() {
// In a const fn all borrows are transient or point to the places given via
// references in the arguments (so we already checked them with
Expand All @@ -506,7 +531,11 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
// NOTE: Once we have heap allocations during CTFE we need to figure out
// how to prevent `const fn` to create long-lived allocations that point
// to (interior) mutable memory.
hir::ConstContext::ConstFn => self.check_op(ops::TransientCellBorrow),
hir::ConstContext::ConstFn => {
if !allow_transient_on_stable {
self.check_op(ops::TransientCellBorrow)
}
}
_ => {
// Locals with StorageDead are definitely not part of the final constant value, and
// it is thus inherently safe to permit such locals to have their
Expand All @@ -517,7 +546,9 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
// The good news is that interning will detect if any unexpected mutable
// pointer slips through.
if self.local_has_storage_dead(place.local) {
self.check_op(ops::TransientCellBorrow);
if !allow_transient_on_stable {
self.check_op(ops::TransientCellBorrow);
}
} else {
self.check_op(ops::CellBorrow);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ where
if Q::in_adt_inherently(cx, def, args) {
return true;
}
// Don't do any value-based reasoning for unions.
if def.is_union() && Q::in_any_value_of_ty(cx, rvalue.ty(cx.body, cx.tcx)) {
return true;
}
Expand Down
16 changes: 16 additions & 0 deletions tests/ui/consts/enclosing-scope-rule.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//@build-pass
// Some code that looks like it might be relying on promotion, but actually this is using the
// enclosing-scope rule, meaning the reference is "extended" to outlive its block and live as long
// as the surrounding block (which in this case is the entire program). There are multiple
// allocations being interned at once.

struct Gen<T>(T);
impl<'a, T> Gen<&'a T> {
// Can't be promoted because `T` might not be `'static`.
const C: &'a [T] = &[];
}

// Can't be promoted because of `Drop`.
const V: &Vec<i32> = &Vec::new();

fn main() {}
9 changes: 9 additions & 0 deletions tests/ui/consts/promote-not.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#![allow(unconditional_panic)]

use std::cell::Cell;
use std::mem::ManuallyDrop;

// We do not promote mutable references.
static mut TEST1: Option<&mut [i32]> = Some(&mut [1, 2, 3]); //~ ERROR temporary value dropped while borrowed
Expand Down Expand Up @@ -69,4 +70,12 @@ fn main() {
let _val: &'static _ = &[&TEST_DROP; 1];
//~^ ERROR temporary value dropped while borrowed
//~| ERROR temporary value dropped while borrowed

// Make sure there is no value-based reasoning for unions.
union UnionWithCell {
f1: i32,
f2: ManuallyDrop<Cell<i32>>,
}
let x: &'static _ = &UnionWithCell { f1: 0 };
//~^ ERROR temporary value dropped while borrowed
}
61 changes: 36 additions & 25 deletions tests/ui/consts/promote-not.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:8:50
--> $DIR/promote-not.rs:9:50
|
LL | static mut TEST1: Option<&mut [i32]> = Some(&mut [1, 2, 3]);
| ----------^^^^^^^^^-
Expand All @@ -9,7 +9,7 @@ LL | static mut TEST1: Option<&mut [i32]> = Some(&mut [1, 2, 3]);
| using this value as a static requires that borrow lasts for `'static`

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:11:18
--> $DIR/promote-not.rs:12:18
|
LL | let x = &mut [1,2,3];
| ^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -19,7 +19,7 @@ LL | };
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:33:29
--> $DIR/promote-not.rs:34:29
|
LL | let _x: &'static i32 = &unsafe { U { x: 0 }.x };
| ------------ ^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -29,7 +29,7 @@ LL | };
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:39:29
--> $DIR/promote-not.rs:40:29
|
LL | let _val: &'static _ = &(Cell::new(1), 2).1;
| ---------- ^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -39,7 +39,7 @@ LL | };
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:20:32
--> $DIR/promote-not.rs:21:32
|
LL | let _x: &'static () = &foo();
| ----------- ^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -49,7 +49,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:28:29
--> $DIR/promote-not.rs:29:29
|
LL | let _x: &'static i32 = &unsafe { U { x: 0 }.x };
| ------------ ^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -59,7 +59,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:46:29
--> $DIR/promote-not.rs:47:29
|
LL | let _val: &'static _ = &(Cell::new(1), 2).0;
| ---------- ^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -70,7 +70,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:47:29
--> $DIR/promote-not.rs:48:29
|
LL | let _val: &'static _ = &(Cell::new(1), 2).1;
| ---------- ^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -81,7 +81,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:50:29
--> $DIR/promote-not.rs:51:29
|
LL | let _val: &'static _ = &(1/0);
| ---------- ^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -92,7 +92,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:51:29
--> $DIR/promote-not.rs:52:29
|
LL | let _val: &'static _ = &(1/(1-1));
| ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -103,7 +103,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:52:29
--> $DIR/promote-not.rs:53:29
|
LL | let _val: &'static _ = &((1+1)/(1-1));
| ---------- ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -114,7 +114,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:53:29
--> $DIR/promote-not.rs:54:29
|
LL | let _val: &'static _ = &(i32::MIN/-1);
| ---------- ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -125,7 +125,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:54:29
--> $DIR/promote-not.rs:55:29
|
LL | let _val: &'static _ = &(i32::MIN/(0-1));
| ---------- ^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -136,7 +136,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:55:29
--> $DIR/promote-not.rs:56:29
|
LL | let _val: &'static _ = &(-128i8/-1);
| ---------- ^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -147,7 +147,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:56:29
--> $DIR/promote-not.rs:57:29
|
LL | let _val: &'static _ = &(1%0);
| ---------- ^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -158,7 +158,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:57:29
--> $DIR/promote-not.rs:58:29
|
LL | let _val: &'static _ = &(1%(1-1));
| ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -169,7 +169,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:58:29
--> $DIR/promote-not.rs:59:29
|
LL | let _val: &'static _ = &([1,2,3][4]+1);
| ---------- ^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -180,7 +180,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:61:29
--> $DIR/promote-not.rs:62:29
|
LL | let _val: &'static _ = &TEST_DROP;
| ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -191,7 +191,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:63:29
--> $DIR/promote-not.rs:64:29
|
LL | let _val: &'static _ = &&TEST_DROP;
| ---------- ^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -202,7 +202,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:63:30
--> $DIR/promote-not.rs:64:30
|
LL | let _val: &'static _ = &&TEST_DROP;
| ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -213,7 +213,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:66:29
--> $DIR/promote-not.rs:67:29
|
LL | let _val: &'static _ = &(&TEST_DROP,);
| ---------- ^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -224,7 +224,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:66:31
--> $DIR/promote-not.rs:67:31
|
LL | let _val: &'static _ = &(&TEST_DROP,);
| ---------- ^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -235,7 +235,7 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:69:29
--> $DIR/promote-not.rs:70:29
|
LL | let _val: &'static _ = &[&TEST_DROP; 1];
| ---------- ^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
Expand All @@ -246,14 +246,25 @@ LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:69:31
--> $DIR/promote-not.rs:70:31
|
LL | let _val: &'static _ = &[&TEST_DROP; 1];
| ---------- ^^^^^^^^^ - temporary value is freed at the end of this statement
| | |
| | creates a temporary value which is freed while still in use
| type annotation requires that borrow lasts for `'static`

error: aborting due to 24 previous errors
error[E0716]: temporary value dropped while borrowed
--> $DIR/promote-not.rs:79:26
|
LL | let x: &'static _ = &UnionWithCell { f1: 0 };
| ---------- ^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
| |
| type annotation requires that borrow lasts for `'static`
LL |
LL | }
| - temporary value is freed at the end of this statement

error: aborting due to 25 previous errors

For more information about this error, try `rustc --explain E0716`.
Loading

0 comments on commit ca7d067

Please sign in to comment.