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.

Sign up for GitHub

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

Jump to bottom

Make mtwt resolution part of the HIR lowering #29782

Closed
nrc opened this issue Nov 11, 2015 · 5 comments
Closed

Make mtwt resolution part of the HIR lowering #29782

nrc opened this issue Nov 11, 2015 · 5 comments
Labels
A-HIR Area: The high-level intermediate representation (HIR) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@nrc
Copy link
Member

nrc commented Nov 11, 2015

Currently we do the resolution step of mtwt lazily and on demand in name resolution and (unfortunately) elsewhere. That means we have to keep the (big) tables around longer than is nice and means we are open to hygiene bugs where people forget to do mtwt resolution.

A better solution is to eagerly do mtwt resolution as part of the HIR lowering step. This also means no more Idents in the HIR, only Names, which is a plus.

Happy to mentor this, it's probably better as a second or third bug than a first though since its not super easy.
@nrc nrc added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-HIR Area: The high-level intermediate representation (HIR) labels Nov 11, 2015
@petrochenkov
Copy link
Contributor

I tried it as an alternative to #29748, I agree, it would be much nicer and would kill such hygiene bugs in their root.
Unfortunately, mtwt::resolve can't be applied to everything during lowering to HIR, because of ambiguity between variants/constants and bindings in patterns (and in paths too). For a specific identifier it should be determined first if it is a new binding (then it needs to be renamed) or not (then it should not be renamed), i.e. this step has to be performed after or during name resolution.

Maybe things can be restructured somehow, so that HIR could be modified once more after resolution?

@nrc
Copy link
Member Author

nrc commented Nov 11, 2015

Could you give an example of a not-new binding that should not be renamed? My understanding was that applying the mtwt algorithm should handle all of that, and mtwt::resolve would always work (as long as it's a variable, not for other objects.

@petrochenkov
Copy link
Contributor 

match opt {
    None => {}
    a => {}
}

None is parsed as PatIdent and if all PatIdents are renamed during lowering, then we get something like

match opt {
    None1 => {}
    a1 => {}
}

when we start name resolution, so None1 is not identified as Option::None during resolution and therefore interpreted as a new binding.

@nrc
Copy link
Member Author

nrc commented Nov 12, 2015

Urgh. I'll have to think about that...

@nrc
Copy link
Member Author

nrc commented Nov 17, 2015

Having thought about this, I agree this is a show-stopper, also the world is a cold and awful place.

I think the 'right' thing to do here is to merge the HIR lowering with name resolution so there are no un-resolved names in the HIR (modulo associated types), that needs thought to see if it will work, I guess.

@nrc nrc closed this as completed Nov 17, 2015
@petrochenkov petrochenkov mentioned this issue Dec 1, 2015
Merged
bors added a commit that referenced this issue Dec 9, 2015
@bors
Auto merge of #30145 - petrochenkov:hyg, r=nrc
462ec05 
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_name`s 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-HIR Area: The high-level intermediate representation (HIR) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nrc @petrochenkov