-
Notifications
You must be signed in to change notification settings - Fork 14k
chalkify: Implement lowering rule Implied-Bound-From-Trait #49435
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
Conversation
src/librustc_traits/lowering.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this comment refer to actually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing, I added it by accident!
src/librustc_traits/lowering.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 meansFromEnv(TraitRef) - if
WC == ProjectionPredicate, then again it meansFromEnv(ProjectionPredicate) - in any other case,
WCis left unchanged, e.g. in the aboveFromEnv(T: 'a)just meansT: '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>).
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I'm going to issue a fix for the eager lowering soon, it should benefit the ergonomics of your PR
src/librustc_traits/lowering.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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());There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was mistaken this would not quite work since we only can have DomainGoal as a consequence. Ok so do as you want 😀
src/librustc_traits/lowering.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to keep this comment just above your code which generates the clauses just below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output looks good, just need to change to FromEnv(...) :- FromEnv(Self: Foo<F>) instead of FromEnv(...) :- Implemented(Self: Foo<F>) 😃
5c67da7 to
287a1ac
Compare
|
Rebased on #49497 and it definitely makes things nicer! 🏂 Note that the test output for HRTBs changed. |
287a1ac to
4ef87c6
Compare
|
Blocked on #49497 |
|
I’d like to review one more time once #49497 lands. |
nikomatsakis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/librustc_traits/lowering.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment actually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither do I, honestly. I was tired when I wrote this.
src/librustc_traits/lowering.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make where_clause implement LowerFromEnv? So we can just do where_clause.lower_from_env()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
})
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, IntoFromEnvGoal turns out to be much simpler. Thanks!
4ef87c6 to
7ac35ea
Compare
|
Changed implementation to use This should be ready, modulo the |
|
@bors r+ |
|
📌 Commit 7ac35ea has been approved by |
…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
|
☀️ Test successful - status-appveyor, status-travis |
For #49177.
TODO:
lower_trait_higher_ranktest correct?Self::Traitfrom the querytcx.predicates_of(<trait_id>).predicates