Move self trait predicate to items #51895
Conversation
rust-highfive
added
the
S-waiting-on-review
|
It occurs to me that a less disruptive way to achieve the main goal of this PR would be to
(We would have to rework slightly how the Crucially, this would still mean that the rest of the compiler thinks that For background, I think that the way that @scalexm and I wanted things to work is that we add the |
|
@bors delegate=scalexm |
|
✌️ @scalexm can now approve this pull request |
|
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 |
src/librustc/ty/sty.rs
Outdated
| pub fn identity<'a, 'gcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>, def_id: DefId) -> TraitRef<'tcx> { | ||
| TraitRef { | ||
| def_id, | ||
| substs: Substs::identity_for_item(tcx, def_id), |
There was a problem hiding this comment.
Can you make sure no existing code uses identity_for_item with TraitRef?
There was a problem hiding this comment.
found a few uses, commit incoming
src/librustc/ty/mod.rs
Outdated
| DefPathData::Trait(_) => true, | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this a free function and not a method on tcx? Also it's a bit sad to see such a specific hack but I don't have any good suggestion.
There was a problem hiding this comment.
Good question, should be a method on tcx I agree. By hack you are referring to the whole param-env change, right? (Not the fn itself)
I'm somewhat torn, as I wrote elsewhere -- in a way, I think adding things to param-env is the right behavior -- or maybe the answer is that predices_of on traits should include Self: Trait but WF should use predicates_defined_on.
|
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
force-pushed
the
move-self-trait-predicate-to-items
branch
from
f461fde
to
2772f57
Compare
|
Regarding the
|
|
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 |
|
OK, so I've been thinking a bit more about this. I didn't fully remember how the this has got me wondering. Maybe we should do this:
Then we can not change In fact, |
Either way sounds fine to me. Picking the minimally invasive path makes sense. Would this roll back some of the changes you made to tests? I hit some of those, and decided it was best for this change not to modify current behavior (unless it's fixing a clear deficiency.) Also, it seems we've come full circle on |
|
@tmandry it would not affect the tests. We could keep the current behavior by making the "well-formed" rules using |
Yeah =) that design is basically saying " |
So with the change, the behavior is still wrong, just manifested differently, correct? I would only be worried about other unintended consequences we don't currently test for. Admittedly I don't understand the cause of the wrong behavior in the first place, which is why I'm apprehensive. I'm also assuming that these WF checks will be subsumed by chalk at some point. |
|
I just pushed a new commit with the approach I outlined here.
On the branch as I have it now, the behavior is not wrong. That is, proving that
Yes, this is the source of my apprehension as well. We've certainly hit surprises when touching things like this. It might be wise to try to leave the behavior of rustc unaffected and then let chalk fix things in a more principled way, no question. I'm not sure what consequences this change might have. We could try a crater run to see. I'm reminded though of some of the complications we encountered after #48557 landed. OK, here is a proposal: I will make this PR preserve behavior (but help with Chalk) and we can land it. Then I will open a separate PR that fixes the WF behavior and we can do a crater run. |
|
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 |
|
OK, the most recent commit reverts rustc's "WF" behavior so that proving that |
|
I'll cleanup the commit history again |
If we have
```rust
struct Foo<T: Copy = String> { .. }
```
the old code would have proven that `String: Copy` was WF -- this,
incidentally, also proved that `String: Copy`. The new code just
proves `String: Copy` directly.
Co-authored-by: Tyler Mandry <tmandry@gmail.com>
Co-authored-by: Tyler Mandry <tmandry@gmail.com>
Co-authored-by: Tyler Mandry <tmandry@gmail.com>
Co-authored-by: Tyler Mandry <tmandry@gmail.com>
|
Cleaned up the history now. |
nikomatsakis
force-pushed
the
move-self-trait-predicate-to-items
branch
from
610f181
to
0f403a3
Compare
src/librustc_typeck/collect.rs
Outdated
| // For traits, add `Self: Trait` predicate. This is | ||
| // not part of the predicates that a user writes, but it | ||
| // is something that one must prove in order to invoke a | ||
| // method method or project an associated type. |
This new query returns only the predicates *directly defined* on an item (in contrast to the more common `predicates_of`, which returns the predicates that must be proven to reference an item). These two sets are almost always identical except for traits, where `predicates_of` includes an artificial `Self: Trait<...>` predicate (basically saying that you cannot use a trait item without proving that the trait is implemented for the type parameters). This new query is only used in chalk lowering, where this artificial `Self: Trait` predicate is problematic. We encode it in metadata but only where needed since it is kind of repetitive with existing information. Co-authored-by: Tyler Mandry <tmandry@gmail.com>
nikomatsakis
force-pushed
the
move-self-trait-predicate-to-items
branch
from
0f403a3
to
90ea49b
Compare
|
@bors r=scalexm |
|
📌 Commit 90ea49b has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 90ea49b with merge 59ebefd20b923a856b47b6f354f8b895888c132d... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
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 |
1 similar comment
|
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 retry |
bors
added
S-waiting-on-bors
|
cc @rust-lang/infra -- I'm assuming these errors reported above are spurious: |
…s, r=scalexm Move self trait predicate to items This is a "reimagination" of @tmandry's PR #50183. The main effect is described in this comment from one of the commits: --- Before we had the following results for `predicates_of`: ```rust trait Foo { // predicates_of: Self: Foo fn bar(); // predicates_of: Self: Foo (inherited from trait) } ``` Now we have removed the `Self: Foo` from the trait. However, we still add it to the trait ITEM. This is because when people do things like `<T as Foo>::bar()`, they still need to prove that `T: Foo`, and having it in the `predicates_of` seems to be the cleanest way to ensure that happens right now (otherwise, we'd need special case code in various places): ```rust trait Foo { // predicates_of: [] fn bar(); // predicates_of: Self: Foo } ``` However, we sometimes want to get the list of *just* the predicates truly defined on a trait item (e.g., for chalk, but also for a few other bits of code). For that, we define `predicates_defined_on`, which does not contain the `Self: Foo` predicate yet, and we plumb that through metadata and so forth. --- I'm assigning @eddyb as the main reviewer, but I thought I might delegate to scalexm for this one in any case. I also want to post an alternative that I'll leave in the comments; it occurred to me as I was writing. =) r? @eddyb cc @scalexm @tmandry @leodasvacas
|
☀️ Test successful - status-appveyor, status-travis |
tmandry
added
the
WG-traits
This is a "reimagination" of @tmandry's PR #50183. The main effect is described in this comment from one of the commits:
Before we had the following results for
predicates_of:Now we have removed the
Self: Foofrom the trait. However, we stilladd it to the trait ITEM. This is because when people do things like
<T as Foo>::bar(), they still need to prove thatT: Foo, andhaving it in the
predicates_ofseems to be the cleanest way toensure that happens right now (otherwise, we'd need special case code
in various places):
However, we sometimes want to get the list of just the predicates
truly defined on a trait item (e.g., for chalk, but also for a few
other bits of code). For that, we define
predicates_defined_on,which does not contain the
Self: Foopredicate yet, and we plumbthat through metadata and so forth.
I'm assigning @eddyb as the main reviewer, but I thought I might delegate to scalexm for this one in any case. I also want to post an alternative that I'll leave in the comments; it occurred to me as I was writing. =)
r? @eddyb
cc @scalexm @tmandry @leodasvacas