-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Separate bindings from other patterns in HIR #33929
Conversation
/// referred to as simply `T::CONST`, in which case they will end up as | ||
/// PatKind::Path, and the resolver will have to sort that out. | ||
/// A path pattern written in qualified form, i.e. `<T as Trait>::CONST` or `<T>::CONST`. | ||
/// Such patterns can only refer to associated constants at the moment. |
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.
Why did this comment change?
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.
Oh, it slipped from the later unpublished part of the patch.
The old comment basically says "yep, it's an associated constant" while it's only a syntactic form - qualified path, it can resolve to a constant, but it also can resolve to any other associated item - a method or an associated type (?), and passes working on HIR should be ready to deal with it.
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.
s/refer/legally refer/
would probably be better
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.
Oh, it slipped from the later unpublished part of the patch.
Thought so. I'd take it out of this PR, it seems out of place.
@petrochenkov About path lowering, @nikomatsakis and I were discussion #33596 and we agreed that the proper solution involves having something like |
Updated. |
@bors r+ |
📌 Commit ae999e9 has been approved by |
⌛ Testing commit ae999e9 with merge d190a99... |
💔 Test failed - auto-linux-64-opt-rustbuild |
@bors retry |
Separate bindings from other patterns in HIR Now when name resolution is done on AST, we can avoid dumping everything that looks like an identifier into `PatKind::Ident` in HIR. `hir::PatKind::Ident` is removed, fresh bindings are now called `hir::PatKind::Binding`, everything else goes to `hir::PatKind::Path`. I intend to do something with `PatKind::Path`/`PatKind::QPath` as well using resolution results, but it requires some audit and maybe some deeper refactoring of relevant resolution/type checking code to do it properly. I'm submitting this part of the patch earlier to notify interested parties that I'm working on this. cc @jseyfried r? @eddyb
Improvements to pattern resolution + some refactoring Continuation of #33929 First commit is a careful rewrite of `resolve_pattern`, pattern path resolution and new binding creation logic is factored out in separate functions, some minor bugs are fixed. Also, `resolve_possibly_assoc_item` doesn't swallow modules now. Later commits are refactorings, see the comment descriptions. I intend to continue this work later with better support for `Def::Err` in patterns in post-resolve stages and cleanup of pattern resolution code in type checker. Fixes #32086 Fixes #34047 ([breaking-change]) Fixes #34074 cc @jseyfried r? @eddyb
Now when name resolution is done on AST, we can avoid dumping everything that looks like an identifier into
PatKind::Ident
in HIR.hir::PatKind::Ident
is removed, fresh bindings are now calledhir::PatKind::Binding
, everything else goes tohir::PatKind::Path
.I intend to do something with
PatKind::Path
/PatKind::QPath
as well using resolution results, but it requires some audit and maybe some deeper refactoring of relevant resolution/type checking code to do it properly.I'm submitting this part of the patch earlier to notify interested parties that I'm working on this.
cc @jseyfried
r? @eddyb