Improvements to pattern resolution + some refactoring #34095
Conversation
| @@ -1196,12 +1195,11 @@ impl<'a, 'gcx, 'tcx> MemCategorizationContext<'a, 'gcx, 'tcx> { | |||
|
|
|||
| (*op)(self, cmt.clone(), pat); | |||
|
|
|||
| // This function can be used during region checking when not all paths are fully | |||
| // resolved. Partially resolved paths in patterns can only legally refer to | |||
| // associated constants which don't require categorization. | |||
There was a problem hiding this comment.
Region-checking runs after all paths have been resolved though, doesn't it?
There was a problem hiding this comment.
Actually, I made this comment after encountering an ICE. But maybe it was "no resolution at all" for a non-path pattern rather than a partial resolution. I'll investigate.
There was a problem hiding this comment.
It's called during closure kind inference.
|
LGTM, modulo the questions I've raised, although I'd like a second look. cc @nrc |
|
I feel that it would be better to make all same-named idents in a match arm resolve to the same pattern, as I did in arielb1@0dbd027 - this makes for much cleaner code, and removes the need for hygiene after lowering. |
| path_names_to_string(path, 0), ty.id, def); | ||
| self.record_def(ty.id, def); | ||
| match def.base_def { | ||
| Def::Mod(..) => { |
There was a problem hiding this comment.
Maybe have some real marker instead of Def::Mod?
There was a problem hiding this comment.
This particular Def::Mod is not a marker, but a real module encountered in type position.
As for Def::Mod used in fake resolutions for <T>::A, it's a status quo, I'd like to come up with some better solution, then used now, but not in this PR. Just using a separate marker for it doesn't make much difference, I did some quick experiments.
That would be great, I wanted to do this someday, but not as a part of this PR. If you already have the implementation, can I just apply it on top of this PR? |
| pat_id: NodeId, | ||
| outer_pat_id: NodeId, | ||
| pat_src: PatternSource, | ||
| bindings_list: &mut HashMap<Name, NodeId>) |
There was a problem hiding this comment.
nit: I think bindings or bindings_map would make more sense
|
@petrochenkov Excellent! @eddyb feel free to r=me if you're on the fence. |
|
Sure. If you won't add it to this PR I will land it myself. |
|
Updated. |
|
☔ The latest upstream changes (presumably #32202) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased. |
|
@bors r=jseyfried |
|
📌 Commit 7b048c3 has been approved by |
|
Oops, just fixed one more typo. |
|
@bors r=jseyfried |
|
📌 Commit d407da6 has been approved by |
|
☔ The latest upstream changes (presumably #34149) made this pull request unmergeable. Please resolve the merge conflicts. |
This simplifies the code considerably, removing one of the last uses of hygienic matching out of resolution.
|
Rebased again. |
|
@bors r=jseyfried |
|
📌 Commit 6d7b35b has been approved by |
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
Some more pattern cleanup and bugfixing The next part of #34095 The most significant fixed mistake is definitions for partially resolved associated types not being updated after full resolution. ``` fn f<T: Fn()>(arg: T::Output) { .... } // <- the definition of T::Output was not updated in def_map ``` For this reason unstable associated types of stable traits, like `FnOnce::Output`, could be used in stable code when written in unqualified form. Now they are properly checked, this is a **[breaking-change]** (pretty minor one, but a crater run would be nice). The fix is not to use unstable library features in stable code, alternatively `FnOnce::Output` can be stabilized. Besides that, paths in struct patterns and expressions `S::A { .. }` are now fully resolved as associated types. Such types cannot be identified as structs at the moment, i.e. the change doesn't make previously invalid code valid, but it improves error diagnostics. Other changes: `Def::Err` is supported better (less chances for ICEs for erroneous code), some incorrect error messages are corrected, some duplicated error messages are not reported, ADT definitions are now available through constructor IDs, everything else is cleanup and code audit. Fixes #34209 Closes #22933 (adds tests) r? @eddyb
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_itemdoesn't swallow modules now.Later commits are refactorings, see the comment descriptions.
I intend to continue this work later with better support for
Def::Errin 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