-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Chalk lowering rule: WellFormed-TraitRef #50250
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors delegate=scalexm Just giving scalexm ability to r+ too |
✌️ @scalexm can now approve this pull request |
cc @csmoe -- I guess you know this doesn't build yet. =) Do you need help w/ that? |
@nikomatsakis |
637d034
to
c87d2d1
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
✌️ @scalexm can now approve this pull request |
src/librustc_traits/lowering.rs
Outdated
/// Used for lowered where clauses (see rustc guide). | ||
trait IntoFromEnvGoal { | ||
trait IntoGoal { |
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.
Nit: could we have two traits IntoFromEnvGoal
and IntoWellFormedGoal
instead of just one? I find the name IntoGoal
not very informative.
src/librustc_traits/lowering.rs
Outdated
fn into_from_env_goal(self) -> Self; | ||
// Transforms an existing goal into a WellFormed goal. | ||
fn into_wellformed_goal(self) -> Self; |
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.
Very nit: well_formed
is how we snake-casify WellFormed
in chalk, so I'd like into_well_formed_goal
for consistency :)
src/librustc_traits/lowering.rs
Outdated
|
||
// WellFormed(Self: Trait<P1..Pn>) | ||
let wellformed_trait = DomainGoal::WellFormed(WhereClauseAtom::Implemented(trait_pred)); | ||
// Impemented(Self: Trait<P1..Pn>) |
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.
Nit: Impemented
-> Implemented
src/librustc_traits/lowering.rs
Outdated
// For each where clause WC: | ||
// forall<Self, P1..Pn> { | ||
// WellFormed(Self: Trait<P1..Pn>) :- Implemented(Self: Trait<P1..Pn>) && WellFormed(WC) | ||
// } |
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 the guide is a bit misleading here, WC
should represent the full set of where clauses e.g. you have WC := WC1, WC2, ..., WCn
and we want to output only one clause which is:
WellFormed(Self: Trait<P1..Pn>) :-
Implemented(Self: Trait<P1..Pn>) && WC1 && ... && WCn
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 you should change your code logic a bit)
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.
Implemented(Self: Trait<P1..Pn>) && WC1 && ... && WCn
^^^ = should this be WellFormed(WC1)?
Since the wellformed guide says:
T: Trait is well-formed if (a) T: Trait is implemented and (b) all the where-clauses declared on Trait are well-formed
Thus do you mean this(merge all the WellFormed(WC_1...n)
inside one clause instead of split them like current test/ui output below)?
Currently(copy from ui test output)
= note: WellFormed(Self: Foo<'a, 'b, S, T, U>) :- Implemented(Self: Foo<'a, 'b, S, T, U>), RegionOutlives('a : 'b).
= note: WellFormed(Self: Foo<'a, 'b, S, T, U>) :- Implemented(Self: Foo<'a, 'b, S, T, U>), TypeOutlives(U : 'b).
= note: WellFormed(Self: Foo<'a, 'b, S, T, U>) :- Implemented(Self: Foo<'a, 'b, S, T, U>), WellFormed(S: std::fmt::Debug).
= note: WellFormed(Self: Foo<'a, 'b, S, T, U>) :- Implemented(Self: Foo<'a, 'b, S, T, U>), WellFormed(S: std::marker::Sized).
= note: WellFormed(Self: Foo<'a, 'b, S, T, U>) :- Implemented(Self: Foo<'a, 'b, S, T, U>), WellFormed(T: std::borrow::Borrow<U>).
= note: WellFormed(Self: Foo<'a, 'b, S, T, U>) :- Implemented(Self: Foo<'a, 'b, S, T, U>), WellFormed(T: std::marker::Sized).
After=>
= note: WellFormed(Self: Foo<'a, 'b, S, T, U>) :- Implemented(...), ..., WellFormed(WC1) , ..., WellFormed(WCn).
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.
Yes absolutely, merge all into one clause (and yes you’re right about WellFormed(WC1), ..., WellFormed(WCn)
☔ The latest upstream changes (presumably #50200) made this pull request unmergeable. Please resolve the merge conflicts. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@nikomatsakis more commits soon. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors r+ |
📌 Commit 37c5c0b has been approved by |
Chalk lowering rule: WellFormed-TraitRef Address chalk lowering "Implemented-From-Env" as part of #49177. r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
Address chalk lowering "Implemented-From-Env" as part of #49177.
r? @nikomatsakis