Skip to content

Cleanup handling of hygiene for built-in macros #64469

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

Merged
merged 4 commits into from
Sep 15, 2019

Conversation

matthewjasper
Copy link
Contributor

This makes most identifiers generated by built-in macros use def-site hygiene, not only the ones that previously used gensyms.

  • ExtCtxt::ident_of now takes a Span and is preferred to Ident::{from_str, from_str_and_span}
  • Remove Span::with_legacy_ctxt
    • assert now uses call-site hygiene because it needs to resolve panic unhygienically.
    • concat_idents now uses call-site hygiene because it wouldn't be very useful with def-site hygiene.
    • everything else is moved to def-site hygiene

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 14, 2019
@petrochenkov
Copy link
Contributor

r=me with Centril's comment addressed

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2019
@matthewjasper
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Sep 15, 2019

📌 Commit 8ab67c8 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 15, 2019
@bors
Copy link
Collaborator

bors commented Sep 15, 2019

⌛ Testing commit 8ab67c8 with merge 117cdf3...

bors added a commit that referenced this pull request Sep 15, 2019
…nkov

Cleanup handling of hygiene for built-in macros

This makes most identifiers generated by built-in macros use def-site hygiene, not only the ones that previously used gensyms.

* `ExtCtxt::ident_of` now takes a `Span` and is preferred to `Ident::{from_str, from_str_and_span}`
* Remove `Span::with_legacy_ctxt`
    * `assert` now uses call-site hygiene because it needs to resolve `panic` unhygienically.
    * `concat_idents` now uses call-site hygiene because it wouldn't be very useful with def-site hygiene.
    * everything else is moved to def-site hygiene

r? @petrochenkov
let sp = cx.with_legacy_ctxt(sp);
// `core::panic` and `std::panic` are different macros, so we use call-site
// context to pick up whichever is currently in scope.
let sp = cx.with_call_site_ctxt(sp);
Copy link
Contributor

@petrochenkov petrochenkov Sep 15, 2019

Choose a reason for hiding this comment

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

I'd rather use the call-site span in a more targeted way though, for the panic identifier specifically, and def-site for everything else.
(Not sure if this makes observable difference in this case, but it's a safer default.)

@bors
Copy link
Collaborator

bors commented Sep 15, 2019

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 117cdf3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 15, 2019
@bors bors merged commit 8ab67c8 into rust-lang:master Sep 15, 2019
@matthewjasper matthewjasper deleted the increase-hygiene-use branch September 19, 2019 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants