Separate projection bounds and predicates#73905
Merged
bors merged 34 commits intorust-lang:masterfrom Oct 6, 2020
Merged
Conversation
rust-highfive
added
the
S-waiting-on-review
Contributor
Author
|
@bors try @rust-timer queue |
Collaborator
|
Awaiting bors try build completion |
Collaborator
|
⌛ Trying commit b90c0fc3abf884ef96b717f49991f6b226e1598b with merge b037eadcbfea58d037fe042f10297285ed9fac05... |
Contributor
|
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 |
Collaborator
|
☀️ Try build successful - checks-actions, checks-azure |
Collaborator
|
Queued b037eadcbfea58d037fe042f10297285ed9fac05 with parent 16957bd, future comparison URL. |
Collaborator
|
Finished benchmarking try commit (b037eadcbfea58d037fe042f10297285ed9fac05): comparison url. |
Contributor
Author
|
@craterbot check |
Collaborator
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
Contributor
Author
|
Actually, it looks like some of the of the perf crates failed to build, I'll cancel if that's the case locally. |
Contributor
Author
|
@craterbot cancel |
Contributor
Author
|
@craterbot abort |
Collaborator
|
🗑️ Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-review
nikomatsakis
reviewed
Jul 1, 2020
Contributor
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
Generally 👍, left some comments/thoughts
src/librustc_middle/query/mod.rs
Outdated
Contributor
There was a problem hiding this comment.
Nit: not grammatical, and a bit "closed mouthed". An example would be good. Is this accurate?
Elaborated version of the predicates from explicit_item_bounds.
Example: if the explicit bounds are T: Eq, then this would return [T: Eq, T: PartialEq]
Does this also include bounds from parent items? We should say so.
Contributor
There was a problem hiding this comment.
(I think the answer is "no", does not include bounds from parent items.)
src/librustc_typeck/collect.rs
Outdated
Contributor
There was a problem hiding this comment.
Is this something you plan to do as part of this PR series? (Or maybe I'll find that in a future commit you already did so...)
src/librustc_infer/traits/util.rs
Outdated
Contributor
There was a problem hiding this comment.
well that looks like it was a dubious helper =)
Contributor
There was a problem hiding this comment.
Pretty sure I'm the culprit 👋
Edit: Yep: #69745
Contributor
There was a problem hiding this comment.
Pre-existing, I guess, but translate_predicate_substs injects impl_ty_value, can we add an assertion (perhaps in that function) that impl_ty_value has no late-bound-regions, and the same for rebased_substs? Otherwise, doing this substitution inside the binder is not safe.
Contributor
There was a problem hiding this comment.
But also -- I feel like we should be able to do this with just an ordinary substitution. Why can't we?
Like, we're taking the bound declared in the trait -- which might be like <Self as Foo>::Bar<'a>: PartialEq<B> or something, and we're converting it to the impl, right? So why can't we just apply the rebased_subst and be done with it?
Contributor
There was a problem hiding this comment.
So given <T as Iterator>::Item, we are no longer checking that T: Iterator is WF, is that correct? say a bit more about what's going on here?
Contributor
There was a problem hiding this comment.
Pre-existing, but can we add a comment indicating what this function does?
I believe what it does is to
- look for an obligation like
<T as Foo>::Bar: Baz, where the self-type is a projection or opaque type - look at the bounds declared on
Barto see if any of them matchBaz - if there is one, it returns the
<T as Foo>::Bar: Bazbound declared within the trait or on the opaque type
src/librustc_typeck/check/mod.rs
Outdated
Contributor
There was a problem hiding this comment.
Can you elaborate a bit on the motivation here? Is this just a redundant check? i.e., each place that specifies the type of the opaque type is also doing these checks, right?
Contributor
There was a problem hiding this comment.
heh I've been meaning to change that "cause" info for years...
src/test/incremental/issue-54242.rs
Outdated
Contributor
Author
There was a problem hiding this comment.
So, the cycle is broken with this PR because bounds don't need to be proven to prove that the trait is well-formed. This makes the code work like it does currently.
matthewjasper
force-pushed
the
projection-bounds-2
branch
2 times, most recently
from
a23032d to
9d2c078
Compare
Contributor
Author
|
So this PR as is currently breaks the following: trait Op where Self::Output: Copy {
type Output;
}
// This now requires an explicit `where T::Output: Copy `
// like any other kind of where clause on the trait, except super traits.
fn f<T: Op>() {}Since this apparently never comes up in rustc/perf/the tests I'm leaving it in for the crater run to satisfy my curiosity about whether anyone is writing bounds like this. I'll probably fix it even if crater can't find anyone doing this. |
Contributor
I was going to ask about that...thanks for reminding me. I too am curious how much it matters. |
Contributor
|
FWIW pub trait Component
where
Self: Sized + 'static,
Self::ModelMsg: Clone,
Self::ViewMsg: Clone,
Self::DomNode: JsCast + AsRef<Node> + Clone, |
Collaborator
|
☔ The latest upstream changes (presumably #74019) made this pull request unmergeable. Please resolve the merge conflicts. |
matthewjasper
force-pushed
the
projection-bounds-2
branch
from
9d2c078 to
c879e01
Compare
Contributor
Author
|
@bors try @rust-timer queue |
Collaborator
|
Awaiting bors try build completion |
This was referenced Dec 11, 2020
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
19 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up to #72788.
projection_predicatestoitem_bounds:intype X: ...) and opaque types (the things afterimpl) from predicates.feature(generic_associated_types)no longer changes how we handle bounds (ICE when adding GAT feature in combination with async/await #73816)Opening for a perf and crater runs.
r? @nikomatsakis