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

Give ast::ExprKind::Paren no-op expressions the same ids as their children. #34355

Merged
merged 2 commits into from
Jun 30, 2016

Conversation

jseyfried
Copy link
Contributor

Having ast::ExprKind::Paren expressions share ids with their children

  • reduces the number of unused NodeIds in the hir map and
  • guarantees that tcx.map.expect_expr(ast_expr.id) is the hir corresponding to ast_expr.

This fixes the bug from #34327, which was introduced in #33296 when I assumed the above guarantee.

r? @nrc

@arielb1
Copy link
Contributor

arielb1 commented Jun 19, 2016

guarantees that tcx.map.expect_expr(ast_expr.id) is the hir corresponding to ast_expr.

This does not sound like a guarantee we want to maintain.

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 19, 2016

@arielb1
Hmm, do you have something in mind that would break that guarantee?
That is, what ast expression besides ExprKind::Paren would not correspond to a unique hir node?

@arielb1
Copy link
Contributor

arielb1 commented Jun 19, 2016

@jseyfried

We are planning to do a major refactoring of ExprPath and PatPath - there may be problems for patterns.

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 19, 2016

@arielb1 ok, I believe save_analysis only relies on the guarantee for expressions now. If we do break the guarantee, it should be straightforward to fix any fallout in save_analysis.

I'm still not sure which ast nodes besides Paren wouldn't correspond a unique hir node, though.

@nrc
Copy link
Member

nrc commented Jun 29, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 29, 2016

📌 Commit 24a3a26 has been approved by nrc

@bors
Copy link
Contributor

bors commented Jun 29, 2016

⌛ Testing commit 24a3a26 with merge ad3e6e6...

@bors
Copy link
Contributor

bors commented Jun 29, 2016

💔 Test failed - auto-linux-64-opt

@jseyfried jseyfried force-pushed the paren_expression_ids_nonunique branch from 24a3a26 to 8557a2e Compare June 29, 2016 11:06
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Jun 29, 2016

📌 Commit 8557a2e has been approved by nrc

Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 29, 2016
…nique, r=nrc

Give `ast::ExprKind::Paren` no-op expressions the same ids as their children.

Having `ast::ExprKind::Paren` expressions share ids with their children
 - reduces the number of unused `NodeId`s in the hir map and
 - guarantees that `tcx.map.expect_expr(ast_expr.id)` is the hir corresponding to `ast_expr`.

This fixes the bug from rust-lang#34327, which was introduced in rust-lang#33296 when I assumed the above guarantee.

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 8557a2e into rust-lang:master Jun 30, 2016
@jseyfried jseyfried deleted the paren_expression_ids_nonunique branch February 12, 2017 13:11
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Jul 19, 2021
The special case breaks several useful invariants (`ExpnId`s are
globally unique, and never change). This special case
was added back in 2016 in rust-lang#34355
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2021
…rochenkov

Remove special case for `ExprKind::Paren` in `MutVisitor`

The special case breaks several useful invariants (`ExpnId`s are
globally unique, and never change). This special case
was added back in 2016 in rust-lang#34355

r? `@petrochenkov`
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