Skip to content
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

Miscellaneous macro expansion cleanup and groundwork #34459

Merged
merged 6 commits into from
Jun 30, 2016

Conversation

jseyfried
Copy link
Contributor

r? @nrc

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jseyfried jseyfried Jun 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jseyfried jseyfried Jun 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is not a big deal anyway.

@jseyfried jseyfried force-pushed the expansion_cleanup branch 3 times, most recently from 309e508 to 72310b6 Compare June 25, 2016 10:17
@nrc
Copy link
Member

nrc commented Jun 27, 2016

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).

@nrc
Copy link
Member

nrc commented Jun 27, 2016

r+ for everything else

@jseyfried
Copy link
Contributor Author

@nrc
I don't think the first commit will change any observable behavior (correct me if I'm wrong). Also, I don't think it would preclude forbidding attributes on expressions.

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 stmt.attrs() and item.attrs()). After the first commit, stmt.attrs() will be empty on item declaration statements so that the item's attributes are only in item.attrs().

That being said, this isn't a big deal -- if you still don't like it I'll remove that commit.

@nrc
Copy link
Member

nrc commented Jun 28, 2016

@bors: r+

sounds ok to me

@bors
Copy link
Contributor

bors commented Jun 28, 2016

📌 Commit 72310b6 has been approved by nrc

@bors
Copy link
Contributor

bors commented Jun 28, 2016

☔ The latest upstream changes (presumably #34424) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Jun 28, 2016

📌 Commit e58963d has been approved by nrc

Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 29, 2016
Miscellaneous macro expansion cleanup and groundwork

r? @nrc
bors added a commit that referenced this pull request Jun 30, 2016
Rollup of 11 pull requests

- Successful merges: #34355, #34446, #34459, #34460, #34467, #34495, #34497, #34499, #34513, #34536, #34542
- Failed merges:
@bors bors merged commit e58963d into rust-lang:master Jun 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants