chalkify: Implement lowering rule Implied-Bound-From-Trait #49435

tmandry · 2018-03-28T03:44:27Z

TODO:

Implement where clauses besides trait and projection predicates
Is the output of the lower_trait_higher_rank test correct?
Remove Self::Trait from the query tcx.predicates_of(<trait_id>).predicates
Consider moving tests to compile-fail to make them more manageable

tmandry · 2018-03-28T03:46:12Z

scalexm · 2018-03-28T08:17:26Z

src/librustc_traits/lowering.rs

@@ -58,6 +58,8 @@ impl<'tcx> Lower<DomainGoal<'tcx>> for ty::TypeOutlivesPredicate<'tcx> {
    }
 }

+/// Lowers a trait or projection predicate to a `FromEnv(...)` clause.


What does this comment refer to actually?

Nothing, I added it by accident!

scalexm · 2018-03-28T08:29:31Z

src/librustc_traits/lowering.rs

@@ -99,6 +101,38 @@ impl<'tcx> Lower<Goal<'tcx>> for ty::Predicate<'tcx> {
    }
 }

+/// For a given `where_clause`, returns a clause `FromEnv(WC) :- Implemented(Self: Trait<P1..Pn>)`.


So actually the rule we want is FromEnv(WC) :- FromEnv(Self: Trait<P1..Pn>). But there is a slight subtlety: the FromEnv in the left hand side is not exactly a real FromEnv predicate, it's rather some notation (maybe not very appropriate, I agree):

if WC == TraitRef, then indeed it means FromEnv(TraitRef)

if WC == ProjectionPredicate, then again it means FromEnv(ProjectionPredicate)

in any other case, WC is left unchanged, e.g. in the above FromEnv(T: 'a) just means T: 'a
see this section in the book.

As an example, given this trait:

trait Foo<'a, T> where T: Copy, T: 'a, T: Iterator<Item = i32> { }

we would like the following clauses:

forall<'a, Self, T> { FromEnv(T: Copy) :- FromEnv(Self: Foo<'a, T>). FromEnv(T: Iterator<Item = i32>) :- FromEnv(Self: Foo<'a, T>). (T: 'a) :- FromEnv(Self: Foo<'a, T>). }

Oops, I really don't know how I mixed that up >.>

Doing the rest in a match clause really ought to be a matter of using Lower IMO, but right now it eagerly lowers straight to a Goal when DomainGoal is needed. I want to play around with this when I get some time.

Yeah, I think I'm going to issue a fix for the eager lowering soon, it should benefit the ergonomics of your PR

scalexm · 2018-03-28T08:40:38Z

src/librustc_traits/lowering.rs

@@ -99,6 +101,38 @@ impl<'tcx> Lower<Goal<'tcx>> for ty::Predicate<'tcx> {
    }
 }

+/// For a given `where_clause`, returns a clause `FromEnv(WC) :- Implemented(Self: Trait<P1..Pn>)`.


As an aside, to avoid writing again code about binders, I'd rather see something like:

