-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[WIP] Move Self: Trait
predicate to trait items instead of the trait itself
#50183
[WIP] Move Self: Trait
predicate to trait items instead of the trait itself
#50183
Conversation
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. |
32d2cab
to
293c60f
Compare
This comment has been minimized.
This comment has been minimized.
293c60f
to
defc7e9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
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 |
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.
Looking good, but I've got a lot of questions. As I mentioned on gitter, might be best if I clone and run some experiments to understand better. =)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
crate fn program_clauses_for<'a, 'tcx>( | |
tcx: TyCtxt<'a, 'tcx, 'tcx>, | |
def_id: DefId, | |
) -> Clauses<'tcx> { | |
match tcx.def_key(def_id).disambiguated_data.data { | |
DefPathData::Trait(_) => program_clauses_for_trait(tcx, def_id), | |
DefPathData::Impl => program_clauses_for_impl(tcx, def_id), | |
DefPathData::AssocTypeInImpl(..) => program_clauses_for_associated_type_value(tcx, def_id), | |
_ => Slice::empty(), | |
} | |
} |
these lines in particular:
rust/src/librustc_traits/lowering.rs
Lines 126 to 127 in d40a0b3
match tcx.def_key(def_id).disambiguated_data.data { | |
DefPathData::Trait(_) => program_clauses_for_trait(tcx, def_id), |
Some(trait_ref.to_predicate()) | ||
} else { | ||
None | ||
}; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
fn takes_lifetime(_f: for<'a> fn(&'a ()) -> <() as Lifetime<'a>>::Out) { | |
} |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: The extra predicate breaks downstream winnowing logic
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 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()); |
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.
This feels right! ✅
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. |
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.
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
The
predicates_of
query on a traitFoo
should 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: Foo
to the trait items, rather than the trait itself.Status: Functional enough to compile std, but some tests failing; needs cleanup.
r? @nikomatsakis