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

Remove last traces of identifier hygiene from HIR #34207

Merged
merged 1 commit into from
Jun 16, 2016

Conversation

petrochenkov
Copy link
Contributor

e783a0a removed the last use of hygiene at post-resolve compilation stages, so we can avoid renaming during lowering to HIR and just keep original names.

r? @nrc

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 10, 2016

I need to be sure clippy is okay with this change, while the compiler itself doesn't use information removed in this patch anymore, external lints still can use it.
Since this patch removes hygiene information from HIR names, lints running in the late lint pass and performing name-resolution-like functions requiring hygiene will have to be moved to the early lint pass, but then they will lose information from later stages (e.g. types) if they use it. Do such lints exist? Is it important to support hygiene in the compiler to be used exclusively by third party tools? I don't have a clear answer.

cc @Manishearth @mcarton

@Manishearth
Copy link
Member

It looks like we already stopped using unhygenize. Might still break, though.

We rely on span expansion info now mostly.

@mcarton
Copy link
Member

mcarton commented Jun 11, 2016

@Manishearth: We still use unhygienize 😛:

$ ag unhygienize
clippy_lints/src/shadow.rs
69:            bindings.push((ident.node.unhygienize(), ident.span))
123:            let name = ident.node.unhygienize();
330:    !path.global && path.segments.len() == 1 && path.segments[0].name.unhygienize() == name
340:        if self.name == name.unhygienize() {

clippy_lints/src/misc.rs
414:                   segment != segment.unhygienize() && // not in bang macro

From the comment, the last one looks like it could use in_macro. I won't have much time today to look at the other.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 11, 2016

unhygienize is not the problem, it can be just removed after this change. The question is about the need for hygienic comparisons in lints together with something like types.

@jseyfried
Copy link
Contributor

Nice! LGTM.
Couldn't lints do hygienic comparisons by looking up the identifiers' resolutions in the def_map and comparing the resolutions?

@petrochenkov
Copy link
Contributor Author

Couldn't lints do hygienic comparisons by looking up the identifiers' resolutions in the def_map and comparing the resolutions?

That's a good observation, all hygienic things (i.e. bindings, labels and some expression paths) have definitions in def_map, except for some function parameters, so yes, they can be used instead.

@mcarton
Copy link
Member

mcarton commented Jun 15, 2016

LGTM, I have a working PR for Clippy based on this.

@jseyfried
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 15, 2016

📌 Commit f59afbc has been approved by jseyfried

@bors
Copy link
Contributor

bors commented Jun 16, 2016

⌛ Testing commit f59afbc with merge 9604420...

Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2016
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
@bors
Copy link
Contributor

bors commented Jun 16, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

bors added a commit that referenced this pull request Jun 16, 2016
Rollup of 4 pull requests

- Successful merges: #34207, #34268, #34270, #34290
- Failed merges:
@bors bors merged commit f59afbc into rust-lang:master Jun 16, 2016
@petrochenkov petrochenkov deleted the nohyg branch September 21, 2016 19:58
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.

6 participants