Skip to content

Commit

Permalink
Fix Instance::resolve() incorrectly returning specialized instances
Browse files Browse the repository at this point in the history
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
  • Loading branch information
wesleywiser committed Dec 27, 2019
1 parent 41501a6 commit 25a8b5d
Show file tree
Hide file tree
Showing 10 changed files with 182 additions and 17 deletions.
2 changes: 1 addition & 1 deletion src/librustc/mir/interpret/queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}

Expand Down
3 changes: 3 additions & 0 deletions src/librustc/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
19 changes: 19 additions & 0 deletions src/librustc/ty/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
22 changes: 13 additions & 9 deletions src/librustc_mir/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand Down Expand Up @@ -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!
Expand Down
8 changes: 7 additions & 1 deletion src/librustc_mir/hair/pattern/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
14 changes: 11 additions & 3 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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()
Expand Down
13 changes: 10 additions & 3 deletions src/librustc_mir/transform/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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();
Expand Down
48 changes: 48 additions & 0 deletions src/test/mir-opt/inline/inline-specialization.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#![feature(specialization)]

fn main() {
let x = <Vec::<()> as Foo>::bar();
}

trait Foo {
fn bar() -> u32;
}

impl<T> Foo for Vec<T> {
#[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 <std::vec::Vec<()> 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
65 changes: 65 additions & 0 deletions src/test/ui/consts/trait_specialization.rs
Original file line number Diff line number Diff line change
@@ -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 <T> 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 <T: Marker> 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<T>() -> &'static str {
if <T as SpecializedTrait>::CONST_BOOL {
"in specialized impl"
} else {
"in default impl"
}
}
fn const_str<T>() -> &'static str {
<T as SpecializedTrait>::CONST_STR
}
fn run_method<T>() -> &'static str {
<T as SpecializedTrait>::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::<TypeA>());
exit_if_not_eq("in default impl", const_bool::<TypeB>());
exit_if_not_eq("in specialized impl", const_str::<TypeA>());
exit_if_not_eq("in default impl", const_str::<TypeB>());
exit_if_not_eq("in specialized impl", run_method::<TypeA>());
exit_if_not_eq("in default impl", run_method::<TypeB>());
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand Down

0 comments on commit 25a8b5d

Please sign in to comment.