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

Add basic support for dyn and impl Trait #226

Merged
merged 15 commits into from
Oct 14, 2019

Conversation

KiChjang
Copy link
Member

@KiChjang KiChjang commented May 8, 2019

cc #218.

@nikomatsakis nikomatsakis self-assigned this May 28, 2019
@nikomatsakis
Copy link
Contributor

@KiChjang so I didn't actually see your WIP commit until just now. I'm taking a look now at the issue and trying to figure out what the next steps are. Will try to leave you some advice =)

@KiChjang
Copy link
Member Author

@nikomatsakis I still need some guidance on how to extend the chalk solver to account for impl Trait and dyn Trait.

@KiChjang KiChjang force-pushed the dyn-and-impl-trait branch 2 times, most recently from 4dce951 to 4c04ed1 Compare September 23, 2019 22:45
match trait_ref.self_type_parameter() {
Some(Ty::Opaque(qwc)) | Some(Ty::Dyn(qwc)) => {
clauses.push(ProgramClauseImplication {
consequence: trait_ref.clone().cast(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't quite look right. For example, if this function were invoked with a trait_ref like Implemented(dyn Foo: Bar), this would generate something like

implemented(dyn Foo: Bar) :-
    dyn Foo: Foo

but what we really want is

Implemented(dyn Foo: Foo).

Moreover, something is a bit funny here -- you're just skipping over the binders in qwc, which isn't really right. What we want is more like:

let self_ty = trait_ref.self_type_parameter().unwrap(); // this should not really return an Option
let wc = qwc.instantiate(self_ty);
clauses.extend(wc.iter().cloned().casted());

what this will do is to take a self_ty like dyn Foo and create clauses like Implemented(dyn Foo: Foo).

However, there are some complications.

For one, the instantiated method I referred to doesn't actually exist yet -- there is subst::apply(&[self_ty], &qwc.value), which I think would be equivalent. The idea of instantiate would be to take a Binders<T> and create a T, substituting a value for the variables bound in Binders.

@KiChjang KiChjang force-pushed the dyn-and-impl-trait branch 2 times, most recently from 48b0a36 to 0a53a25 Compare October 1, 2019 00:43
@nikomatsakis
Copy link
Contributor

Ok I took a stab at building this branch locally and I see errors in the following files. Some notes:


error[E0004]: non-exhaustive patterns: `&Dyn(_)` and `&Opaque(_)` not covered
  --> chalk-solve/src/clauses/env_elaborator.rs:49:15
   |
49 |         match ty {
   |               ^^ patterns `&Dyn(_)` and `&Opaque(_)` not covered
   |
   = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

These have to do with implied bounds. I would leave these rules as empty but add "FIXME #203" for now.


error[E0004]: non-exhaustive patterns: `(&Dyn(_), &Apply(_))`, `(&Opaque(_), &Apply(_))`, `(&Dyn(_), &Dyn(_))` and 9 more not covered
   --> chalk-solve/src/infer/unify.rs:107:15
    |
107 |         match (a, b) {
    |               ^^^^^^ patterns `(&Dyn(_), &Apply(_))`, `(&Opaque(_), &Apply(_))`, `(&Dyn(_), &Dyn(_))` and 9 more not covered
    |
    = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

You basically want rules very analogous to the Apply rules. Although, hmm, it occurs to me that we're going to have some issues around sorting the where clauses in the types -- but let's just assume they're in a canonical ordering for now, we can patch that up later. (i.e., for now we'd say that dyn (Foo + Bar) and dyn (Bar + Foo) are distinct types -- later we'd assume some sort of normalization, so this code wouldn't change in any case).

So you want to add an analogous match arm like this one:

(&Ty::Apply(ref apply1), &Ty::Apply(ref apply2)) => {
if apply1.name != apply2.name {
return Err(NoSolution);
}
Zip::zip_with(self, &apply1.parameters, &apply2.parameters)
}

and then add Dyn/Opaque to these match arms:

(&Ty::InferenceVar(var), ty @ &Ty::Apply(_))
| (ty @ &Ty::Apply(_), &Ty::InferenceVar(var))
| (&Ty::InferenceVar(var), ty @ &Ty::ForAll(_))
| (ty @ &Ty::ForAll(_), &Ty::InferenceVar(var)) => self.unify_var_ty(var, ty),

(ty @ &Ty::Apply(_), &Ty::Projection(ref proj))
| (ty @ &Ty::ForAll(_), &Ty::Projection(ref proj))
| (ty @ &Ty::InferenceVar(_), &Ty::Projection(ref proj))
| (&Ty::Projection(ref proj), ty @ &Ty::Projection(_))
| (&Ty::Projection(ref proj), ty @ &Ty::Apply(_))
| (&Ty::Projection(ref proj), ty @ &Ty::ForAll(_))
| (&Ty::Projection(ref proj), ty @ &Ty::InferenceVar(_)) => {
self.unify_projection_ty(proj, ty)
}


error[E0004]: non-exhaustive patterns: `(&Dyn(_), _)` and `(&Opaque(_), _)` not covered
   --> chalk-solve/src/solve/slg/aggregate.rs:171:15
    |
171 |         match (ty0, ty1) {
    |               ^^^^^^^^^^ patterns `(&Dyn(_), _)` and `(&Opaque(_), _)` not covered
    |
    = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

For now we could probably handle "de-aggregation" in the same "not that great" way we handle some other cases, like so:

(Ty::BoundVar(_), Ty::BoundVar(_)) | (Ty::ForAll(_), Ty::ForAll(_)) => {
self.new_variable()
}

but we should add a FIXME for this. (Also, this code seems a bit wrong to me, in that I think it should try to ensure that aggregate_tys(foo, foo) also returns foo, but oh well, that's a separate bug, or maybe I'm missing something.)


error[E0004]: non-exhaustive patterns: `(&Dyn(_), &Apply(_))`, `(&Opaque(_), &Apply(_))`, `(&Dyn(_), &Dyn(_))` and 9 more not covered
   --> chalk-solve/src/solve/slg/resolvent.rs:342:15
    |
342 |         match (answer, pending) {
    |               ^^^^^^^^^^^^^^^^^ patterns `(&Dyn(_), &Apply(_))`, `(&Opaque(_), &Apply(_))`, `(&Dyn(_), &Dyn(_))` and 9 more not covered
    |
    = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

As the comment says:

// Otherwise, the answer and the selected subgoal ought to be a perfect match for
// one another.

we ought to be able to match up the structure and descend down, so we probably just want something like this

(Ty::Apply(answer), Ty::Apply(pending)) => Zip::zip_with(self, answer, pending),

but we have to be sure the Zip trait is implemented.


error[E0004]: non-exhaustive patterns: `&Dyn(_)` and `&Opaque(_)` not covered
  --> chalk-solve/src/wf.rs:68:15
   |
68 |         match self {
   |               ^^^^ patterns `&Dyn(_)` and `&Opaque(_)` not covered
   |
   = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

I think this should just behave analogously to apply: so push the type into the accumulate and recurse on the patterns, much like this

Ty::Apply(app) => {
accumulator.push(self.clone());
app.parameters.fold(accumulator);
}

@KiChjang KiChjang force-pushed the dyn-and-impl-trait branch from 0a53a25 to b8a7470 Compare October 4, 2019 21:15
@nikomatsakis nikomatsakis marked this pull request as ready for review October 14, 2019 10:05
@nikomatsakis nikomatsakis merged commit 186da37 into rust-lang:master Oct 14, 2019
@KiChjang KiChjang deleted the dyn-and-impl-trait branch October 14, 2019 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants