Skip to content

Remove hir::Ident - #33654

Merged
bors merged 3 commits into
rust-lang:masterfrom
petrochenkov:hirident
May 18, 2016
Merged

Remove hir::Ident#33654
bors merged 3 commits into
rust-lang:masterfrom
petrochenkov:hirident

Conversation

@petrochenkov

@petrochenkovpetrochenkov commented May 15, 2016

Copy link
Copy Markdown
Contributor

Now when name resolution is done on AST, hir::Ident is no longer necessary.
See #30145 for more details.

r? @nrc

nrc
nrc reviewed May 15, 2016
View reviewed changes
Comment threadsrc/librustc/hir/lowering.rs Outdated

@nrcnrcMay 15, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we only sometimes rename here?

@petrochenkovpetrochenkovMay 15, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We don't want to rename constants or unit variants/structs, that can also be represented with PatKind::Ident due to parsing ambiguities, only fresh bindings.

@nrcnrcMay 15, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At this stage we are post-parsing and post-name resolution, so where do the remaining ambiguities get resolved? Put another way, in some sense this renaming must happen somewhere, where does it happen (and how)?

@petrochenkovpetrochenkovMay 16, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm not sure I understand the question.

At this stage we are post-parsing and post-name resolution, so where do the remaining ambiguities get resolved?

All ambiguities are resolved during name resolution, there's nothing ambiguous at this stage.

Put another way, in some sense this renaming must happen somewhere, where does it happen (and how)?

Renaming is done twice, once during name resolution, its results aren't saved anywhere, and once more during lowering to HIR, its results are persistent and kept in HIR.
Names in HIR need to be properly renamed to correctly treat cases like this, which in principle can be interpreted as requiring some rudimentary form of name resolution during type checking.

@petrochenkovpetrochenkovMay 16, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

If tables of "canonical bindings" were filled during the main name resolution pass and saved for later, then HIR could keep plain non-renamed Names for everything.

@nrcnrcMay 18, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for the explanation!

@bors

bors commented May 16, 2016

Copy link
Copy Markdown
Collaborator

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

@petrochenkov
Remove hir::Ident
aad347c
@petrochenkov
lowering: Rename identifiers only when necessary
79b343d
Do not rename invalid identifiers, they stop being invalid after renaming
@petrochenkov
Fix rebase
02a1eef
@petrochenkov

petrochenkov commented May 16, 2016

Copy link
Copy Markdown
ContributorAuthor

Rebased.

@nrc

nrc commented May 18, 2016

Copy link
Copy Markdown
Member

@bors: r+

@bors

bors commented May 18, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 02a1eef has been approved by nrc

@bors

bors commented May 18, 2016

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 02a1eef with merge 310d899...

bors added a commit that referenced this pull request May 18, 2016
@bors
Auto merge of#33654- petrochenkov:hirident, r=nrc
310d899
Remove hir::Ident
Now when name resolution is done on AST, `hir::Ident` is no longer necessary.
See #30145 for more details.
r? @nrc
@bors

bors commented May 18, 2016

Copy link
Copy Markdown
Collaborator

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors
bors merged commit 02a1eef into rust-lang:masterMay 18, 2016
This was referenced May 18, 2016
Merged
Closed
Merged
Merged
@mcartonmcarton mentioned this pull request May 19, 2016
Merged
@petrochenkovpetrochenkov mentioned this pull request Jun 10, 2016
Merged
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2016
@Manishearth
Rollup merge ofrust-lang#34207- petrochenkov:nohyg, r=jseyfried
d84993b
Remove last traces of identifier hygiene from HIR
rust-lang@e783a0a removed the [last](rust-lang#33654 (comment)) [use](rust-lang#33654 (comment)) of hygiene at post-resolve compilation stages, so we can avoid renaming during lowering to HIR and just keep original names.
r? @nrc
@petrochenkov
petrochenkov deleted the hirident branch September 21, 2016 19:56
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nrcnrc

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@petrochenkov@bors@nrc