Miscellaneous macro expansion cleanup and groundwork #34459
Conversation
| @@ -2283,24 +2283,25 @@ impl<'a> Resolver<'a> { | |||
| PatKind::Ident(bmode, ref ident, ref opt_pat) => { | |||
| // First try to resolve the identifier as some existing | |||
| // entity, then fall back to a fresh binding. | |||
| let resolution = if let Ok(resolution) = self.resolve_path(pat.id, | |||
| &Path::from_ident(ident.span, ident.node), 0, ValueNS) { | |||
| let local_def = self.resolve_identifier(ident.node, ValueNS, true); | |||
There was a problem hiding this comment.
Why is this preferable?
I used resolve_path solely for stylistic reasons - it returns exactly what we want here (PathResolution), and returns "fully correct" result with all the adjustments like Def::Local -> Def::Upvar (even if they don't matter here).
I also don't like mixing "intermediate" resolution byproducts like LocalDef with final PathResolutions, again for stylistic reasons - high-level routines like resolve_pattern should work only with the latter. Can resolve_identifier return fully adjusted Result<PathResolution>, like resolve_path?
There was a problem hiding this comment.
I changed from resolve_path to resolve_identifier specifically to avoid the adjustments.
For example, consider:
fn f(x: i32) {
fn g(x: i32) {}
}Here, if we call resolve_path on g's argument x, it "should" resolve to f's argument x. When this resolution gets adjusted, it would emit
can't capture dynamic environment in a fn item
(since we've already pushed the Ribs for g), which we clearly don't want.
I say "should" because right now, g's argument x does not resolve to f's argument x because of an implementation detail of the current hygiene algorithm. I'm working on a simplification of the hygiene algorithm after which g's argument x will resolve to f's argument x, and this change is groundwork for that.
There was a problem hiding this comment.
Something is wrong here. Ideally, the adjustment routine should not report "can't capture" errors, it should just leave Locals outside of closures unadjusted.
Regarding
fn f(x: i32) {
fn g(x: i32) {}
}
the consensus in #33118 seemed to be that items in block scopes are isolated from local variables, i.e. g shouldn't "see" the outer x and the inner x won't resolve to it.
There was a problem hiding this comment.
@petrochenkov
I agree with that consensus, but haven't gotten around to implementing it yet. Once that's implemented and the inner x doesn't resolve to the outer x as you described, I agree that it would probably be better to use resolve_path or to have resolve_identifier do adjustment.
There was a problem hiding this comment.
Ok, this is not a big deal anyway.
jseyfried
force-pushed
the
expansion_cleanup
branch
3 times, most recently
from
309e508
to
72310b6
Compare
|
What is the motivation for the first commit? The precise behaviour here was not specified in the RFC (16) and I don't see why one behaviour is better than the other. (My personal preference is not to allow attributes on expressions, only statements, and implementing that would require reverting this commit, I think). |
|
r+ for everything else |
|
@nrc My motivation is to have each attribute apply to exactly one AST node. Right now, we consider attributes on an item in a statement position to apply to both the item and the statement (that is, the attribute is included in That being said, this isn't a big deal -- if you still don't like it I'll remove that commit. |
|
@bors: r+ sounds ok to me |
|
📌 Commit 72310b6 has been approved by |
|
☔ The latest upstream changes (presumably #34424) made this pull request unmergeable. Please resolve the merge conflicts. |
… on the statement
…ssify ident patterns
jseyfried
force-pushed
the
expansion_cleanup
branch
from
72310b6
to
e58963d
Compare
|
@bors r=nrc |
|
📌 Commit e58963d has been approved by |
Miscellaneous macro expansion cleanup and groundwork r? @nrc
r? @nrc