From 25a8b5d58e3899084e191ffd9456f39d29c3263b Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Fri, 27 Dec 2019 11:44:36 -0500 Subject: [PATCH 1/4] Fix `Instance::resolve()` incorrectly returning specialized instances We only want to return specializations when `Reveal::All` is passed, not when `Reveal::UserFacing` is. Resolving this fixes several issues with the `ConstProp`, `SimplifyBranches`, and `Inline` MIR optimization passes. Fixes #66901 --- src/librustc/mir/interpret/queries.rs | 2 +- src/librustc/traits/project.rs | 3 + src/librustc/ty/instance.rs | 19 ++++++ src/librustc_mir/const_eval/eval_queries.rs | 22 ++++--- src/librustc_mir/hair/pattern/mod.rs | 8 ++- src/librustc_mir/transform/const_prop.rs | 14 +++- src/librustc_mir/transform/inline.rs | 13 +++- .../mir-opt/inline/inline-specialization.rs | 48 ++++++++++++++ src/test/ui/consts/trait_specialization.rs | 65 +++++++++++++++++++ .../self-in-enum-definition.stderr | 5 ++ 10 files changed, 182 insertions(+), 17 deletions(-) create mode 100644 src/test/mir-opt/inline/inline-specialization.rs create mode 100644 src/test/ui/consts/trait_specialization.rs diff --git a/src/librustc/mir/interpret/queries.rs b/src/librustc/mir/interpret/queries.rs index e6caa146a62..c593a51e457 100644 --- a/src/librustc/mir/interpret/queries.rs +++ b/src/librustc/mir/interpret/queries.rs @@ -18,7 +18,7 @@ pub fn const_eval_poly(self, def_id: DefId) -> ConstEvalResult<'tcx> { let substs = InternalSubsts::identity_for_item(self, def_id); let instance = ty::Instance::new(def_id, substs); let cid = GlobalId { instance, promoted: None }; - let param_env = self.param_env(def_id); + let param_env = self.param_env(def_id).with_reveal_all(); self.const_eval_validated(param_env.and(cid)) } diff --git a/src/librustc/traits/project.rs b/src/librustc/traits/project.rs index 90381895f0a..bcb012ea514 100644 --- a/src/librustc/traits/project.rs +++ b/src/librustc/traits/project.rs @@ -1028,6 +1028,9 @@ fn assemble_candidates_from_impls<'cx, 'tcx>( // In either case, we handle this by not adding a // candidate for an impl if it contains a `default` // type. + // + // NOTE: This should be kept in sync with the similar code in + // `rustc::ty::instance::resolve_associated_item()`. let node_item = assoc_ty_def(selcx, impl_data.impl_def_id, obligation.predicate.item_def_id); diff --git a/src/librustc/ty/instance.rs b/src/librustc/ty/instance.rs index faa83ceadde..cfd1779c080 100644 --- a/src/librustc/ty/instance.rs +++ b/src/librustc/ty/instance.rs @@ -346,6 +346,25 @@ fn resolve_associated_item<'tcx>( traits::VtableImpl(impl_data) => { let (def_id, substs) = traits::find_associated_item(tcx, param_env, trait_item, rcvr_substs, &impl_data); + + let resolved_item = tcx.associated_item(def_id); + + // Since this is a trait item, we need to see if the item is either a trait default item + // or a specialization because we can't resolve those unless we can `Reveal::All`. + // NOTE: This should be kept in sync with the similar code in + // `rustc::traits::project::assemble_candidates_from_impls()`. + let eligible = if !resolved_item.defaultness.is_default() { + true + } else if param_env.reveal == traits::Reveal::All { + !trait_ref.needs_subst() + } else { + false + }; + + if !eligible { + return None; + } + let substs = tcx.erase_regions(&substs); Some(ty::Instance::new(def_id, substs)) } diff --git a/src/librustc_mir/const_eval/eval_queries.rs b/src/librustc_mir/const_eval/eval_queries.rs index 745b6aabfa6..6c4b69d9d76 100644 --- a/src/librustc_mir/const_eval/eval_queries.rs +++ b/src/librustc_mir/const_eval/eval_queries.rs @@ -212,11 +212,7 @@ pub fn const_eval_validated_provider<'tcx>( key.param_env.reveal = Reveal::UserFacing; match tcx.const_eval_validated(key) { // try again with reveal all as requested - Err(ErrorHandled::TooGeneric) => { - // Promoteds should never be "too generic" when getting evaluated. - // They either don't get evaluated, or we are in a monomorphic context - assert!(key.value.promoted.is_none()); - } + Err(ErrorHandled::TooGeneric) => {} // dedupliate calls other => return other, } @@ -301,10 +297,18 @@ pub fn const_eval_raw_provider<'tcx>( // Ensure that if the above error was either `TooGeneric` or `Reported` // an error must be reported. let v = err.report_as_error(ecx.tcx, "could not evaluate static initializer"); - tcx.sess.delay_span_bug( - err.span, - &format!("static eval failure did not emit an error: {:#?}", v), - ); + + // If this is `Reveal:All`, then we need to make sure an error is reported but if + // this is `Reveal::UserFacing`, then it's expected that we could get a + // `TooGeneric` error. When we fall back to `Reveal::All`, then it will either + // succeed or we'll report this error then. + if key.param_env.reveal == Reveal::All { + tcx.sess.delay_span_bug( + err.span, + &format!("static eval failure did not emit an error: {:#?}", v), + ); + } + v } else if def_id.is_local() { // constant defined in this crate, we can figure out a lint level! diff --git a/src/librustc_mir/hair/pattern/mod.rs b/src/librustc_mir/hair/pattern/mod.rs index 6dd3c0f80da..8c380589151 100644 --- a/src/librustc_mir/hair/pattern/mod.rs +++ b/src/librustc_mir/hair/pattern/mod.rs @@ -742,7 +742,13 @@ fn lower_path(&mut self, qpath: &hir::QPath, id: hir::HirId, span: Span) -> Pat< let kind = match res { Res::Def(DefKind::Const, def_id) | Res::Def(DefKind::AssocConst, def_id) => { let substs = self.tables.node_substs(id); - match self.tcx.const_eval_resolve(self.param_env, def_id, substs, Some(span)) { + // Use `Reveal::All` here because patterns are always monomorphic even if their function isn't. + match self.tcx.const_eval_resolve( + self.param_env.with_reveal_all(), + def_id, + substs, + Some(span), + ) { Ok(value) => { let pattern = self.const_to_pat(value, id, span); if !is_associated_const { diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index a6b30ab5e68..62717daed16 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -33,6 +33,7 @@ ScalarMaybeUndef, StackPopCleanup, }; use crate::rustc::ty::subst::Subst; +use crate::rustc::ty::TypeFoldable; use crate::transform::{MirPass, MirSource}; /// The maximum number of bytes that we'll allocate space for a return value. @@ -293,13 +294,20 @@ fn new( source: MirSource<'tcx>, ) -> ConstPropagator<'mir, 'tcx> { let def_id = source.def_id(); - let param_env = tcx.param_env(def_id); + let substs = &InternalSubsts::identity_for_item(tcx, def_id); + let mut param_env = tcx.param_env(def_id); + + // If we're evaluating inside a monomorphic function, then use `Reveal::All` because + // we want to see the same instances that codegen will see. This allows us to `resolve()` + // specializations. + if !substs.needs_subst() { + param_env = param_env.with_reveal_all(); + } + let span = tcx.def_span(def_id); let mut ecx = InterpCx::new(tcx.at(span), param_env, ConstPropMachine, ()); let can_const_prop = CanConstProp::check(body); - let substs = &InternalSubsts::identity_for_item(tcx, def_id); - let ret = ecx .layout_of(body.return_ty().subst(tcx, substs)) .ok() diff --git a/src/librustc_mir/transform/inline.rs b/src/librustc_mir/transform/inline.rs index aa0c71ff0f1..98cd3417709 100644 --- a/src/librustc_mir/transform/inline.rs +++ b/src/librustc_mir/transform/inline.rs @@ -8,8 +8,8 @@ use rustc::mir::visit::*; use rustc::mir::*; -use rustc::ty::subst::{Subst, SubstsRef}; -use rustc::ty::{self, Instance, InstanceDef, ParamEnv, Ty, TyCtxt}; +use rustc::ty::subst::{InternalSubsts, Subst, SubstsRef}; +use rustc::ty::{self, Instance, InstanceDef, ParamEnv, Ty, TyCtxt, TypeFoldable}; use super::simplify::{remove_dead_blocks, CfgSimplifier}; use crate::transform::{MirPass, MirSource}; @@ -66,7 +66,14 @@ fn run_pass(&self, caller_body: &mut BodyAndCache<'tcx>) { let mut callsites = VecDeque::new(); - let param_env = self.tcx.param_env(self.source.def_id()); + let mut param_env = self.tcx.param_env(self.source.def_id()); + + let substs = &InternalSubsts::identity_for_item(self.tcx, self.source.def_id()); + + // For monomorphic functions, we can use `Reveal::All` to resolve specialized instances. + if !substs.needs_subst() { + param_env = param_env.with_reveal_all(); + } // Only do inlining into fn bodies. let id = self.tcx.hir().as_local_hir_id(self.source.def_id()).unwrap(); diff --git a/src/test/mir-opt/inline/inline-specialization.rs b/src/test/mir-opt/inline/inline-specialization.rs new file mode 100644 index 00000000000..9591019bb4f --- /dev/null +++ b/src/test/mir-opt/inline/inline-specialization.rs @@ -0,0 +1,48 @@ +#![feature(specialization)] + +fn main() { + let x = as Foo>::bar(); +} + +trait Foo { + fn bar() -> u32; +} + +impl Foo for Vec { + #[inline(always)] + default fn bar() -> u32 { 123 } +} + +// END RUST SOURCE +// START rustc.main.Inline.before.mir +// let mut _0: (); +// let _1: u32; +// scope 1 { +// debug x => _1; +// } +// bb0: { +// StorageLive(_1); +// _1 = const as Foo>::bar() -> bb1; +// } +// bb1: { +// _0 = (); +// StorageDead(_1); +// return; +// } +// END rustc.main.Inline.before.mir +// START rustc.main.Inline.after.mir +// let mut _0: (); +// let _1: u32; +// scope 1 { +// debug x => _1; +// } +// scope 2 { +// } +// bb0: { +// StorageLive(_1); +// _1 = const 123u32; +// _0 = (); +// StorageDead(_1); +// return; +// } +// END rustc.main.Inline.after.mir diff --git a/src/test/ui/consts/trait_specialization.rs b/src/test/ui/consts/trait_specialization.rs new file mode 100644 index 00000000000..8010d2fe1ae --- /dev/null +++ b/src/test/ui/consts/trait_specialization.rs @@ -0,0 +1,65 @@ +// ignore-wasm32-bare which doesn't support `std::process:exit()` +// compile-flags: -Zmir-opt-level=2 +// run-pass + +// Tests that specialization does not cause optimizations running on polymorphic MIR to resolve +// to a `default` implementation. + +#![feature(specialization)] + +trait Marker {} + +trait SpecializedTrait { + const CONST_BOOL: bool; + const CONST_STR: &'static str; + fn method() -> &'static str; +} +impl SpecializedTrait for T { + default const CONST_BOOL: bool = false; + default const CONST_STR: &'static str = "in default impl"; + #[inline(always)] + default fn method() -> &'static str { + "in default impl" + } +} +impl SpecializedTrait for T { + const CONST_BOOL: bool = true; + const CONST_STR: &'static str = "in specialized impl"; + fn method() -> &'static str { + "in specialized impl" + } +} + +fn const_bool() -> &'static str { + if ::CONST_BOOL { + "in specialized impl" + } else { + "in default impl" + } +} +fn const_str() -> &'static str { + ::CONST_STR +} +fn run_method() -> &'static str { + ::method() +} + +struct TypeA; +impl Marker for TypeA {} +struct TypeB; + +#[inline(never)] +fn exit_if_not_eq(left: &str, right: &str) { + if left != right { + std::process::exit(1); + } +} + +pub fn main() { + exit_if_not_eq("in specialized impl", const_bool::()); + exit_if_not_eq("in default impl", const_bool::()); + exit_if_not_eq("in specialized impl", const_str::()); + exit_if_not_eq("in default impl", const_str::()); + exit_if_not_eq("in specialized impl", run_method::()); + exit_if_not_eq("in default impl", run_method::()); +} diff --git a/src/test/ui/type-alias-enum-variants/self-in-enum-definition.stderr b/src/test/ui/type-alias-enum-variants/self-in-enum-definition.stderr index dc4050e44ab..db535b53fcf 100644 --- a/src/test/ui/type-alias-enum-variants/self-in-enum-definition.stderr +++ b/src/test/ui/type-alias-enum-variants/self-in-enum-definition.stderr @@ -4,6 +4,11 @@ error[E0391]: cycle detected when const-evaluating + checking `Alpha::V3::{{cons LL | V3 = Self::V1 {} as u8 + 2, | ^^^^^^^^ | +note: ...which requires const-evaluating + checking `Alpha::V3::{{constant}}#0`... + --> $DIR/self-in-enum-definition.rs:5:10 + | +LL | V3 = Self::V1 {} as u8 + 2, + | ^^^^^^^^ note: ...which requires const-evaluating `Alpha::V3::{{constant}}#0`... --> $DIR/self-in-enum-definition.rs:5:10 | From 3eb0585173deb61ed0ed3e15fad360882d894568 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 26 Dec 2019 15:10:13 +0100 Subject: [PATCH 2/4] Work around a resolve bug in const prop --- src/librustc_mir/transform/const_prop.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 62717daed16..bf8e63ebe3d 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -20,7 +20,7 @@ HasDataLayout, HasTyCtxt, LayoutError, LayoutOf, Size, TargetDataLayout, TyLayout, }; use rustc::ty::subst::InternalSubsts; -use rustc::ty::{self, Instance, ParamEnv, Ty, TyCtxt}; +use rustc::ty::{self, Instance, ParamEnv, Ty, TyCtxt, TypeFoldable}; use rustc_data_structures::fx::FxHashMap; use rustc_index::vec::IndexVec; use syntax::ast::Mutability; @@ -418,6 +418,12 @@ fn use_ecx(&mut self, source_info: SourceInfo, f: F) -> Option } fn eval_constant(&mut self, c: &Constant<'tcx>) -> Option> { + // `eval_const_to_op` uses `Instance::resolve` which still has a bug (#66901) in the + // presence of trait items with a default body. So we just bail out if we aren't 100% + // monomorphic. + if c.literal.needs_subst() { + return None; + } self.ecx.tcx.span = c.span; match self.ecx.eval_const_to_op(c.literal, None) { Ok(op) => Some(op), From b3abebd78dcab1cadf7535271ff2ac6188b33cb8 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 26 Dec 2019 17:28:07 +0100 Subject: [PATCH 3/4] Prevent polymorphic const prop on assignments --- src/librustc_mir/transform/const_prop.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index bf8e63ebe3d..206f8043786 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -570,6 +570,13 @@ fn const_prop( _ => {} } + // `eval_rvalue_into_place` uses `Instance::resolve` for constants which still has a bug + // (#66901) in the presence of trait items with a default body. So we just bail out if we + // aren't 100% monomorphic. + if rvalue.needs_subst() { + return None; + } + self.use_ecx(source_info, |this| { trace!("calling eval_rvalue_into_place(rvalue = {:?}, place = {:?})", rvalue, place); this.ecx.eval_rvalue_into_place(rvalue, place)?; From 5fd8abd2278624dd7e3b08a46a5599850c81b40d Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Sun, 29 Dec 2019 00:26:25 +0100 Subject: [PATCH 4/4] Ensure that we don't cause *new* hard errors if we suddenly can evaluate more constants during const prop --- src/librustc_mir/transform/const_prop.rs | 70 ++++++++++++++---------- 1 file changed, 41 insertions(+), 29 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 206f8043786..c36f7935115 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -6,6 +6,7 @@ use rustc::hir::def::DefKind; use rustc::hir::def_id::DefId; +use rustc::hir::HirId; use rustc::mir::interpret::{InterpResult, PanicInfo, Scalar}; use rustc::mir::visit::{ MutVisitor, MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor, @@ -33,7 +34,6 @@ ScalarMaybeUndef, StackPopCleanup, }; use crate::rustc::ty::subst::Subst; -use crate::rustc::ty::TypeFoldable; use crate::transform::{MirPass, MirSource}; /// The maximum number of bytes that we'll allocate space for a return value. @@ -261,6 +261,9 @@ struct ConstPropagator<'mir, 'tcx> { source_scopes: IndexVec, local_decls: IndexVec>, ret: Option>, + // Because we have `MutVisitor` we can't obtain the `SourceInfo` from a `Location`. So we store + // the last known `SourceInfo` here and just keep revisiting it. + source_info: Option, } impl<'mir, 'tcx> LayoutOf for ConstPropagator<'mir, 'tcx> { @@ -339,6 +342,7 @@ fn new( //FIXME(wesleywiser) we can't steal this because `Visitor::super_visit_body()` needs it local_decls: body.local_decls.clone(), ret: ret.map(Into::into), + source_info: None, } } @@ -360,6 +364,13 @@ fn remove_const(&mut self, local: Local) { LocalState { value: LocalValue::Uninitialized, layout: Cell::new(None) }; } + fn lint_root(&self, source_info: SourceInfo) -> Option { + match &self.source_scopes[source_info.scope].local_data { + ClearCrossCrate::Set(data) => Some(data.lint_root), + ClearCrossCrate::Clear => None, + } + } + fn use_ecx(&mut self, source_info: SourceInfo, f: F) -> Option where F: FnOnce(&mut Self) -> InterpResult<'tcx, T>, @@ -368,10 +379,7 @@ fn use_ecx(&mut self, source_info: SourceInfo, f: F) -> Option // FIXME(eddyb) move this to the `Panic(_)` error case, so that // `f(self)` is always called, and that the only difference when the // scope's `local_data` is missing, is that the lint isn't emitted. - let lint_root = match &self.source_scopes[source_info.scope].local_data { - ClearCrossCrate::Set(data) => data.lint_root, - ClearCrossCrate::Clear => return None, - }; + let lint_root = self.lint_root(source_info)?; let r = match f(self) { Ok(val) => Some(val), Err(error) => { @@ -417,19 +425,31 @@ fn use_ecx(&mut self, source_info: SourceInfo, f: F) -> Option r } - fn eval_constant(&mut self, c: &Constant<'tcx>) -> Option> { - // `eval_const_to_op` uses `Instance::resolve` which still has a bug (#66901) in the - // presence of trait items with a default body. So we just bail out if we aren't 100% - // monomorphic. - if c.literal.needs_subst() { - return None; - } + fn eval_constant( + &mut self, + c: &Constant<'tcx>, + source_info: SourceInfo, + ) -> Option> { self.ecx.tcx.span = c.span; match self.ecx.eval_const_to_op(c.literal, None) { Ok(op) => Some(op), Err(error) => { let err = error_to_const_error(&self.ecx, error); - err.report_as_error(self.ecx.tcx, "erroneous constant used"); + match self.lint_root(source_info) { + Some(lint_root) if c.literal.needs_subst() => { + // Out of backwards compatibility we cannot report hard errors in unused + // generic functions using associated constants of the generic parameters. + err.report_as_lint( + self.ecx.tcx, + "erroneous constant used", + lint_root, + Some(c.span), + ); + } + _ => { + err.report_as_error(self.ecx.tcx, "erroneous constant used"); + } + } None } } @@ -442,7 +462,7 @@ fn eval_place(&mut self, place: &Place<'tcx>, source_info: SourceInfo) -> Option fn eval_operand(&mut self, op: &Operand<'tcx>, source_info: SourceInfo) -> Option> { match *op { - Operand::Constant(ref c) => self.eval_constant(c), + Operand::Constant(ref c) => self.eval_constant(c, source_info), Operand::Move(ref place) | Operand::Copy(ref place) => { self.eval_place(place, source_info) } @@ -509,10 +529,7 @@ fn const_prop( let right_size = r.layout.size; let r_bits = r.to_scalar().and_then(|r| r.to_bits(right_size)); if r_bits.map_or(false, |b| b >= left_bits as u128) { - let lint_root = match &self.source_scopes[source_info.scope].local_data { - ClearCrossCrate::Set(data) => data.lint_root, - ClearCrossCrate::Clear => return None, - }; + let lint_root = self.lint_root(source_info)?; let dir = if *op == BinOp::Shr { "right" } else { "left" }; self.tcx.lint_hir( ::rustc::lint::builtin::EXCEEDING_BITSHIFTS, @@ -570,13 +587,6 @@ fn const_prop( _ => {} } - // `eval_rvalue_into_place` uses `Instance::resolve` for constants which still has a bug - // (#66901) in the presence of trait items with a default body. So we just bail out if we - // aren't 100% monomorphic. - if rvalue.needs_subst() { - return None; - } - self.use_ecx(source_info, |this| { trace!("calling eval_rvalue_into_place(rvalue = {:?}, place = {:?})", rvalue, place); this.ecx.eval_rvalue_into_place(rvalue, place)?; @@ -769,18 +779,19 @@ fn tcx(&self) -> TyCtxt<'tcx> { fn visit_constant(&mut self, constant: &mut Constant<'tcx>, location: Location) { trace!("visit_constant: {:?}", constant); self.super_constant(constant, location); - self.eval_constant(constant); + self.eval_constant(constant, self.source_info.unwrap()); } fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Location) { trace!("visit_statement: {:?}", statement); + let source_info = statement.source_info; + self.source_info = Some(source_info); if let StatementKind::Assign(box (ref place, ref mut rval)) = statement.kind { let place_ty: Ty<'tcx> = place.ty(&self.local_decls, self.tcx).ty; if let Ok(place_layout) = self.tcx.layout_of(self.param_env.and(place_ty)) { if let Some(local) = place.as_local() { - let source = statement.source_info; let can_const_prop = self.can_const_prop[local]; - if let Some(()) = self.const_prop(rval, place_layout, source, place) { + if let Some(()) = self.const_prop(rval, place_layout, source_info, place) { if can_const_prop == ConstPropMode::FullConstProp || can_const_prop == ConstPropMode::OnlyPropagateInto { @@ -823,8 +834,9 @@ fn visit_statement(&mut self, statement: &mut Statement<'tcx>, location: Locatio } fn visit_terminator(&mut self, terminator: &mut Terminator<'tcx>, location: Location) { - self.super_terminator(terminator, location); let source_info = terminator.source_info; + self.source_info = Some(source_info); + self.super_terminator(terminator, location); match &mut terminator.kind { TerminatorKind::Assert { expected, ref msg, ref mut cond, .. } => { if let Some(value) = self.eval_operand(&cond, source_info) {