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

Trait selection caching does not handle subtyping properly #30225

Closed
arielb1 opened this issue Dec 5, 2015 · 6 comments
Closed

Trait selection caching does not handle subtyping properly #30225

arielb1 opened this issue Dec 5, 2015 · 6 comments
Assignees
Labels
A-trait-system Area: Trait system E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@arielb1
Copy link
Contributor

arielb1 commented Dec 5, 2015

STR

trait Foo<U,V> : Sized {
    fn foo(self, u: Option<U>, v: Option<V>) {}
}

struct A;
struct B;

impl Foo<A, B> for () {}      // impl A

fn toxic() {
    // cache the resolution <() as Foo<$0,$1>> = impl A
    let u = None;
    let v = None;
    Foo::foo((), u, v);
}

fn bomb() {
    let mut u = None; // type is Option<$0>
    let mut v = None; // type is Option<$1>
    let mut x = None; // type is Option<$2>

    Foo::foo(x.unwrap(),u,v); // register <$2 as Foo<$0, $1>>
    u = v; // mark $0 and $1 in a subtype relationship
    x = Some(()); // set $2 = (), allowing impl selection
                  // to proceed for <() as Foo<$0, $1>> = impl A.
                  // kaboom
                  // NOTE: the obligation's resolution must be delayed for the
                  // ICE to occur to prevent coercions from interfering.
}

fn main() {}

Results

error: internal compiler error: Impl DefId { krate: 0, node: DefIndex(15) => ().Foo<A, B> } was matchable against Obligation(predicate=Binder(TraitPredicate(<_ as Foo<_, _>>)),depth=0) but now is not
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
note: run with `RUST_BACKTRACE=1` for a backtrace
thread 'rustc' panicked at 'Box<Any>', ../src/libsyntax/diagnostic.rs:253

Comments

@nikomatsakis and me were worried that this might be possible since we had multidispatch, but this is the first working example. I found this while poking with the unification machinery.

@arielb1 arielb1 added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ A-trait-system Area: Trait system I-nominated labels Dec 5, 2015
@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 16, 2015
@nikomatsakis
Copy link
Contributor

Yeah, I've been waiting for someone to make an example for this. I had a strategy for fixing it but never got around to implementing it. Thanks for making an example @arielb1 !

@nikomatsakis
Copy link
Contributor

triage: P-medium

@nikomatsakis nikomatsakis self-assigned this Dec 17, 2015
@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Dec 17, 2015
@brson brson added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Aug 4, 2016
@brson
Copy link
Contributor

brson commented Aug 4, 2016

@nikomatsakis can mentor.

@cramertj
Copy link
Member

@nikomatsakis I'd be interested in working on this if you're still willing to mentor.

@nikomatsakis
Copy link
Contributor

@cramertj hmm, at this point I think the way I want to solve this is as part of a bigger refactoring to overhaul trait system. I recently wrote the very first post of a series of posts that I hope to write explaining the approach; I need to think about the actual steps for making that transition. Would you maybe be interested in helping with that task?

@cramertj
Copy link
Member

@nikomatsakis I would absolutely be interested in helping!

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 12, 2017
bors added a commit that referenced this issue Apr 13, 2017
…ion, r=arielb1

Handle subtyping in inference through obligations

We currently store subtyping relations in the `TypeVariables` structure as a kind of special case. This branch uses normal obligations to propagate subtyping, thus converting our inference variables into normal fallback. It also does a few other things:

- Removes the (unstable, outdated) support for custom type inference fallback.
    - It's not clear how we want this to work, but we know that we don't want it to work the way it currently does.
    - The existing support was also just getting in my way.
- Fixes #30225, which was caused by the trait caching code pretending type variables were normal unification variables, when indeed they were not (but now are).

There is one fishy part of these changes: when computing the LUB/GLB of a "bivariant" type parameter, I currently return the `a` value. Bivariant type parameters are only allowed in a very particular situation, where the type parameter is only used as an associated type output, like this:

```rust
pub struct Foo<A, B>
    where A: Fn() -> B
{
    data: A
}
```

In principle, if one had `T=Foo<A, &'a u32>` and `U=Foo<A, &'b u32>` and (e.g.) `A: for<'a> Fn() -> &'a u32`, then I think that computing the LUB of `T` and `U` might do the wrong thing. Probably the right behavior is just to create a fresh type variable. However, that particular example would not compile (because the where-clause is illegal; `'a` does not appear in any input type). I was not able to make an example that *would* compile and demonstrate this shortcoming, and handling the LUB/GLB was mildly inconvenient, so I left it as is. I am considering whether to revisit this or what.

I have started a crater run to test the impact of these changes.
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 E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants