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

Fix bad caching of ~const Drop bounds #92149

Merged
merged 1 commit into from
Dec 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,15 @@ pub struct TraitPredicate<'tcx> {
pub type PolyTraitPredicate<'tcx> = ty::Binder<'tcx, TraitPredicate<'tcx>>;

impl<'tcx> TraitPredicate<'tcx> {
pub fn remap_constness(&mut self, tcx: TyCtxt<'tcx>, param_env: &mut ParamEnv<'tcx>) {
if unlikely!(Some(self.trait_ref.def_id) == tcx.lang_items().drop_trait()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems kind of weird to be special-casing Drop here. Are there any other traits that need this kind of special handling? Are you sure that this method is called in all of the places where it needs to be?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. No because we only need to special case ~const Drop bounds.
  2. Yes because I have audited all usages of a method I added made for exactly this purpose. Remapping T: ~const SomeTrait to T: SomeTrait when not in a const context.

See also https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/~const.20Drop which would be a better solution

// remap without changing constness of this predicate.
// this is because `T: ~const Drop` has a different meaning to `T: Drop`
param_env.remap_constness_with(self.constness)
} else {
*param_env = param_env.with_constness(self.constness.and(param_env.constness()))
}
}
pub fn def_id(self) -> DefId {
self.trait_ref.def_id
}
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
let mut param_env = obligation.param_env;

fresh_trait_pred = fresh_trait_pred.map_bound(|mut pred| {
param_env = param_env.with_constness(pred.constness.and(param_env.constness()));
pred.remap_constness(self.tcx(), &mut param_env);
pred
});

Expand Down Expand Up @@ -1269,7 +1269,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}
let tcx = self.tcx();
let mut pred = cache_fresh_trait_pred.skip_binder();
param_env = param_env.with_constness(pred.constness.and(param_env.constness()));
pred.remap_constness(tcx, &mut param_env);

if self.can_use_global_caches(param_env) {
if let Some(res) = tcx.selection_cache.get(&param_env.and(pred), tcx) {
Expand Down Expand Up @@ -1322,7 +1322,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
let tcx = self.tcx();
let mut pred = cache_fresh_trait_pred.skip_binder();

param_env = param_env.with_constness(pred.constness.and(param_env.constness()));
pred.remap_constness(tcx, &mut param_env);

if !self.can_cache_candidate(&candidate) {
debug!(?pred, ?candidate, "insert_candidate_cache - candidate is not cacheable");
Expand Down
29 changes: 29 additions & 0 deletions src/test/ui/rfc-2632-const-trait-impl/issue-92111.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Regression test for #92111.
//
// The issue was that we normalize trait bounds before caching
// results of selection. Checking that `impl NoDrop for S` requires

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// results of selection. Checking that `impl NoDrop for S` requires
// results of selection. Checking that `impl Tr for S` requires

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Though bors has already begun testing this PR. The fix would probably have to be separated to another PR.

// checking `S: !Drop` because it cannot overlap with the blanket
// impl. Then we save the (unsatisfied) result from checking `S: Drop`.
// Then the call to `a` checks whether `S: ~const Drop` but we normalize
// it to `S: Drop` which the cache claims to be unsatisfied.
//
// check-pass

#![feature(const_trait_impl)]
#![feature(const_fn_trait_bound)]

pub trait Tr {}

#[allow(drop_bounds)]
impl<T: Drop> Tr for T {}

#[derive(Debug)]
pub struct S(i32);

impl Tr for S {}

const fn a<T: ~const Drop>(t: T) {}

fn main() {
a(S(0));
}