-
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
GAT/const_generics: Allow with_opt_const_param to return GAT param def_id #81911
Conversation
r? @varkor (rust-highfive has picked a reviewer for you, use r? to override) |
6a06bf1
to
1d9ac3c
Compare
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.
Hi! Can you add some comments along the lines of what I asked for? That'd be really helpful.
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.
OK, those comments helped a lot, and I understand why the code works! I think they could be made better. Left some thoughts. Let me know what you think.
@@ -29,6 +29,64 @@ pub(super) fn opt_const_param_of(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Option< | |||
let parent_node = tcx.hir().get(parent_node_id); | |||
|
|||
match parent_node { | |||
// This matches on types who's paths couldn't be resolved without typeck'ing e.g. |
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 was reading the rustdoc for the opt_const_param_of
query and I think this isn't quite right.
This query would, I think, be invoked on the 3
in your example, and it would be returning the const N1: usize
node.
It doesn't have much to do with Self::Assoc
in particular, from what I can tell, except that this is the context in which the 3
appears.
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.
In other words, I think if you wrote <Self as Foo>::Assoc<3>
the query could still be invoked
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.
OK, I'm wrong, I see your point. The query is invoked on 3
, but this match arm is specific to the case where the 3
appears as part of a Self::Assoc<3>
type thing.
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 am leaving my stream of consciousness to help guide you in how to improve the comment)
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 rewrote that comment to put more emphasis on what some of the variables correspond to and what exactly the match arm matches on :)
Node::Ty(hir_ty @ Ty { kind: TyKind::Path(QPath::TypeRelative(_, segment)), .. }) => { | ||
// Walk up from the parent_node to find an item so that | ||
// we can resolve the relative path to an actual associated type. | ||
// For the code example above, this item would be the Foo trait. |
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 found this comment confusing. I think what's going on is this:
- Find the item I that contains the anonymous constant so that we can create the context for it
- Using that context, we will convert the HIR for
Self::Assoc<3>
into a type, which will be a fully resolved projection like<Self as Foo>::Assoc<3>
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.
Yea I wasn't great at this comment as I'm not all that familiar with the exact details of this Item stuff. What you said sounds correct to me so I'll just uh take that and put it in the comment x]
@bors r+ |
📌 Commit 7ca96ed has been approved by |
…komatsakis GAT/const_generics: Allow with_opt_const_param to return GAT param def_id Fixes rust-lang#75415 Fixes rust-lang#79666 cc `@lcnr` I've absolutely no idea who to r? for this...
…komatsakis GAT/const_generics: Allow with_opt_const_param to return GAT param def_id Fixes rust-lang#75415 Fixes rust-lang#79666 cc ``@lcnr`` I've absolutely no idea who to r? for this...
Rollup of 10 pull requests Successful merges: - rust-lang#79775 (Fix injected errors when running doctests on a crate named after a keyword) - rust-lang#81012 (Stabilize the partition_point feature) - rust-lang#81479 (Allow casting mut array ref to mut ptr) - rust-lang#81506 (HWAddressSanitizer support) - rust-lang#81741 (Increment `self.index` before calling `Iterator::self.a.__iterator_ge…) - rust-lang#81850 (use RWlock when accessing os::env) - rust-lang#81911 (GAT/const_generics: Allow with_opt_const_param to return GAT param def_id) - rust-lang#82022 (Push a `char` instead of a `str` with len one into a String) - rust-lang#82023 (Remove unnecessary lint allow attrs on example) - rust-lang#82030 (Use `Iterator::all` instead of open-coding it) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #75415
Fixes #79666
cc @lcnr
I've absolutely no idea who to r? for this...