Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specialization does not work correctly during constant evaluation. #66901

Closed
Lymia opened this issue Nov 30, 2019 · 2 comments · Fixed by #67662
Closed

Specialization does not work correctly during constant evaluation. #66901

Lymia opened this issue Nov 30, 2019 · 2 comments · Fixed by #67662
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-specialization Area: Trait impl specialization C-bug Category: This is a bug. F-specialization `#![feature(specialization)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Lymia
Copy link
Contributor

Lymia commented Nov 30, 2019

A crate using specialization I've been working on seems to have broken sometime between nightly-2019-11-19 and nightly-2019-11-20. After a bit of debugging, it seems the issue is that specialization doesn't always work correctly in const contexts.

This code, for example, returns the following:

const_bool::<TypeA>: in default impl
const_bool::<TypeB>: in default impl
const_str::<TypeA>: in specialized impl
const_str::<TypeB>: in default impl
run_method::<TypeA>: in specialized impl
run_method::<TypeB>: in default impl

On earlier versions, this would have returned the following:

const_bool::<TypeA>: in specialized impl
const_bool::<TypeB>: in default impl
const_str::<TypeA>: in specialized impl
const_str::<TypeB>: in default impl
run_method::<TypeA>: in specialized impl
run_method::<TypeB>: in default impl

This bug does not happen when the -Z mir-opt-level=0 flag is set, and it behaves the same as prior versions.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 30, 2019

cc @wesleywiser const prop is evaluating polymorphic code and evaluating to a default instead of bailing out due to being too generic

@eddyb
Copy link
Member

eddyb commented Nov 30, 2019

I'm guessing Instance::resolve is missing this bit of logic (NB "polymorphic MIR optimizations"):

// Only reveal a specializable default if we're past type-checking
// and the obligations is monomorphic, otherwise passes such as
// transmute checking and polymorphic MIR optimizations could
// get a result which isn't correct for all monomorphizations.
if !is_default {
true
} else if obligation.param_env.reveal == Reveal::All {
debug_assert!(!poly_trait_ref.needs_infer());
if !poly_trait_ref.needs_subst() {
true
} else {
false
}
} else {
false
}

EDIT: that is, this should perform the same checks to pick Some or None:

traits::VtableImpl(impl_data) => {
let (def_id, substs) = traits::find_associated_item(
tcx, param_env, trait_item, rcvr_substs, &impl_data);
let substs = tcx.erase_regions(&substs);
Some(ty::Instance::new(def_id, substs))
}

@jonas-schievink jonas-schievink added A-specialization Area: Trait impl specialization C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-eval Area: constant evaluation (mir interpretation) F-specialization `#![feature(specialization)]` requires-nightly This issue requires a nightly compiler in some way. labels Nov 30, 2019
@bors bors closed this as completed in 25a8b5d Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) A-specialization Area: Trait impl specialization C-bug Category: This is a bug. F-specialization `#![feature(specialization)]` requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants