From 6f75d3fcc04e6da1f10424e29da978a81698e81b Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Fri, 13 Dec 2019 13:20:16 -0800 Subject: [PATCH] Simplify `Qualif` interface --- .../transform/check_consts/qualifs.rs | 335 ++++++++---------- .../transform/check_consts/resolver.rs | 31 +- .../transform/check_consts/validation.rs | 2 +- src/librustc_mir/transform/promote_consts.rs | 12 +- 4 files changed, 178 insertions(+), 202 deletions(-) diff --git a/src/librustc_mir/transform/check_consts/qualifs.rs b/src/librustc_mir/transform/check_consts/qualifs.rs index baff8383c20a4..9359ec16533a5 100644 --- a/src/librustc_mir/transform/check_consts/qualifs.rs +++ b/src/librustc_mir/transform/check_consts/qualifs.rs @@ -1,7 +1,9 @@ -//! A copy of the `Qualif` trait in `qualify_consts.rs` that is suitable for the new validator. +//! Structural const qualification. +//! +//! See the `Qualif` trait for more info. use rustc::mir::*; -use rustc::ty::{self, Ty}; +use rustc::ty::{self, AdtDef, Ty}; use rustc_span::DUMMY_SP; use super::Item as ConstCx; @@ -14,11 +16,16 @@ pub fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> ConstQualifs } /// A "qualif"(-ication) is a way to look for something "bad" in the MIR that would disqualify some -/// code for promotion or prevent it from evaluating at compile time. So `return true` means -/// "I found something bad, no reason to go on searching". `false` is only returned if we -/// definitely cannot find anything bad anywhere. +/// code for promotion or prevent it from evaluating at compile time. /// -/// The default implementations proceed structurally. +/// Normally, we would determine what qualifications apply to each type and error when an illegal +/// operation is performed on such a type. However, this was found to be too imprecise, especially +/// in the presence of `enum`s. If only a single variant of an enum has a certain qualification, we +/// needn't reject code unless it actually constructs and operates on the qualifed variant. +/// +/// To accomplish this, const-checking and promotion use a value-based analysis (as opposed to a +/// type-based one). Qualifications propagate structurally across variables: If a local (or a +/// projection of a local) is assigned a qualifed value, that local itself becomes qualifed. pub trait Qualif { /// The name of the file used to debug the dataflow analysis that computes this qualif. const ANALYSIS_NAME: &'static str; @@ -26,157 +33,27 @@ pub trait Qualif { /// Whether this `Qualif` is cleared when a local is moved from. const IS_CLEARED_ON_MOVE: bool = false; + /// Extracts the field of `ConstQualifs` that corresponds to this `Qualif`. fn in_qualifs(qualifs: &ConstQualifs) -> bool; - /// Return the qualification that is (conservatively) correct for any value - /// of the type. - fn in_any_value_of_ty(_cx: &ConstCx<'_, 'tcx>, _ty: Ty<'tcx>) -> bool; - - fn in_projection_structurally( - cx: &ConstCx<'_, 'tcx>, - per_local: &mut impl FnMut(Local) -> bool, - place: PlaceRef<'tcx>, - ) -> bool { - if let [proj_base @ .., elem] = place.projection { - let base_qualif = Self::in_place( - cx, - per_local, - PlaceRef { local: place.local, projection: proj_base }, - ); - let qualif = base_qualif - && Self::in_any_value_of_ty( - cx, - Place::ty_from(place.local, proj_base, *cx.body, cx.tcx) - .projection_ty(cx.tcx, elem) - .ty, - ); - match elem { - ProjectionElem::Deref - | ProjectionElem::Subslice { .. } - | ProjectionElem::Field(..) - | ProjectionElem::ConstantIndex { .. } - | ProjectionElem::Downcast(..) => qualif, - - ProjectionElem::Index(local) => qualif || per_local(*local), - } - } else { - bug!("This should be called if projection is not empty"); - } - } - - fn in_projection( - cx: &ConstCx<'_, 'tcx>, - per_local: &mut impl FnMut(Local) -> bool, - place: PlaceRef<'tcx>, - ) -> bool { - Self::in_projection_structurally(cx, per_local, place) - } - - fn in_place( - cx: &ConstCx<'_, 'tcx>, - per_local: &mut impl FnMut(Local) -> bool, - place: PlaceRef<'tcx>, - ) -> bool { - match place { - PlaceRef { local, projection: [] } => per_local(local), - PlaceRef { local: _, projection: [.., _] } => Self::in_projection(cx, per_local, place), - } - } - - fn in_operand( - cx: &ConstCx<'_, 'tcx>, - per_local: &mut impl FnMut(Local) -> bool, - operand: &Operand<'tcx>, - ) -> bool { - match *operand { - Operand::Copy(ref place) | Operand::Move(ref place) => { - Self::in_place(cx, per_local, place.as_ref()) - } - - Operand::Constant(ref constant) => { - // Check the qualifs of the value of `const` items. - if let ty::ConstKind::Unevaluated(def_id, _, promoted) = constant.literal.val { - assert!(promoted.is_none()); - // Don't peek inside trait associated constants. - if cx.tcx.trait_of_item(def_id).is_none() { - let qualifs = cx.tcx.at(constant.span).mir_const_qualif(def_id); - if !Self::in_qualifs(&qualifs) { - return false; - } - - // Just in case the type is more specific than - // the definition, e.g., impl associated const - // with type parameters, take it into account. - } - } - // Otherwise use the qualifs of the type. - Self::in_any_value_of_ty(cx, constant.literal.ty) - } - } - } - - fn in_rvalue_structurally( - cx: &ConstCx<'_, 'tcx>, - per_local: &mut impl FnMut(Local) -> bool, - rvalue: &Rvalue<'tcx>, - ) -> bool { - match *rvalue { - Rvalue::NullaryOp(..) => false, - - Rvalue::Discriminant(ref place) | Rvalue::Len(ref place) => { - Self::in_place(cx, per_local, place.as_ref()) - } - - Rvalue::Use(ref operand) - | Rvalue::Repeat(ref operand, _) - | Rvalue::UnaryOp(_, ref operand) - | Rvalue::Cast(_, ref operand, _) => Self::in_operand(cx, per_local, operand), - - Rvalue::BinaryOp(_, ref lhs, ref rhs) - | Rvalue::CheckedBinaryOp(_, ref lhs, ref rhs) => { - Self::in_operand(cx, per_local, lhs) || Self::in_operand(cx, per_local, rhs) - } - - Rvalue::Ref(_, _, ref place) | Rvalue::AddressOf(_, ref place) => { - // Special-case reborrows to be more like a copy of the reference. - if let [proj_base @ .., ProjectionElem::Deref] = place.projection.as_ref() { - let base_ty = Place::ty_from(place.local, proj_base, *cx.body, cx.tcx).ty; - if let ty::Ref(..) = base_ty.kind { - return Self::in_place( - cx, - per_local, - PlaceRef { local: place.local, projection: proj_base }, - ); - } - } - - Self::in_place(cx, per_local, place.as_ref()) - } - - Rvalue::Aggregate(_, ref operands) => { - operands.iter().any(|o| Self::in_operand(cx, per_local, o)) - } - } - } - - fn in_rvalue( - cx: &ConstCx<'_, 'tcx>, - per_local: &mut impl FnMut(Local) -> bool, - rvalue: &Rvalue<'tcx>, - ) -> bool { - Self::in_rvalue_structurally(cx, per_local, rvalue) - } - - fn in_call( - cx: &ConstCx<'_, 'tcx>, - _per_local: &mut impl FnMut(Local) -> bool, - _callee: &Operand<'tcx>, - _args: &[Operand<'tcx>], - return_ty: Ty<'tcx>, - ) -> bool { - // Be conservative about the returned value of a const fn. - Self::in_any_value_of_ty(cx, return_ty) - } + /// Returns `true` if *any* value of the given type could possibly have this `Qualif`. + /// + /// This function determines `Qualif`s when we cannot do a value-based analysis. Since qualif + /// propagation is context-insenstive, this includes function arguments and values returned + /// from a call to another function. + /// + /// It also determines the `Qualif`s for primitive types. + fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> bool; + + /// Returns `true` if this `Qualif` is inherent to the given struct or enum. + /// + /// By default, `Qualif`s propagate into ADTs in a structural way: An ADT only becomes + /// qualified if part of it is assigned a value with that `Qualif`. However, some ADTs *always* + /// have a certain `Qualif`, regardless of whether their fields have it. For example, a type + /// with a custom `Drop` impl is inherently `NeedsDrop`. + /// + /// Returning `true` for `in_adt_inherently` but `false` for `in_any_value_of_ty` is unsound. + fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &AdtDef) -> bool; } /// Constant containing interior mutability (`UnsafeCell`). @@ -197,26 +74,10 @@ impl Qualif for HasMutInterior { !ty.is_freeze(cx.tcx, cx.param_env, DUMMY_SP) } - fn in_rvalue( - cx: &ConstCx<'_, 'tcx>, - per_local: &mut impl FnMut(Local) -> bool, - rvalue: &Rvalue<'tcx>, - ) -> bool { - match *rvalue { - Rvalue::Aggregate(ref kind, _) => { - if let AggregateKind::Adt(def, ..) = **kind { - if Some(def.did) == cx.tcx.lang_items().unsafe_cell_type() { - let ty = rvalue.ty(*cx.body, cx.tcx); - assert_eq!(Self::in_any_value_of_ty(cx, ty), true); - return true; - } - } - } - - _ => {} - } - - Self::in_rvalue_structurally(cx, per_local, rvalue) + fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &AdtDef) -> bool { + // Exactly one type, `UnsafeCell`, has the `HasMutInterior` qualif inherently. + // It arises structurally for all other types. + Some(adt.did) == cx.tcx.lang_items().unsafe_cell_type() } } @@ -238,19 +99,127 @@ impl Qualif for NeedsDrop { ty.needs_drop(cx.tcx, cx.param_env) } - fn in_rvalue( - cx: &ConstCx<'_, 'tcx>, - per_local: &mut impl FnMut(Local) -> bool, - rvalue: &Rvalue<'tcx>, - ) -> bool { - if let Rvalue::Aggregate(ref kind, _) = *rvalue { + fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &AdtDef) -> bool { + adt.has_dtor(cx.tcx) + } +} + +// FIXME: Use `mir::visit::Visitor` for the `in_*` functions if/when it supports early return. + +/// Returns `true` if this `Rvalue` contains qualif `Q`. +pub fn in_rvalue(cx: &ConstCx<'_, 'tcx>, in_local: &mut F, rvalue: &Rvalue<'tcx>) -> bool +where + Q: Qualif, + F: FnMut(Local) -> bool, +{ + match rvalue { + Rvalue::NullaryOp(..) => Q::in_any_value_of_ty(cx, rvalue.ty(*cx.body, cx.tcx)), + + Rvalue::Discriminant(place) | Rvalue::Len(place) => { + in_place::(cx, in_local, place.as_ref()) + } + + Rvalue::Use(operand) + | Rvalue::Repeat(operand, _) + | Rvalue::UnaryOp(_, operand) + | Rvalue::Cast(_, operand, _) => in_operand::(cx, in_local, operand), + + Rvalue::BinaryOp(_, lhs, rhs) | Rvalue::CheckedBinaryOp(_, lhs, rhs) => { + in_operand::(cx, in_local, lhs) || in_operand::(cx, in_local, rhs) + } + + Rvalue::Ref(_, _, place) | Rvalue::AddressOf(_, place) => { + // Special-case reborrows to be more like a copy of the reference. + if let &[ref proj_base @ .., ProjectionElem::Deref] = place.projection.as_ref() { + let base_ty = Place::ty_from(place.local, proj_base, *cx.body, cx.tcx).ty; + if let ty::Ref(..) = base_ty.kind { + return in_place::( + cx, + in_local, + PlaceRef { local: place.local, projection: proj_base }, + ); + } + } + + in_place::(cx, in_local, place.as_ref()) + } + + Rvalue::Aggregate(kind, operands) => { + // Return early if we know that the struct or enum being constructed is always + // qualified. if let AggregateKind::Adt(def, ..) = **kind { - if def.has_dtor(cx.tcx) { + if Q::in_adt_inherently(cx, def) { return true; } } + + // Otherwise, proceed structurally... + operands.iter().any(|o| in_operand::(cx, in_local, o)) } + } +} - Self::in_rvalue_structurally(cx, per_local, rvalue) +/// Returns `true` if this `Place` contains qualif `Q`. +pub fn in_place(cx: &ConstCx<'_, 'tcx>, in_local: &mut F, place: PlaceRef<'tcx>) -> bool +where + Q: Qualif, + F: FnMut(Local) -> bool, +{ + let mut projection = place.projection; + while let [ref proj_base @ .., proj_elem] = projection { + match *proj_elem { + ProjectionElem::Index(index) if in_local(index) => return true, + + ProjectionElem::Deref + | ProjectionElem::Field(_, _) + | ProjectionElem::ConstantIndex { .. } + | ProjectionElem::Subslice { .. } + | ProjectionElem::Downcast(_, _) + | ProjectionElem::Index(_) => {} + } + + let base_ty = Place::ty_from(place.local, proj_base, *cx.body, cx.tcx); + let proj_ty = base_ty.projection_ty(cx.tcx, proj_elem).ty; + if !Q::in_any_value_of_ty(cx, proj_ty) { + return false; + } + + projection = proj_base; + } + + assert!(projection.is_empty()); + in_local(place.local) +} + +/// Returns `true` if this `Operand` contains qualif `Q`. +pub fn in_operand(cx: &ConstCx<'_, 'tcx>, in_local: &mut F, operand: &Operand<'tcx>) -> bool +where + Q: Qualif, + F: FnMut(Local) -> bool, +{ + let constant = match operand { + Operand::Copy(place) | Operand::Move(place) => { + return in_place::(cx, in_local, place.as_ref()); + } + + Operand::Constant(c) => c, + }; + + // Check the qualifs of the value of `const` items. + if let ty::ConstKind::Unevaluated(def_id, _, promoted) = constant.literal.val { + assert!(promoted.is_none()); + // Don't peek inside trait associated constants. + if cx.tcx.trait_of_item(def_id).is_none() { + let qualifs = cx.tcx.at(constant.span).mir_const_qualif(def_id); + if !Q::in_qualifs(&qualifs) { + return false; + } + + // Just in case the type is more specific than + // the definition, e.g., impl associated const + // with type parameters, take it into account. + } } + // Otherwise use the qualifs of the type. + Q::in_any_value_of_ty(cx, constant.literal.ty) } diff --git a/src/librustc_mir/transform/check_consts/resolver.rs b/src/librustc_mir/transform/check_consts/resolver.rs index 3e14cc6d32a67..e42f64b5c7384 100644 --- a/src/librustc_mir/transform/check_consts/resolver.rs +++ b/src/librustc_mir/transform/check_consts/resolver.rs @@ -8,7 +8,7 @@ use rustc_index::bit_set::BitSet; use std::marker::PhantomData; -use super::{Item, Qualif}; +use super::{qualifs, Item, Qualif}; use crate::dataflow::{self as old_dataflow, generic as dataflow}; /// A `Visitor` that propagates qualifs between locals. This defines the transfer function of @@ -66,18 +66,15 @@ where fn apply_call_return_effect( &mut self, _block: BasicBlock, - func: &mir::Operand<'tcx>, - args: &[mir::Operand<'tcx>], + _func: &mir::Operand<'tcx>, + _args: &[mir::Operand<'tcx>], return_place: &mir::Place<'tcx>, ) { + // We cannot reason about another function's internals, so use conservative type-based + // qualification for the result of a function call. let return_ty = return_place.ty(*self.item.body, self.item.tcx).ty; - let qualif = Q::in_call( - self.item, - &mut |l| self.qualifs_per_local.contains(l), - func, - args, - return_ty, - ); + let qualif = Q::in_any_value_of_ty(self.item, return_ty); + if !return_place.is_indirect() { self.assign_qualif_direct(return_place, qualif); } @@ -110,7 +107,11 @@ where rvalue: &mir::Rvalue<'tcx>, location: Location, ) { - let qualif = Q::in_rvalue(self.item, &mut |l| self.qualifs_per_local.contains(l), rvalue); + let qualif = qualifs::in_rvalue::( + self.item, + &mut |l| self.qualifs_per_local.contains(l), + rvalue, + ); if !place.is_indirect() { self.assign_qualif_direct(place, qualif); } @@ -125,8 +126,12 @@ where // here; that occurs in `apply_call_return_effect`. if let mir::TerminatorKind::DropAndReplace { value, location: dest, .. } = kind { - let qualif = - Q::in_operand(self.item, &mut |l| self.qualifs_per_local.contains(l), value); + let qualif = qualifs::in_operand::( + self.item, + &mut |l| self.qualifs_per_local.contains(l), + value, + ); + if !dest.is_indirect() { self.assign_qualif_direct(dest, qualif); } diff --git a/src/librustc_mir/transform/check_consts/validation.rs b/src/librustc_mir/transform/check_consts/validation.rs index adffd444eb68b..48367fc24654d 100644 --- a/src/librustc_mir/transform/check_consts/validation.rs +++ b/src/librustc_mir/transform/check_consts/validation.rs @@ -344,7 +344,7 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> { Rvalue::Ref(_, BorrowKind::Shared, ref place) | Rvalue::Ref(_, BorrowKind::Shallow, ref place) | Rvalue::AddressOf(Mutability::Not, ref place) => { - let borrowed_place_has_mut_interior = HasMutInterior::in_place( + let borrowed_place_has_mut_interior = qualifs::in_place::( &self.item, &mut |local| self.qualifs.has_mut_interior(local, location), place.as_ref(), diff --git a/src/librustc_mir/transform/promote_consts.rs b/src/librustc_mir/transform/promote_consts.rs index 7dd134a35d909..1336206e18626 100644 --- a/src/librustc_mir/transform/promote_consts.rs +++ b/src/librustc_mir/transform/promote_consts.rs @@ -407,15 +407,17 @@ impl<'tcx> Validator<'_, 'tcx> { // FIXME(eddyb) maybe cache this? fn qualif_local(&self, local: Local) -> bool { - let per_local = &mut |l| self.qualif_local::(l); - if let TempState::Defined { location: loc, .. } = self.temps[local] { let num_stmts = self.body[loc.block].statements.len(); if loc.statement_index < num_stmts { let statement = &self.body[loc.block].statements[loc.statement_index]; match &statement.kind { - StatementKind::Assign(box (_, rhs)) => Q::in_rvalue(&self.item, per_local, rhs), + StatementKind::Assign(box (_, rhs)) => qualifs::in_rvalue::( + &self.item, + &mut |l| self.qualif_local::(l), + rhs, + ), _ => { span_bug!( statement.source_info.span, @@ -427,9 +429,9 @@ impl<'tcx> Validator<'_, 'tcx> { } else { let terminator = self.body[loc.block].terminator(); match &terminator.kind { - TerminatorKind::Call { func, args, .. } => { + TerminatorKind::Call { .. } => { let return_ty = self.body.local_decls[local].ty; - Q::in_call(&self.item, per_local, func, args, return_ty) + Q::in_any_value_of_ty(&self.item, return_ty) } kind => { span_bug!(terminator.source_info.span, "{:?} not promotable", kind);