Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

experiment: remove value-based reasoning for interior mutability #122789

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 27 additions & 47 deletions compiler/rustc_const_eval/src/transform/check_consts/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use std::mem;
use std::ops::Deref;

use super::ops::{self, NonConstOp, Status};
use super::qualifs::{self, HasMutInterior, NeedsDrop, NeedsNonConstDrop};
use super::qualifs::{self, NeedsDrop, NeedsNonConstDrop};
use super::resolver::FlowSensitiveAnalysis;
use super::{ConstCx, Qualif};
use crate::const_eval::is_unstable_const_fn;
Expand All @@ -31,7 +31,6 @@ type QualifResults<'mir, 'tcx, Q> =

#[derive(Default)]
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 @@ -97,36 +96,6 @@ impl<'mir, 'tcx> Qualifs<'mir, 'tcx> {
needs_non_const_drop.get().contains(local)
}

/// Returns `true` if `local` is `HasMutInterior` at the given `Location`.
///
/// Only updates the cursor if absolutely necessary.
pub fn has_mut_interior(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
location: Location,
) -> bool {
let ty = ccx.body.local_decls[local].ty;
// Peeking into opaque types causes cycles if the current function declares said opaque
// type. Thus we avoid short circuiting on the type and instead run the more expensive
// analysis that looks at the actual usage within this function
if !ty.has_opaque_types() && !HasMutInterior::in_any_value_of_ty(ccx, ty) {
return false;
}

let has_mut_interior = self.has_mut_interior.get_or_insert_with(|| {
let ConstCx { tcx, body, .. } = *ccx;

FlowSensitiveAnalysis::new(HasMutInterior, ccx)
.into_engine(tcx, body)
.iterate_to_fixpoint()
.into_results_cursor(body)
});

has_mut_interior.seek_before_primary_effect(location);
has_mut_interior.get().contains(local)
}

