Skip to content
New issue

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

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

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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