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

broken interaction between marker traits, lifetimes, and caching #102360

Closed
Tracked by #29864
lcnr opened this issue Sep 27, 2022 · 1 comment · Fixed by #102472
Closed
Tracked by #29864

broken interaction between marker traits, lifetimes, and caching #102360

lcnr opened this issue Sep 27, 2022 · 1 comment · Fixed by #102472
Labels
A-trait-system Area: Trait system C-bug Category: This is a bug. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Sep 27, 2022

let cache_fresh_trait_pred = self.infcx.freshen(stack.obligation.predicate);

erases all lifetimes for caching, including 'static.

freshener: infcx.freshener_keep_static(),

let fresh_trait_pred = obligation.predicate.fold_with(&mut self.freshener);

// If we erased any lifetimes, then we want to use
// `EvaluatedToOkModuloRegions` instead of `EvaluatedToOk`
// as your final result. The result will be cached using
// the freshened trait predicate as a key, so we need
// our result to be correct by *any* choice of original lifetimes,
// not just the lifetime choice for this particular (non-erased)
// predicate.
// See issue #80691
if stack.fresh_trait_pred.has_erased_regions() {
result = result.max(EvaluatedToOkModuloRegions);
}

changes the evaluation result depending on whether there are any erased regions in the predicate, excluding 'static.

if other.evaluation.must_apply_considering_regions() {

selection only drops allowed-to-overlap candidate if the other resulted in EvaluatedToOk.

This means that depending on the order of evaluation, marker traits can either succeed or result in ambiguity

@lcnr lcnr added C-bug Category: This is a bug. A-trait-system Area: Trait system labels Sep 27, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Sep 27, 2022

this has been found in #102205 where changing caching to also keep 'static caused this test to fail:

#![allow(order_dependent_trait_objects)]
trait Trait {}

impl Trait for dyn Send + Sync {}
impl Trait for dyn Sync + Send {}
fn assert_trait<T: Trait + ?Sized>() {}

fn main() {
    // compiles right now as we first evaluate `dyn Send + Sync + 'static`,
    // but store `dyn Send + Sync + 'erased` in the cache.
    assert_trait::<dyn Send + Sync>();
}

@compiler-errors compiler-errors added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Sep 27, 2022
@bors bors closed this as completed in 4bd33fd Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system C-bug Category: This is a bug. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants