From 93f85f5a9d97fc49826d0063c9db0a71602d3283 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Sat, 23 Oct 2021 00:00:00 +0000 Subject: [PATCH] Consider indirect mutation during const qualification dataflow Previously a local would be qualified if either one of two separate data flow computations indicated so. First determined if a local could contain the qualif, but ignored any forms of indirect mutation. Second determined if a local could be mutably borrowed (and so indirectly mutated), but which in turn ignored the qualif. The end result was incorrect because the effect of indirect mutation was effectivelly ignored in the all but the final stage of computation. In the new implementation the indirect mutation is directly incorporated into the qualif data flow. The local variable becomes immediately qualified once it is mutably borrowed and borrowed place type can contain the qualif. In general we will now reject additional programs, program that were prevously unintentionally accepted. There are also some cases which are now accepted but were previously rejected, because previous implementation didn't consider whether borrowed place could have the qualif under the consideration. --- .../src/transform/check_consts/check.rs | 42 +---- .../src/transform/check_consts/resolver.rs | 178 +++++++++++++++--- .../consts/qualif-indirect-mutation-fail.rs | 44 +++++ .../qualif-indirect-mutation-fail.stderr | 33 ++++ .../consts/qualif-indirect-mutation-pass.rs | 16 ++ 5 files changed, 250 insertions(+), 63 deletions(-) create mode 100644 src/test/ui/consts/qualif-indirect-mutation-fail.rs create mode 100644 src/test/ui/consts/qualif-indirect-mutation-fail.stderr create mode 100644 src/test/ui/consts/qualif-indirect-mutation-pass.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 85f37c813d8..3cb4d415021 100644 --- a/compiler/rustc_const_eval/src/transform/check_consts/check.rs +++ b/compiler/rustc_const_eval/src/transform/check_consts/check.rs @@ -12,7 +12,6 @@ use rustc_middle::ty::cast::CastTy; use rustc_middle::ty::subst::{GenericArgKind, InternalSubsts}; use rustc_middle::ty::{self, adjustment::PointerCast, Instance, InstanceDef, Ty, TyCtxt}; use rustc_middle::ty::{Binder, TraitPredicate, TraitRef}; -use rustc_mir_dataflow::impls::MaybeMutBorrowedLocals; use rustc_mir_dataflow::{self, Analysis}; use rustc_span::{sym, Span, Symbol}; use rustc_trait_selection::traits::error_reporting::InferCtxtExt; @@ -27,12 +26,6 @@ use super::resolver::FlowSensitiveAnalysis; use super::{is_lang_panic_fn, is_lang_special_const_fn, ConstCx, Qualif}; use crate::const_eval::is_unstable_const_fn; -// We are using `MaybeMutBorrowedLocals` as a proxy for whether an item may have been mutated -// through a pointer prior to the given point. This is okay even though `MaybeMutBorrowedLocals` -// kills locals upon `StorageDead` because a local will never be used after a `StorageDead`. -type IndirectlyMutableResults<'mir, 'tcx> = - rustc_mir_dataflow::ResultsCursor<'mir, 'tcx, MaybeMutBorrowedLocals<'mir, 'tcx>>; - type QualifResults<'mir, 'tcx, Q> = rustc_mir_dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'mir, 'mir, 'tcx, Q>>; @@ -41,36 +34,9 @@ pub struct Qualifs<'mir, 'tcx> { has_mut_interior: Option>, needs_drop: Option>, needs_non_const_drop: Option>, - indirectly_mutable: Option>, } impl Qualifs<'mir, 'tcx> { - pub fn indirectly_mutable( - &mut self, - ccx: &'mir ConstCx<'mir, 'tcx>, - local: Local, - location: Location, - ) -> bool { - let indirectly_mutable = self.indirectly_mutable.get_or_insert_with(|| { - let ConstCx { tcx, body, param_env, .. } = *ccx; - - // We can use `unsound_ignore_borrow_on_drop` here because custom drop impls are not - // allowed in a const. - // - // FIXME(ecstaticmorse): Someday we want to allow custom drop impls. How do we do this - // without breaking stable code? - MaybeMutBorrowedLocals::mut_borrows_only(tcx, &body, param_env) - .unsound_ignore_borrow_on_drop() - .into_engine(tcx, &body) - .pass_name("const_qualification") - .iterate_to_fixpoint() - .into_results_cursor(&body) - }); - - indirectly_mutable.seek_before_primary_effect(location); - indirectly_mutable.get().contains(local) - } - /// Returns `true` if `local` is `NeedsDrop` at the given `Location`. /// /// Only updates the cursor if absolutely necessary @@ -95,7 +61,7 @@ impl Qualifs<'mir, 'tcx> { }); needs_drop.seek_before_primary_effect(location); - needs_drop.get().contains(local) || self.indirectly_mutable(ccx, local, location) + needs_drop.get().contains(local) } /// Returns `true` if `local` is `NeedsNonConstDrop` at the given `Location`. @@ -122,7 +88,7 @@ impl Qualifs<'mir, 'tcx> { }); needs_non_const_drop.seek_before_primary_effect(location); - needs_non_const_drop.get().contains(local) || self.indirectly_mutable(ccx, local, location) + needs_non_const_drop.get().contains(local) } /// Returns `true` if `local` is `HasMutInterior` at the given `Location`. @@ -149,7 +115,7 @@ impl Qualifs<'mir, 'tcx> { }); has_mut_interior.seek_before_primary_effect(location); - has_mut_interior.get().contains(local) || self.indirectly_mutable(ccx, local, location) + has_mut_interior.get().contains(local) } fn in_return_place( @@ -195,7 +161,7 @@ impl Qualifs<'mir, 'tcx> { .into_results_cursor(&ccx.body); cursor.seek_after_primary_effect(return_loc); - cursor.contains(RETURN_PLACE) + cursor.get().contains(RETURN_PLACE) } }; diff --git a/compiler/rustc_const_eval/src/transform/check_consts/resolver.rs b/compiler/rustc_const_eval/src/transform/check_consts/resolver.rs index e20b86dd452..ff2271fb396 100644 --- a/compiler/rustc_const_eval/src/transform/check_consts/resolver.rs +++ b/compiler/rustc_const_eval/src/transform/check_consts/resolver.rs @@ -5,7 +5,11 @@ use rustc_index::bit_set::BitSet; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::{self, BasicBlock, Local, Location, Statement, StatementKind}; +use rustc_mir_dataflow::fmt::DebugWithContext; +use rustc_mir_dataflow::JoinSemiLattice; +use rustc_span::DUMMY_SP; +use std::fmt; use std::marker::PhantomData; use super::{qualifs, ConstCx, Qualif}; @@ -13,13 +17,13 @@ use super::{qualifs, ConstCx, Qualif}; /// A `Visitor` that propagates qualifs between locals. This defines the transfer function of /// `FlowSensitiveAnalysis`. /// -/// This transfer does nothing when encountering an indirect assignment. Consumers should rely on -/// the `MaybeMutBorrowedLocals` dataflow pass to see if a `Local` may have become qualified via -/// an indirect assignment or function call. +/// To account for indirect assignments, data flow conservatively assumes that local becomes +/// qualified immediately after it is borrowed or its address escapes. The borrow must allow for +/// mutation, which includes shared borrows of places with interior mutability. The type of +/// borrowed place must contain the qualif. struct TransferFunction<'a, 'mir, 'tcx, Q> { ccx: &'a ConstCx<'mir, 'tcx>, - qualifs_per_local: &'a mut BitSet, - + state: &'a mut State, _qualif: PhantomData, } @@ -27,17 +31,18 @@ impl TransferFunction<'a, 'mir, 'tcx, Q> where Q: Qualif, { - fn new(ccx: &'a ConstCx<'mir, 'tcx>, qualifs_per_local: &'a mut BitSet) -> Self { - TransferFunction { ccx, qualifs_per_local, _qualif: PhantomData } + fn new(ccx: &'a ConstCx<'mir, 'tcx>, state: &'a mut State) -> Self { + TransferFunction { ccx, state, _qualif: PhantomData } } fn initialize_state(&mut self) { - self.qualifs_per_local.clear(); + self.state.qualif.clear(); + self.state.borrow.clear(); for arg in self.ccx.body.args_iter() { let arg_ty = self.ccx.body.local_decls[arg].ty; if Q::in_any_value_of_ty(self.ccx, arg_ty) { - self.qualifs_per_local.insert(arg); + self.state.qualif.insert(arg); } } } @@ -47,7 +52,7 @@ where match (value, place.as_ref()) { (true, mir::PlaceRef { local, .. }) => { - self.qualifs_per_local.insert(local); + self.state.qualif.insert(local); } // For now, we do not clear the qualif if a local is overwritten in full by @@ -55,7 +60,7 @@ where // with aggregates where we overwrite all fields with assignments, which would not // get this feature. (false, mir::PlaceRef { local: _, projection: &[] }) => { - // self.qualifs_per_local.remove(*local); + // self.state.qualif.remove(*local); } _ => {} @@ -78,6 +83,29 @@ where self.assign_qualif_direct(&return_place, qualif); } } + + fn address_of_allows_mutation(&self, mt: mir::Mutability, place: mir::Place<'tcx>) -> bool { + match mt { + mir::Mutability::Mut => true, + mir::Mutability::Not => self.shared_borrow_allows_mutation(place), + } + } + + fn ref_allows_mutation(&self, kind: mir::BorrowKind, place: mir::Place<'tcx>) -> bool { + match kind { + mir::BorrowKind::Mut { .. } => true, + mir::BorrowKind::Shared | mir::BorrowKind::Shallow | mir::BorrowKind::Unique => { + self.shared_borrow_allows_mutation(place) + } + } + } + + fn shared_borrow_allows_mutation(&self, place: mir::Place<'tcx>) -> bool { + !place + .ty(self.ccx.body, self.ccx.tcx) + .ty + .is_freeze(self.ccx.tcx.at(DUMMY_SP), self.ccx.param_env) + } } impl Visitor<'tcx> for TransferFunction<'_, '_, 'tcx, Q> @@ -95,7 +123,12 @@ where // it no longer needs to be dropped. if let mir::Operand::Move(place) = operand { if let Some(local) = place.as_local() { - self.qualifs_per_local.remove(local); + // For backward compatibility with the MaybeMutBorrowedLocals used in an earlier + // implementation we retain qualif if a local had been borrowed before. This might + // not be strictly necessary since the local is no longer initialized. + if !self.state.borrow.contains(local) { + self.state.qualif.remove(local); + } } } } @@ -106,11 +139,8 @@ where rvalue: &mir::Rvalue<'tcx>, location: Location, ) { - let qualif = qualifs::in_rvalue::( - self.ccx, - &mut |l| self.qualifs_per_local.contains(l), - rvalue, - ); + let qualif = + qualifs::in_rvalue::(self.ccx, &mut |l| self.state.qualif.contains(l), rvalue); if !place.is_indirect() { self.assign_qualif_direct(place, qualif); } @@ -120,10 +150,53 @@ where self.super_assign(place, rvalue, location); } + fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { + self.super_rvalue(rvalue, location); + + match rvalue { + mir::Rvalue::AddressOf(mt, borrowed_place) => { + if !borrowed_place.is_indirect() + && self.address_of_allows_mutation(*mt, *borrowed_place) + { + let place_ty = borrowed_place.ty(self.ccx.body, self.ccx.tcx).ty; + if Q::in_any_value_of_ty(self.ccx, place_ty) { + self.state.qualif.insert(borrowed_place.local); + self.state.borrow.insert(borrowed_place.local); + } + } + } + + mir::Rvalue::Ref(_, kind, borrowed_place) => { + if !borrowed_place.is_indirect() && self.ref_allows_mutation(*kind, *borrowed_place) + { + let place_ty = borrowed_place.ty(self.ccx.body, self.ccx.tcx).ty; + if Q::in_any_value_of_ty(self.ccx, place_ty) { + self.state.qualif.insert(borrowed_place.local); + self.state.borrow.insert(borrowed_place.local); + } + } + } + + mir::Rvalue::Cast(..) + | mir::Rvalue::ShallowInitBox(..) + | mir::Rvalue::Use(..) + | mir::Rvalue::ThreadLocalRef(..) + | mir::Rvalue::Repeat(..) + | mir::Rvalue::Len(..) + | mir::Rvalue::BinaryOp(..) + | mir::Rvalue::CheckedBinaryOp(..) + | mir::Rvalue::NullaryOp(..) + | mir::Rvalue::UnaryOp(..) + | mir::Rvalue::Discriminant(..) + | mir::Rvalue::Aggregate(..) => {} + } + } + fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) { match statement.kind { StatementKind::StorageDead(local) => { - self.qualifs_per_local.remove(local); + self.state.qualif.remove(local); + self.state.borrow.remove(local); } _ => self.super_statement(statement, location), } @@ -136,7 +209,7 @@ where if let mir::TerminatorKind::DropAndReplace { value, place, .. } = &terminator.kind { let qualif = qualifs::in_operand::( self.ccx, - &mut |l| self.qualifs_per_local.contains(l), + &mut |l| self.state.qualif.contains(l), value, ); @@ -145,6 +218,9 @@ where } } + // We ignore borrow on drop because custom drop impls are not allowed in consts. + // FIXME: Reconsider if accounting for borrows in drops is necessary for const drop. + // We need to assign qualifs to the dropped location before visiting the operand that // replaces it since qualifs can be cleared on move. self.super_terminator(terminator, location); @@ -165,24 +241,76 @@ where FlowSensitiveAnalysis { ccx, _qualif: PhantomData } } - fn transfer_function( - &self, - state: &'a mut BitSet, - ) -> TransferFunction<'a, 'mir, 'tcx, Q> { + fn transfer_function(&self, state: &'a mut State) -> TransferFunction<'a, 'mir, 'tcx, Q> { TransferFunction::::new(self.ccx, state) } } +#[derive(Clone, Debug, PartialEq, Eq)] +pub(super) struct State { + /// Describes whether a local contains qualif. + pub qualif: BitSet, + /// Describes whether a local's address escaped and it might become qualified as a result an + /// indirect mutation. + pub borrow: BitSet, +} + +impl State { + #[inline] + pub(super) fn contains(&self, local: Local) -> bool { + self.qualif.contains(local) + } +} + +impl DebugWithContext for State { + fn fmt_with(&self, ctxt: &C, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("qualif: ")?; + self.qualif.fmt_with(ctxt, f)?; + f.write_str(" borrow: ")?; + self.borrow.fmt_with(ctxt, f)?; + Ok(()) + } + + fn fmt_diff_with(&self, old: &Self, ctxt: &C, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self == old { + return Ok(()); + } + + if self.qualif != old.qualif { + f.write_str("qualif: ")?; + self.qualif.fmt_diff_with(&old.qualif, ctxt, f)?; + f.write_str("\n")?; + } + + if self.borrow != old.borrow { + f.write_str("borrow: ")?; + self.qualif.fmt_diff_with(&old.borrow, ctxt, f)?; + f.write_str("\n")?; + } + + Ok(()) + } +} + +impl JoinSemiLattice for State { + fn join(&mut self, other: &Self) -> bool { + self.qualif.join(&other.qualif) || self.borrow.join(&other.borrow) + } +} + impl rustc_mir_dataflow::AnalysisDomain<'tcx> for FlowSensitiveAnalysis<'_, '_, 'tcx, Q> where Q: Qualif, { - type Domain = BitSet; + type Domain = State; const NAME: &'static str = Q::ANALYSIS_NAME; fn bottom_value(&self, body: &mir::Body<'tcx>) -> Self::Domain { - BitSet::new_empty(body.local_decls.len()) + State { + qualif: BitSet::new_empty(body.local_decls.len()), + borrow: BitSet::new_empty(body.local_decls.len()), + } } fn initialize_start_block(&self, _body: &mir::Body<'tcx>, state: &mut Self::Domain) { diff --git a/src/test/ui/consts/qualif-indirect-mutation-fail.rs b/src/test/ui/consts/qualif-indirect-mutation-fail.rs new file mode 100644 index 00000000000..cedead00fec --- /dev/null +++ b/src/test/ui/consts/qualif-indirect-mutation-fail.rs @@ -0,0 +1,44 @@ +// compile-flags: --crate-type=lib +#![feature(const_mut_refs)] +#![feature(const_precise_live_drops)] +#![feature(const_swap)] + +// Mutable borrow of a field with drop impl. +pub const fn f() { + let mut a: (u32, Option) = (0, None); //~ ERROR destructors cannot be evaluated + let _ = &mut a.1; +} + +// Mutable borrow of a type with drop impl. +pub const A1: () = { + let mut x = None; //~ ERROR destructors cannot be evaluated + let mut y = Some(String::new()); + let a = &mut x; + let b = &mut y; + std::mem::swap(a, b); + std::mem::forget(y); +}; + +// Mutable borrow of a type with drop impl. +pub const A2: () = { + let mut x = None; + let mut y = Some(String::new()); + let a = &mut x; + let b = &mut y; + std::mem::swap(a, b); + std::mem::forget(y); + let _z = x; //~ ERROR destructors cannot be evaluated +}; + +// Shared borrow of a type that might be !Freeze and Drop. +pub const fn g1() { + let x: Option = None; //~ ERROR destructors cannot be evaluated + let _ = x.is_some(); +} + +// Shared borrow of a type that might be !Freeze and Drop. +pub const fn g2() { + let x: Option = None; + let _ = x.is_some(); + let _y = x; //~ ERROR destructors cannot be evaluated +} diff --git a/src/test/ui/consts/qualif-indirect-mutation-fail.stderr b/src/test/ui/consts/qualif-indirect-mutation-fail.stderr new file mode 100644 index 00000000000..aa6ed465594 --- /dev/null +++ b/src/test/ui/consts/qualif-indirect-mutation-fail.stderr @@ -0,0 +1,33 @@ +error[E0493]: destructors cannot be evaluated at compile-time + --> $DIR/qualif-indirect-mutation-fail.rs:8:9 + | +LL | let mut a: (u32, Option) = (0, None); + | ^^^^^ constant functions cannot evaluate destructors + +error[E0493]: destructors cannot be evaluated at compile-time + --> $DIR/qualif-indirect-mutation-fail.rs:14:9 + | +LL | let mut x = None; + | ^^^^^ constants cannot evaluate destructors + +error[E0493]: destructors cannot be evaluated at compile-time + --> $DIR/qualif-indirect-mutation-fail.rs:30:9 + | +LL | let _z = x; + | ^^ constants cannot evaluate destructors + +error[E0493]: destructors cannot be evaluated at compile-time + --> $DIR/qualif-indirect-mutation-fail.rs:35:9 + | +LL | let x: Option = None; + | ^ constant functions cannot evaluate destructors + +error[E0493]: destructors cannot be evaluated at compile-time + --> $DIR/qualif-indirect-mutation-fail.rs:43:9 + | +LL | let _y = x; + | ^^ constant functions cannot evaluate destructors + +error: aborting due to 5 previous errors + +For more information about this error, try `rustc --explain E0493`. diff --git a/src/test/ui/consts/qualif-indirect-mutation-pass.rs b/src/test/ui/consts/qualif-indirect-mutation-pass.rs new file mode 100644 index 00000000000..35a9b70a5f6 --- /dev/null +++ b/src/test/ui/consts/qualif-indirect-mutation-pass.rs @@ -0,0 +1,16 @@ +// compile-flags: --crate-type=lib +// check-pass +#![feature(const_mut_refs)] +#![feature(const_precise_live_drops)] + +pub const fn f() { + let mut x: (Option, u32) = (None, 0); + let mut a = 10; + *(&mut a) = 11; + x.1 = a; +} + +pub const fn g() { + let mut a: (u32, Option) = (0, None); + let _ = &mut a.0; +}