impl<'tcx> DomainGoal<'tcx> { fn into_from_env_goal(self) -> DomainGoal<'tcx> { match self { DomainGoal::Holds(wca) => DomainGoal::FromEnv(wca), other => other // unchanged } } }

then the same thing recursively for Goal:

impl<'tcx> Goal<'tcx> { fn into_from_env_goal(self) -> Goal<'tcx> { match self { Goal::DomainGoal(dg) => Goal::DomainGoal(dg.into_from_env_goal()), Goal::Quantified(q, g) => Goal::Quantified(q, g.into_from_env_goal()), /* ... */ } } }

and then you can just do something like:

let predicates = tcx.predicates_of(def_id).predicates.lower(); /* do appropriate things to remove the `Self: Trait` predicate */ let predicates = predicates.into_iter().map(|p| p.into_from_env_goal());

Actually I was mistaken this would not quite work since we only can have DomainGoal as a consequence. Ok so do as you want 😀

scalexm · 2018-03-28T08:41:39Z

src/librustc_traits/lowering.rs

+    // forall<Self, P1..Pn> {
+    //   FromEnv(WC) :- FromEnv(Self: Trait<P1..Pn)
+    // }
+


You might want to keep this comment just above your code which generates the clauses just below.

scalexm · 2018-03-28T08:44:19Z

src/test/ui/chalkify/lower_trait_higher_rank.rs

+#[rustc_dump_program_clauses] //~ ERROR Implemented(Self: Foo<F>) :-
+                              //~| ERROR FromEnv
+                              //~| ERROR forall
+                              //~| ERROR forall


The output looks good, just need to change to FromEnv(...) :- FromEnv(Self: Foo<F>) instead of FromEnv(...) :- Implemented(Self: Foo<F>) 😃

tmandry · 2018-03-31T05:53:33Z

Rebased on #49497 and it definitely makes things nicer! 🏂

Note that the test output for HRTBs changed.

nikomatsakis · 2018-04-04T20:19:55Z

Blocked on #49497

scalexm · 2018-04-04T20:21:17Z

I’d like to review one more time once #49497 lands.

nikomatsakis

Few nits -- seems roughly right? I've been doing reviews all day though so my "attention to detail" is starting to flag. I'll try and take another look tomorrow.

nikomatsakis · 2018-04-04T20:20:27Z

src/librustc_traits/lowering.rs

+    where_clause: &ty::Predicate<'tcx>,
+) -> Clause<'tcx> {
+    // We can't use the full `Lower` chain here, as it'll wrap our where clause in `Holds` instead
+    // of `FromEnv`. We also handle ty::Binder similarly to `Lower`, but at the Clause level.


I don't understand this comment actually

Neither do I, honestly. I was tired when I wrote this.

nikomatsakis · 2018-04-04T20:22:05Z

src/librustc_traits/lowering.rs

+            Trait(binder) => binder.lower_from_env(),
+            Projection(binder) => binder.lower_from_env(),
+            RegionOutlives(p) => p.lower(),
+            TypeOutlives(p) => p.lower(),


Can we make where_clause implement LowerFromEnv? So we can just do where_clause.lower_from_env()?

I was going to suggest, instead of the LowerFromEnv machinery:

trait IntoFromEnvGoal { fn into_from_env_goal(self) -> Self; } impl<'tcx> IntoFromEnvGoal for DomainGoal<'tcx> { fn into_from_env_goal(self) -> DomainGoal<'tcx> { /* ... */ } }

and then a straightforward:

ProgramClause::ForAll( // we might map a dummy binder here, ok for now where_clause.lower().map_bound(|goal| ProgramClause { goal: goal.into_from_env_goal(), hypotheses }) )

Yep, IntoFromEnvGoal turns out to be much simpler. Thanks!

tmandry · 2018-04-06T18:26:20Z

Changed implementation to use IntoFromEnvGoal as suggested by @scalexm, rebased, and squashed. Also, added some Outlives predicates to the test.

This should be ready, modulo the predicates_of query, which I can do as part of this PR or separately. I'll be working on that this afternoon.

nikomatsakis · 2018-04-09T17:53:45Z

@bors r+

bors · 2018-04-09T17:53:46Z

📌 Commit 7ac35ea has been approved by nikomatsakis

bors · 2018-04-10T00:33:01Z

⌛ Testing commit 7ac35ea with merge a8a8d6b...

…atsakis chalkify: Implement lowering rule Implied-Bound-From-Trait For #49177. TODO: - [x] Implement where clauses besides trait and projection predicates - [x] Is the output of the `lower_trait_higher_rank` test correct? - [ ] Remove `Self::Trait` from the query `tcx.predicates_of(<trait_id>).predicates` - [ ] Consider moving tests to compile-fail to make them more manageable

bors · 2018-04-10T03:27:40Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a8a8d6b to master...

rust-highfive assigned nikomatsakis Mar 28, 2018

scalexm suggested changes Mar 28, 2018

View reviewed changes

nikomatsakis mentioned this pull request Mar 28, 2018

🛠 Extend the "chalk lowering" pass with remaining rules #49177

Closed

11 tasks

shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2018

tmandry added WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 A-trait-system Area: Trait system labels Mar 30, 2018

tmandry force-pushed the rule-implied-bound-from-trait branch from 5c67da7 to 287a1ac Compare March 31, 2018 05:49

tmandry force-pushed the rule-implied-bound-from-trait branch from 287a1ac to 4ef87c6 Compare March 31, 2018 15:50

nikomatsakis added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2018

nikomatsakis requested changes Apr 4, 2018

View reviewed changes

chalkify: Implement Rule Implied-Bound-From-Trait

7ac35ea

tmandry force-pushed the rule-implied-bound-from-trait branch from 4ef87c6 to 7ac35ea Compare April 6, 2018 18:22

tmandry changed the title ~~[WIP] chalkify: Implement lowering rule Implied-Bound-From-Trait~~ chalkify: Implement lowering rule Implied-Bound-From-Trait Apr 6, 2018

scalexm approved these changes Apr 6, 2018

View reviewed changes

nikomatsakis approved these changes Apr 9, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 9, 2018

bors merged commit 7ac35ea into rust-lang:master Apr 10, 2018

bors mentioned this pull request Apr 10, 2018

traits: Implement interning for Goal and Clause #49800

Merged

tmandry deleted the rule-implied-bound-from-trait branch April 14, 2018 03:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chalkify: Implement lowering rule Implied-Bound-From-Trait #49435

chalkify: Implement lowering rule Implied-Bound-From-Trait #49435

tmandry commented Mar 28, 2018 •

edited

Loading

tmandry commented Mar 28, 2018

scalexm Mar 28, 2018

tmandry Mar 28, 2018

scalexm Mar 28, 2018

tmandry Mar 28, 2018

scalexm Mar 28, 2018

scalexm Mar 28, 2018

scalexm Mar 28, 2018

scalexm Mar 28, 2018

scalexm Mar 28, 2018

tmandry commented Mar 31, 2018 •

edited

Loading

nikomatsakis commented Apr 4, 2018

scalexm commented Apr 4, 2018

nikomatsakis left a comment

nikomatsakis Apr 4, 2018

tmandry Apr 6, 2018

nikomatsakis Apr 4, 2018

scalexm Apr 4, 2018 •

edited

Loading

tmandry Apr 6, 2018

tmandry commented Apr 6, 2018

nikomatsakis commented Apr 9, 2018

bors commented Apr 9, 2018

bors commented Apr 10, 2018

bors commented Apr 10, 2018

chalkify: Implement lowering rule Implied-Bound-From-Trait #49435

chalkify: Implement lowering rule Implied-Bound-From-Trait #49435

Conversation

tmandry commented Mar 28, 2018 • edited Loading

tmandry commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmandry commented Mar 31, 2018 • edited Loading

nikomatsakis commented Apr 4, 2018

scalexm commented Apr 4, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scalexm Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmandry commented Apr 6, 2018

nikomatsakis commented Apr 9, 2018

bors commented Apr 9, 2018

bors commented Apr 10, 2018

bors commented Apr 10, 2018

tmandry commented Mar 28, 2018 •

edited

Loading

tmandry commented Mar 31, 2018 •

edited

Loading

scalexm Apr 4, 2018 •

edited

Loading