fn in_return_place(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
Expand All @@ -152,7 +121,6 @@ impl<'mir, 'tcx> Qualifs<'mir, 'tcx> {
ConstQualifs {
needs_drop: self.needs_drop(ccx, RETURN_PLACE, return_loc),
needs_non_const_drop: self.needs_non_const_drop(ccx, RETURN_PLACE, return_loc),
has_mut_interior: self.has_mut_interior(ccx, RETURN_PLACE, return_loc),
tainted_by_errors,
}
}
Expand Down Expand Up @@ -373,6 +341,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,20 +467,12 @@ 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
// when the `*` is not the first projection, `place_as_reborrow` does not recognize
// them as such, so we end up here. This should probably be considered a
// `TransientCellBorrow` (we consider the equivalent mutable case a
// `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() {
if borrowed_place_has_mut_interior {
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 @@ -508,6 +483,11 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
// to (interior) mutable memory.
hir::ConstContext::ConstFn => self.check_op(ops::TransientCellBorrow),
_ => {
// For indirect places, we are not creating a new permanent borrow, it's just as
// transient as the already existing one. For reborrowing references this is handled
// at the top of `visit_rvalue`, but for raw pointers we handle it here.
// Pointers/references to `static mut` and cases where the `*` is not the first
// projection also end up here.
// 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
// address taken as we can't end up with a reference to them in the
Expand All @@ -516,7 +496,7 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
// `StorageDead` in every control flow path leading to a `return` terminator.
// The good news is that interning will detect if any unexpected mutable
// pointer slips through.
if self.local_has_storage_dead(place.local) {
if place.is_indirect() || self.local_has_storage_dead(place.local) {
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 @@ -415,7 +415,7 @@ impl<'tcx> NonConstOp<'tcx> for LiveDrop<'tcx> {
pub struct TransientCellBorrow;
impl<'tcx> NonConstOp<'tcx> for TransientCellBorrow {
fn status_in_item(&self, _: &ConstCx<'_, 'tcx>) -> Status {
Status::Unstable(sym::const_refs_to_cell)
Status::Allowed
}
fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> Diag<'tcx> {
ccx.tcx
Expand Down
34 changes: 0 additions & 34 deletions compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ pub fn in_any_value_of_ty<'tcx>(
tainted_by_errors: Option<ErrorGuaranteed>,
) -> ConstQualifs {
ConstQualifs {
has_mut_interior: HasMutInterior::in_any_value_of_ty(cx, ty),
needs_drop: NeedsDrop::in_any_value_of_ty(cx, ty),
needs_non_const_drop: NeedsNonConstDrop::in_any_value_of_ty(cx, ty),
tainted_by_errors,
Expand Down Expand Up @@ -83,39 +82,6 @@ pub trait Qualif {
fn deref_structural<'tcx>(cx: &ConstCx<'_, 'tcx>) -> bool;
}

/// Constant containing interior mutability (`UnsafeCell<T>`).
/// This must be ruled out to make sure that evaluating the constant at compile-time
/// and at *any point* during the run-time would produce the same result. In particular,
/// promotion of temporaries must not change program behavior; if the promoted could be
/// written to, that would be a problem.
pub struct HasMutInterior;

impl Qualif for HasMutInterior {
const ANALYSIS_NAME: &'static str = "flow_has_mut_interior";

fn in_qualifs(qualifs: &ConstQualifs) -> bool {
qualifs.has_mut_interior
}

fn in_any_value_of_ty<'tcx>(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> bool {
!ty.is_freeze(cx.tcx, cx.param_env)
}

fn in_adt_inherently<'tcx>(
_cx: &ConstCx<'_, 'tcx>,
adt: AdtDef<'tcx>,
_: GenericArgsRef<'tcx>,
) -> bool {
// Exactly one type, `UnsafeCell`, has the `HasMutInterior` qualif inherently.
// It arises structurally for all other types.
adt.is_unsafe_cell()
}

fn deref_structural<'tcx>(_cx: &ConstCx<'_, 'tcx>) -> bool {
false
}
}

/// Constant containing an ADT that implements `Drop`.
/// This must be ruled out because implicit promotion would remove side-effects
/// that occur as part of dropping that value. N.B., the implicit promotion has
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_middle/src/mir/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,6 @@ pub struct BorrowCheckResult<'tcx> {
/// `Qualif`.
#[derive(Clone, Copy, Debug, Default, TyEncodable, TyDecodable, HashStable)]
pub struct ConstQualifs {
pub has_mut_interior: bool,
pub needs_drop: bool,
pub needs_non_const_drop: bool,
pub tainted_by_errors: Option<ErrorGuaranteed>,
Expand Down
18 changes: 16 additions & 2 deletions compiler/rustc_mir_transform/src/promote_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,8 +389,22 @@ impl<'tcx> Validator<'_, 'tcx> {
}

BorrowKind::Shared => {
let has_mut_interior = self.qualif_local::<qualifs::HasMutInterior>(place.local);
if has_mut_interior {
// 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);
}
}
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_pattern_analysis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#![allow(rustc::untranslatable_diagnostic)]
#![allow(rustc::diagnostic_outside_of_impl)]
#![feature(freeze)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this actually gets merged, please set this only when feature = "rustc". This crate needs to build on stable for rust-analyzer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it needed just for this?

PatOrWild::Wild => &Wildcard,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an experiment, it won't get merged. Also the crate can't build on stable any more with this rustc change.

Copy link
Member Author

@RalfJung RalfJung Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it needed just for this?

Yes. That previously got promoted even though other enum variants have interior mutability, which is not entirely a sound thing to do (Cc rust-lang/unsafe-code-guidelines#493).


pub mod constructor;
#[cfg(feature = "rustc")]
Expand Down Expand Up @@ -90,9 +91,9 @@ pub trait PatCx: Sized + fmt::Debug {
/// Errors that can abort analysis.
type Error: fmt::Debug;
/// The index of an enum variant.
type VariantIdx: Clone + index::Idx + fmt::Debug;
type VariantIdx: Clone + index::Idx + fmt::Debug + std::marker::Freeze;
/// A string literal
type StrLit: Clone + PartialEq + fmt::Debug;
type StrLit: Clone + PartialEq + fmt::Debug + std::marker::Freeze;
/// Extra data to store in a match arm.
type ArmData: Copy + Clone + fmt::Debug;
/// Extra data to store in a pattern.
Expand Down
Loading