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

Implement parsing and conversion of generalized where clauses #20002

Merged
merged 2 commits into from
Dec 20, 2014

Conversation

jroesch
Copy link
Member

@jroesch jroesch commented Dec 19, 2014

Going to tentatively open a PR before I move on to other things tonight. I think everything should be good minus a failing test case for region bounds (gonna push that now, i.e 'a : 'b). r? @nikomatsakis

Fixes #20023.

@nikomatsakis
Copy link
Contributor

I'll take another read through, but what is here looks pretty good to me.

There are several follow-on points though. These could be addressed in separate PRs. I will create issues for these points and attach them to #17657.

  1. According to the RFC, where clauses that do not involve parameters (e.g., where int : Eq) are supposed to be illegal, but we can always open an issue to check for those.

  2. We'll want to modify the code that checks that impls are consistent with traits. I think that it only looks at the bounds of the TypeParameterDef. We should modify it to just call to_bounds() on the Generics and examine the resulting predicate sets. We may want to change it to just create its own trait fulfillment context and check that, given the parameter environment derived from the trait, we can derive all the requirements of the impl. This could also be something we address in a followup PR.

  3. The last point gives rise to a curious case where the impl (which has more concrete types than the trait) actually knows that some methods can never be invoked. We never did clarify what to do here. I am presently leaning towards allowing impl methods to be declared with no body, which corresponds to a case that we can statically show does not occur. Thus fn foo(); in an impl would be equivalent to fn foo() { panic!() } (and that is how we would codegen it), except that we statically check that the where clauses from the trait, if substituted with the type parameters of the impl, cannot be satisfied.

  4. I think we will need to modify the parser to permit for qualifiers to appear in more places. In particular, i'd like to be able to write something like this:

    where for<'a, 'b> &'a T : Foo<&'b T>

but today the parser only accepts for after the :. the rest of the code already considers 'a and 'b to be in scope for the self-type of the trait, so it's just the parser that has to be changed. I imagine that the for, in that case, would distribute over all the attached where clauses, so:

where for<'a> T : Foo<&'a T> + Bar<&'a T>

would be equivalent to:

where T : for<'a> Foo&'a T> + for<'a> Bar<&'a T>

Points 3 and 4 probably require an RFC.

@brson brson mentioned this pull request Dec 19, 2014
7 tasks
@nikomatsakis
Copy link
Contributor

I've created issues for each of those points in #17657

@@ -1459,6 +1459,8 @@ impl<'a, 'tcx, 'v> Visitor<'v> for VisiblePrivateTypesVisitor<'a, 'tcx> {
self.check_ty_param_bound(bound_pred.span, bound)
}
}
// ? for Niko: Anything to do here?
Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is fine, I don't see how privacy impacts anything here.

@nikomatsakis
Copy link
Contributor

OK, I went through the commits and offered commits. It's also probably a good idea to cleanup the history a bit, though I'm not a stickler about this (but the intermediate commits don't seem... especially useful here; also, don't worry about preserving authorship on the commit that came from me). I'd like to see some more tests. Here are two I wrote to test the code, they seem to work (which is awesome):

https://github.com/nikomatsakis/rust/tree/jroesch-generalized-where-clause-parser/

It'd be great to see also tests that add builtin bounds locally, since they are treated rather differently. In particular Copy and Sized. I think though that Sized still has some parts of the code that use the bounds lists rather than checking the parameter environment. I was working on refactoring those. So you may encounter that those tests don't work, in which case we should just follow another follow-up issue (and I should figure out why my branch wasn't bootstrapping...)

@nikomatsakis
Copy link
Contributor

OK, ping me on IRC if there are any questions.

@jroesch
Copy link
Member Author

jroesch commented Dec 20, 2014

I just rebased, and squashed them into a single commit. Hopefully I have addressed all the comments, and will begin chasing down some of the follow up issues.

@jroesch jroesch force-pushed the generalized-where-clause-parser branch 2 times, most recently from cec2295 to 5e86648 Compare December 20, 2014 03:45
@nikomatsakis
Copy link
Contributor

As discussed on IRC, r+ with one additional test that shows a where clause with a generic trait, so that substitution is required.

@jroesch jroesch force-pushed the generalized-where-clause-parser branch 3 times, most recently from b1e1e77 to f999f0d Compare December 20, 2014 04:02
@jroesch
Copy link
Member Author

jroesch commented Dec 20, 2014

Tests should be in an acceptable state now. Should be good to go 👍.

@nikomatsakis
Copy link
Contributor

Giving p=1 because this enables work on stdlib

@jroesch jroesch force-pushed the generalized-where-clause-parser branch 2 times, most recently from cee12ab to f4ed79d Compare December 20, 2014 10:32
@jroesch jroesch force-pushed the generalized-where-clause-parser branch from f4ed79d to 2436f79 Compare December 20, 2014 10:44
Implement support in the parser for generalized where clauses,
as well as the conversion of ast::WherePredicates to
ty::Predicate in `collect.rs`.
@jroesch jroesch force-pushed the generalized-where-clause-parser branch from bb6a77b to bdd61f8 Compare December 20, 2014 10:48
@jroesch jroesch force-pushed the generalized-where-clause-parser branch from bdd61f8 to d87b308 Compare December 20, 2014 11:54
bors added a commit that referenced this pull request Dec 20, 2014
…ser, r=nikomatsakis

This is the same branch as #20002 but with the pretty-printing test fixed.
@bors bors merged commit d87b308 into rust-lang:master Dec 20, 2014
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.

Parse and accept outlives expressions in where clauses
3 participants