From 6a958964842af3208c7db3e410b8b969d5f3dd36 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 29 Feb 2024 10:14:16 +0100 Subject: [PATCH 1/4] add comment and test: we do not do value-based reasoning for promotion of unions --- .../src/transform/check_consts/qualifs.rs | 1 + tests/ui/consts/promote-not.rs | 9 +++ tests/ui/consts/promote-not.stderr | 61 +++++++++++-------- 3 files changed, 46 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs b/compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs index 7eb3c181d6958..1847847d9d2ae 100644 --- a/compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs +++ b/compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs @@ -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; } diff --git a/tests/ui/consts/promote-not.rs b/tests/ui/consts/promote-not.rs index 47a06e8a72b71..e0f02354354b1 100644 --- a/tests/ui/consts/promote-not.rs +++ b/tests/ui/consts/promote-not.rs @@ -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 @@ -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>, + } + let x: &'static _ = &UnionWithCell { f1: 0 }; + //~^ ERROR temporary value dropped while borrowed } diff --git a/tests/ui/consts/promote-not.stderr b/tests/ui/consts/promote-not.stderr index 67ac5922efd96..9dbd703d48903 100644 --- a/tests/ui/consts/promote-not.stderr +++ b/tests/ui/consts/promote-not.stderr @@ -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]); | ----------^^^^^^^^^- @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -246,7 +246,7 @@ 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 @@ -254,6 +254,17 @@ LL | let _val: &'static _ = &[&TEST_DROP; 1]; | | 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`. From 3ad70a1ade5f0c643225ccead9ea48fce969b4fe Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 29 Feb 2024 10:16:05 +0100 Subject: [PATCH 2/4] const checking: do not do value-based reasoning for interior mutability --- .../src/const_eval/eval_queries.rs | 8 ++--- .../src/transform/check_consts/check.rs | 32 ++++++++++++++----- tests/ui/consts/refs-to-cell-in-final.rs | 15 +++++++++ tests/ui/consts/refs-to-cell-in-final.stderr | 8 ++++- 4 files changed, 50 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs index 9e4e7911c3a73..c18b5b795d427 100644 --- a/compiler/rustc_const_eval/src/const_eval/eval_queries.rs +++ b/compiler/rustc_const_eval/src/const_eval/eval_queries.rs @@ -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)?; diff --git a/compiler/rustc_const_eval/src/transform/check_consts/check.rs b/compiler/rustc_const_eval/src/transform/check_consts/check.rs index c98dc4deb7579..165d91aface44 100644 --- a/compiler/rustc_const_eval/src/transform/check_consts/check.rs +++ b/compiler/rustc_const_eval/src/transform/check_consts/check.rs @@ -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>, needs_drop: Option>, needs_non_const_drop: Option>, @@ -484,11 +484,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::( - 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 = + !place.ty(self.body, self.tcx).ty.is_freeze(self.tcx, self.param_env); // 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 @@ -498,6 +497,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::( + 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 @@ -506,7 +516,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 @@ -517,7 +531,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); } diff --git a/tests/ui/consts/refs-to-cell-in-final.rs b/tests/ui/consts/refs-to-cell-in-final.rs index 6a849ff0df929..1b606ca64388e 100644 --- a/tests/ui/consts/refs-to-cell-in-final.rs +++ b/tests/ui/consts/refs-to-cell-in-final.rs @@ -15,4 +15,19 @@ static RAW_SYNC_S: SyncPtr> = SyncPtr { x: &Cell::new(42) }; const RAW_SYNC_C: SyncPtr> = SyncPtr { x: &Cell::new(42) }; //~^ ERROR: cannot refer to interior mutable data +// This one does not get promoted because of `Drop`, and then enters interesting codepaths because +// as a value it has no interior mutability, but as a type it does. See +// . Value-based reasoning for interior mutability +// is questionable (https://github.com/rust-lang/unsafe-code-guidelines/issues/493) so for hnow we +// reject this, i.e., this tests that const-qualif does *not* do value-based reasoning. +pub enum JsValue { + Undefined, + Object(Cell), +} +impl Drop for JsValue { + fn drop(&mut self) {} +} +const UNDEFINED: &JsValue = &JsValue::Undefined; +//~^ERROR: cannot refer to interior mutable data + fn main() {} diff --git a/tests/ui/consts/refs-to-cell-in-final.stderr b/tests/ui/consts/refs-to-cell-in-final.stderr index fae16fa01251c..533275999c4b9 100644 --- a/tests/ui/consts/refs-to-cell-in-final.stderr +++ b/tests/ui/consts/refs-to-cell-in-final.stderr @@ -12,6 +12,12 @@ error[E0492]: constants cannot refer to interior mutable data LL | const RAW_SYNC_C: SyncPtr> = SyncPtr { x: &Cell::new(42) }; | ^^^^^^^^^^^^^^ this borrow of an interior mutable value may end up in the final value -error: aborting due to 2 previous errors +error[E0492]: constants cannot refer to interior mutable data + --> $DIR/refs-to-cell-in-final.rs:30:29 + | +LL | const UNDEFINED: &JsValue = &JsValue::Undefined; + | ^^^^^^^^^^^^^^^^^^^ this borrow of an interior mutable value may end up in the final value + +error: aborting due to 3 previous errors For more information about this error, try `rustc --explain E0492`. From ac1ed220e1d970854678ec7002ec0c4b671a9c5f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 1 Mar 2024 12:22:05 +0100 Subject: [PATCH 3/4] add an interesting test; make const-check understand that empty arrays are not interior mutable --- .../src/transform/check_consts/check.rs | 17 ++++++++++++++++- tests/ui/consts/enclosing-scope-rule.rs | 16 ++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 tests/ui/consts/enclosing-scope-rule.rs diff --git a/compiler/rustc_const_eval/src/transform/check_consts/check.rs b/compiler/rustc_const_eval/src/transform/check_consts/check.rs index 165d91aface44..22331e5739b66 100644 --- a/compiler/rustc_const_eval/src/transform/check_consts/check.rs +++ b/compiler/rustc_const_eval/src/transform/check_consts/check.rs @@ -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> { @@ -487,7 +502,7 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> { // 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 = - !place.ty(self.body, self.tcx).ty.is_freeze(self.tcx, self.param_env); + 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 diff --git a/tests/ui/consts/enclosing-scope-rule.rs b/tests/ui/consts/enclosing-scope-rule.rs new file mode 100644 index 0000000000000..7041ad9af34e0 --- /dev/null +++ b/tests/ui/consts/enclosing-scope-rule.rs @@ -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); +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 = &Vec::new(); + +fn main() {} From c7d99c858ecd9dfce00d8685c72d13d0eb87619a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 2 Mar 2024 10:45:26 +0100 Subject: [PATCH 4/4] experiment: do not use value-based interior mut reasoning for promotion --- .../rustc_mir_transform/src/promote_consts.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/compiler/rustc_mir_transform/src/promote_consts.rs b/compiler/rustc_mir_transform/src/promote_consts.rs index 2e11da4d585ef..7aff68de22f92 100644 --- a/compiler/rustc_mir_transform/src/promote_consts.rs +++ b/compiler/rustc_mir_transform/src/promote_consts.rs @@ -393,6 +393,24 @@ impl<'tcx> Validator<'_, 'tcx> { if has_mut_interior { return Err(Unpromotable); } + // Let's just see what happens if we reject anything `!Freeze`... + // (Except ZST which definitely can't have interior mut) + let ty = place.ty(self.body, self.tcx).ty; + let has_interior_mut = 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), + }; + if has_interior_mut { + return Err(Unpromotable); + } } // FIXME: consider changing this to only promote &mut [] for default borrows,