[WIP] Move Self: Trait predicate to trait items instead of the trait itself
#50183
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
TODO (for my own sanity):
|
|
☔ The latest upstream changes (presumably #49837) made this pull request unmergeable. Please resolve the merge conflicts. |
tmandry
force-pushed
the
move-self-trait-predicate-to-items
branch
from
32d2cab
to
293c60f
Compare
This comment has been minimized.
This comment has been minimized.
tmandry
force-pushed
the
move-self-trait-predicate-to-items
branch
from
293c60f
to
defc7e9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tmandry
force-pushed
the
move-self-trait-predicate-to-items
branch
from
cfc20f0
to
edeb9c4
Compare
This comment has been minimized.
This comment has been minimized.
This can matter for obligation causes that get reported as errors, when there are more than one of the same obligation in the forest.
tmandry
force-pushed
the
move-self-trait-predicate-to-items
branch
from
3ccb69d
to
9374cd2
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 |
|
Okay, this now passes all tests up until rustdoc (I'll get to that as soon as I can.) I think it's ready for an initial review, @nikomatsakis. EDIT: I had mentioned a possible refactoring here, but I'm already confused by what I meant :) |
|
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/mod.rs
Outdated
| @@ -2742,6 +2747,23 @@ fn param_env<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
| traits::normalize_param_env_or_error(tcx, def_id, unnormalized_env, cause) | |||
| } | |||
|
|
|||
| pub fn is_trait<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> bool { | |||
| if let Some(id) = tcx.hir.as_local_node_id(def_id) { | |||
There was a problem hiding this comment.
This check doesn't look it is returning the right thing across crates I would prefer to inspect the def-key, as you see here:
rust/src/librustc_traits/lowering.rs
Lines 122 to 132 in d40a0b3
these lines in particular:
rust/src/librustc_traits/lowering.rs
Lines 126 to 127 in d40a0b3
| Some(trait_ref.to_predicate()) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
This feels a bit ad-hoc to me. Can you remember why you added this? Also, when does this "has escaping regions" check return false? I would think .. never ? You could add a assert!(!trait_ref.has_escaping_regions()); instead (and perhaps prove me wrong).
If I understand correctly, this is saying that, to prove that trait Foo { .. } is well-formed, we have to prove that Self: Foo? I'm not sure why that would be useful.
There was a problem hiding this comment.
There are indeed places where trait_ref has escaping regions. There is a call skip_binder() above (look for (*)).
I believe an example that triggers this is here:
rust/src/test/compile-fail/issue-35570.rs
Lines 34 to 35 in 3eadd75
If the trait ref does have escaping regions, it would get filtered out below anyway. Except that trait_ref.to_predicate() invokes ty::Binder::dummy, which has the assertion you mentioned and we get a panic. Using ty::Binder::bind is incorrect and messes up our indices.
On a related note, this fn was copied and modified from the above. I should find a way to remove the repetition.
There was a problem hiding this comment.
If I understand correctly, this is saying that, to prove that trait Foo { .. } is well-formed, we have to prove that Self: Foo? I'm not sure why that would be useful.
It gets used to check bounds on fn items as well.
There was a problem hiding this comment.
Hmm. I'll have to double check what's going on here then, I was confused.
|
|
||
| // A `Self: Trait` predicate on trait items breaks selection, so filter it out. | ||
| // FIXME: This is a big hack! | ||
| if let Some(trait_def_id) = trait_m_predicates.parent { |
There was a problem hiding this comment.
What error do you get here? This feels surprising too! I would rather that -- instead of "filtering this out" -- we add something to the environment that allows it to be proven true, if needed. But it seems like the presence of this should fall out from changes elsewhere.
There was a problem hiding this comment.
A full explanation is here: https://paper.dropbox.com/doc/Removing-SelfTrait-from-predicates_of-notes-neNFC7UaxCj353Du3Dd2d#:h2=Other-questions
That whole doc is really just for this particular change (the title implies that it's for the whole task.)
There was a problem hiding this comment.
TL;DR: The extra predicate breaks downstream winnowing logic
There was a problem hiding this comment.
I wondered if that would happen.
| let parent_id = tcx.hir.get_parent(node_id); | ||
| let parent_def_id = tcx.hir.local_def_id(parent_id); | ||
| debug_assert!(ty::is_trait_node(tcx, parent_id)); | ||
| predicates.push(ty::TraitRef::identity(tcx, parent_def_id).to_predicate()); |
shepmaster
added
the
S-waiting-on-author
shepmaster
removed
the
S-waiting-on-review
|
Ping from triage @tmandry and @nikomatsakis — are there clear next steps for this PR? |
|
@shepmaster: Waiting on some feedback from @nikomatsakis, who is traveling this week. We still need to discern whether this is the right approach. |
pietroalbini
added
S-waiting-on-review
|
Ping from triage @nikomatsakis! The author needs some feedback! |
Skip the binder in less places during WF check. I thought this might help fix rust-lang#50781, but I couldn't actually observe any effect for this changes. Maybe it's still a worthwhile refactoring since skipping binders is generally a bad thing. There is a (manageble) conflict with rust-lang#50183.
pietroalbini
added
the
T-compiler
|
Ping from triage! The PR author needs some feedback, can @nikomatsakis or someone else from @rust-lang/compiler give it? |
1 similar comment
|
Ping from triage! The PR author needs some feedback, can @nikomatsakis or someone else from @rust-lang/compiler give it? |
|
Ping from triage @rust-lang/compiler! This PR has been ignored for more than three weeks, can we get someone to check in on it? |
|
So it looks like rustdoc never used For trait items however, it does use rust/src/librustdoc/clean/mod.rs Lines 2292 to 2294 in 41affd0 rust/src/librustdoc/clean/mod.rs Lines 2346 to 2355 in 41affd0 There is already some logic to do cleanup for |
|
Ping from triage @nikomatsakis / @rust-lang/compiler this PR needs your review! |
|
Thought I'd post an update for anyone wondering what's going on with this PR. Most communication over this PR has been going on in the WG-traits Discord. In short, we've been wrestling with some idiosyncrasies of the type system that make this change much trickier than originally anticipated. @nikomatsakis has been experimenting with some changes to the approach, but is traveling this week. I haven't had much time to dig in myself lately, but it may be beyond me at this point :) Given that making further progress on this probably requires an extensive understanding of the type checker and there are many other edition-related priorities, this PR was getting starved for attention. However, it has gotten more lately, and I expect that will continue since it's starting to block progress on #49177! |
|
Ping from triage @nikomatsakis! This PR needs your 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 |
|
So I've reworked these commits. I'm going to close and re-open this PR with a new branch at this point I think. |
|
(Just so we can have some fresh conversation!) |
…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
tmandry
added
the
WG-traits
The
predicates_ofquery on a traitFooshould only return where clauses on that trait. Right now, it includesSelf: Foo, which causes problems when lowering to new chalk-style logic.This PR moves
Self: Footo the trait items, rather than the trait itself.Status: Functional enough to compile std, but some tests failing; needs cleanup.
r? @nikomatsakis