Do MTWT resolution during lowering to HIR #30145

petrochenkov · Dec 1, 2015

Instead of ast::Ident, bindings, paths and labels in HIR now keep a new structure called hir::Ident containing mtwt-renamed name and the original not-renamed unhygienic_name. name is supposed to be used by default, unhygienic_name is rarely used.

This is not ideal, but better than the status quo for two reasons:

MTWT tables can be cleared immediately after lowering to HIR
This is less bug-prone, because it is impossible now to forget applying mtwt::resolve to a name. It is still possible to use name instead of unhygienic_name by mistake, but unhygienic_names are used only in few very special circumstances, so it shouldn't be a problem.

Besides name resolution unhygienic_name is used in some lints and debuginfo. unhygienic_name can be very well approximated by "reverse renaming" token::intern(name.as_str()) or even plain string name.as_str(), except that it would break gensyms like iter in desugared for loops. This approximation is likely good enough for lints and debuginfo, but not for name resolution, unfortunately (see #27639), so unhygienic_name has to be kept.

cc #29782

r? @nrc

bors · Dec 4, 2015

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

petrochenkov · Dec 4, 2015

Rebased.

nrc · Dec 6, 2015

nrc reviewed Dec 6, 2015

View changes

nrc · Dec 6, 2015

src/librustc_driver/driver.rs

+
+        // Discard MTWT tables that aren't required past lowering to HIR.
+        if !sess.opts.debugging_opts.keep_mtwt_tables &&
+           !sess.opts.debugging_opts.save_analysis {


Why does save-analysis need the mtwt tables?

It calls lower_crate, lowering uses mtwt tables.

nrc · Dec 6, 2015

nrc reviewed Dec 6, 2015

View changes

nrc · Dec 6, 2015

src/librustc_front/hir.rs

+}
+
+impl Ident {
+    pub fn from_name(name: Name) -> Ident {


should this be from_name_unhygienic or something? It seems like using this where you shouldn't would introduce hygiene bugs.

I'm not sure how to call it better. For example, for loop desugaring calls from_name with gensym iter exactly for unhygienic_name to be hygienic.

from_hygienic_name? Since the caller is guaranteeing that the input is already hygienic? Otherwise just a comment explaining the danger would be good.

nrc · Dec 6, 2015

nrc reviewed Dec 6, 2015

View changes

nrc · Dec 6, 2015

src/librustc_front/lowering.rs

-pub fn lower_path(lctx: &LoweringContext, p: &Path) -> hir::Path {
+// Path segments are usually unhygienic, hygienic path segments can occur only in
+// identifier-like paths originating from `ExprPath`.
+// Make life simpler for rustc_resolve by renaming only such segments.


How much simpler does this make resolve? This seems quite a fragile treatment

Renaming all path segments requires many uses of unhygienic_name in all kind of paths all over the resolve instead of one place in resolve_identifier. Most of path segments are unhygienic, only identifiers ~~are~~can be hygienic.
I even thought about splitting ExprPath into ExprPath and ExprIdent (by analogy with PatEnum and PatIdent in patterns) to make this problem go away, but this is likely not beneficial for later stages after resolve.

We probably want to do something about the ExprPath/PatEnum/PatIdent mess in the HIR when we have post-resolve HIR.

Yeah, the more I learn about these things the more I like the idea of doing name resolution on AST and removing all this confusion from HIR.

nrc · Dec 6, 2015

Nice!

petrochenkov · Dec 7, 2015

Updated with comments for Ident::from_name and for identifiers in path segments

nrc · Dec 7, 2015

@bors: r+

bors · Dec 7, 2015

📌 Commit c1d3164 has been approved by nrc

bors · Dec 9, 2015

⌛️ Testing commit c1d3164 with merge 462ec05...

bors · Dec 9, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, 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-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned nrc Dec 1, 2015

Do MTWT resolution during lowering to HIR

Loading status checks…

eb789de

petrochenkov force-pushed the petrochenkov:hyg branch from 0aea51c to eb789de Dec 4, 2015

Add comments for Ident::from_name and identifiers in path segments

Loading status checks…

c1d3164

bors merged commit c1d3164 into rust-lang:master Dec 9, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Dec 9, 2015

Closed

Warn unused trait imports #30021

Closed

Improve OwnedSlice and use it in HIR #30095

petrochenkov referenced this pull request Apr 30, 2016
Merged
Remove the requirement that ast->hir lowering be reproducible #33296

petrochenkov referenced this pull request May 15, 2016
Merged
Remove hir::Ident #33654

bors added a commit that referenced this pull request May 18, 2016

Auto merge of #33654 - petrochenkov:hirident, r=nrc
Remove hir::Ident Now when name resolution is done on AST, `hir::Ident` is no longer necessary. See #30145 for more details. r? @nrc

310d899

oli-obk referenced this pull request May 20, 2016
Merged
Rustup to *rustc 1.10.0-nightly (764ef92ae 2016-05-19)* and bump to 0.0.69 #944

petrochenkov deleted the petrochenkov:hyg branch Sep 21, 2016

rust-lang/rust

Join GitHub today

Do MTWT resolution during lowering to HIR #30145

Conversation

petrochenkov commented Dec 1, 2015

rust-highfive assigned nrc Dec 1, 2015

This comment has been minimized.

bors commented Dec 4, 2015

petrochenkov force-pushed the petrochenkov:hyg branch from 0aea51c to eb789de Dec 4, 2015

This comment has been minimized.

petrochenkov commented Dec 4, 2015

nrc reviewed Dec 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

nrc reviewed Dec 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nrc reviewed Dec 6, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nrc commented Dec 6, 2015

This comment has been minimized.

petrochenkov commented Dec 7, 2015

This comment has been minimized.

nrc commented Dec 7, 2015

This comment has been minimized.

bors commented Dec 7, 2015

This comment has been minimized.

bors commented Dec 9, 2015

bors added a commit that referenced this pull request Dec 9, 2015

This comment has been minimized.

bors commented Dec 9, 2015

Hide details View details bors merged commit c1d3164 into rust-lang:master Dec 9, 2015 2 checks passed

2 checks passed

This was referenced Dec 9, 2015

petrochenkov referenced this pull request Apr 30, 2016

petrochenkov referenced this pull request May 15, 2016

bors added a commit that referenced this pull request May 18, 2016

oli-obk referenced this pull request May 20, 2016

petrochenkov deleted the petrochenkov:hyg branch Sep 21, 2016

petrochenkov force-pushed the petrochenkov:hyg branch from `0aea51c` to `eb789de` Dec 4, 2015

nrc reviewed Dec 6, 2015

View changes

nrc reviewed Dec 6, 2015

View changes

nrc reviewed Dec 6, 2015

View changes

bors merged commit `c1d3164` into rust-lang:master Dec 9, 2015
2 checks passed