From 25a8b5d58e3899084e191ffd9456f39d29c3263b Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Fri, 27 Dec 2019 11:44:36 -0500 Subject: [PATCH] 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 e6caa146a627f..c593a51e457b8 100644 --- a/src/librustc/mir/interpret/queries.rs +++ b/src/librustc/mir/interpret/queries.rs @@ -18,7 +18,7 @@ impl<'tcx> TyCtxt<'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 90381895f0a33..bcb012ea51494 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 faa83ceaddea2..cfd1779c080ec 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 745b6aabfa6bb..6c4b69d9d767e 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 6dd3c0f80da24..8c380589151e8 100644 --- a/src/librustc_mir/hair/pattern/mod.rs +++ b/src/librustc_mir/hair/pattern/mod.rs @@ -742,7 +742,13 @@ impl<'a, 'tcx> PatCtxt<'a, 'tcx> { 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 a6b30ab5e68cf..62717daed1642 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -33,6 +33,7 @@ use crate::interpret::{ 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 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { 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 aa0c71ff0f176..98cd341770965 100644 --- a/src/librustc_mir/transform/inline.rs +++ b/src/librustc_mir/transform/inline.rs @@ -8,8 +8,8 @@ use rustc_index::vec::{Idx, IndexVec}; 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 @@ impl Inliner<'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 0000000000000..9591019bb4f70 --- /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 0000000000000..8010d2fe1aee9 --- /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 dc4050e44abb1..db535b53fcf37 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 |