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 macro metavars respect (non-)hygiene #68746

Merged
merged 1 commit into from
Mar 17, 2020
Merged

Make macro metavars respect (non-)hygiene #68746

merged 1 commit into from
Mar 17, 2020

Conversation

matthewjasper
Copy link
Contributor

@matthewjasper matthewjasper commented Feb 1, 2020 •
edited
Loading

This makes them more consistent with other name resolution while not breaking any code on crater.

@matthewjasper
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented Feb 2, 2020

⌛ Trying commit 54bcfd60092e9f25afa12deeea9ccccf60fb921c with merge 3738e44c8fffe9e1fdfda9304a4df74ce7b29152...

@bors
Copy link
Contributor

bors commented Feb 2, 2020

☀️ Try build successful - checks-azure
Build commit: 3738e44c8fffe9e1fdfda9304a4df74ce7b29152 (3738e44c8fffe9e1fdfda9304a4df74ce7b29152)

@matthewjasper
Copy link
Contributor Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-68746 created and queued.
🤖 Automatically detected try build 3738e44c8fffe9e1fdfda9304a4df74ce7b29152
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-68746 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Feb 2, 2020
@craterbot
Copy link
Collaborator

🎉 Experiment pr-68746 is completed!
📊 1 regressed and 0 fixed (88601 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Feb 4, 2020
@matthewjasper
Copy link
Contributor Author

Failure looks spurious. Checking perf looks ok
@rust-timer build 3738e44c8fffe9e1fdfda9304a4df74ce7b29152
r? @petrochenkov

@rust-timer
Copy link
Collaborator

Queued 3738e44c8fffe9e1fdfda9304a4df74ce7b29152 with parent 0cbcb17, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 3738e44c8fffe9e1fdfda9304a4df74ce7b29152, comparison URL.

@petrochenkov
Copy link
Contributor

(This will probably has to wait until the next weekend.)

@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 16, 2020 •
edited
Loading

This will have to wait for one more week, unfortunately.
(The change is nontrivial, but not super urgent, so it ends up in in the end of my review queue for the second weekend in a row.)

@petrochenkov
Copy link
Contributor

I'd like to run a slightly different experiment through crater first - #69410.
Marking this as blocked for now.

@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 23, 2020 •
edited
Loading

I'm generally skeptical about applying the term "name resolution" and name resolution techniques to things in macro_rules.

I don't think the current behavior is deliberate, it's probably mostly a consequence of PartialEq and Hash being implemented for Ident.
Other token comparisons in macro_rules deliberately use unhygienic token matching.
Proc macros are also only able to use token matching and name resolution is not available to them.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2020
@petrochenkov petrochenkov mentioned this pull request Feb 23, 2020
Closed
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 8, 2020
@petrochenkov
Copy link
Contributor

I'm not sure what to do here.

  • Comparing full Idents as is (status quo) looks like the worst variant.
    Proc macros should be able to generate arbitrary tokens for macro_rules items without interfering with their variable matching. So we certainly need to strip the "transparent" layers from the compared identifiers.
  • On the other hand, stripping more than only the "transparent" layers breaks code that intentionally relies on this "hygiene" as [experiment] Use unhygienic token matching for identifying macro variables #69410 shows.

Perhaps doing what this PR does is the least evil after all.
By doing it we effectively revert to the status quo before ~Rust 1.30 when we still compared full Idents, but didn't have the transparent layers at all.

@petrochenkov
Copy link
Contributor

I have only one concern regarding the implementation - keeping Idents in the Binders map is pretty fragile because it's easy to forget to "normalize" them with modern_and_legacy().
It would be great to have Binders to keep a comparable newtype around Ident that performs the modern_and_legacy() normalization during its construction from Ident.

I wanted to migrate all ident comparisons to such newtypes since ~2015, I guess, but never got to this task.

@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 Mar 8, 2020
@matthewjasper matthewjasper force-pushed the metahygiene branch 2 times, most recently from 39824fe to d161124 Compare March 11, 2020 20:19
Centril added a commit to Centril/rust that referenced this pull request Mar 12, 2020
@Centril
Rollup merge of rust-lang#68746 - matthewjasper:metahygiene, r=petroc…
39fb6ca 
…henkov

Make macro metavars respect (non-)hygiene

This makes them more consistent with other name resolution while not breaking any code on crater.
@Centril Centril mentioned this pull request Mar 12, 2020
Closed
bors added a commit that referenced this pull request Mar 13, 2020
@bors
Auto merge of #69958 - Centril:rollup-ecgx11p, r=Centril
9757e90 
Rollup of 8 pull requests

Successful merges:

 - #68746 (Make macro metavars respect (non-)hygiene)
 - #69189 (Erase regions in writeback)
 - #69402 (Extend search)
 - #69403 (Implement `Copy` for `IoSlice`)
 - #69460 (Move some `build-pass` tests to `check-pass`)
 - #69802 (fix more clippy findings)
 - #69809 (remove lifetimes that can be elided (clippy::needless_lifetimes))
 - #69949 (triagebot.toml: add ping aliases)

Failed merges:

 - #69589 (ast: `Mac`/`Macro` -> `MacCall`)

r? @ghost
@bors
Copy link
Contributor

bors commented Mar 15, 2020

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 15, 2020
@matthewjasper
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Mar 16, 2020

📌 Commit 0a9a4153433d99f49d5d6e283c234351a8dff73b 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 Mar 16, 2020
@matthewjasper
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Mar 16, 2020

📌 Commit ec86270 has been approved by petrochenkov

Centril added a commit to Centril/rust that referenced this pull request Mar 17, 2020
@Centril
Rollup merge of rust-lang#68746 - matthewjasper:metahygiene, r=petroc…
9d56bb0 
…henkov

Make macro metavars respect (non-)hygiene

This makes them more consistent with other name resolution while not breaking any code on crater.
@Centril Centril mentioned this pull request Mar 17, 2020
Closed
Centril added a commit to Centril/rust that referenced this pull request Mar 17, 2020
@Centril
Rollup merge of rust-lang#68746 - matthewjasper:metahygiene, r=petroc…
34afc76 
…henkov

Make macro metavars respect (non-)hygiene

This makes them more consistent with other name resolution while not breaking any code on crater.
@Centril Centril mentioned this pull request Mar 17, 2020
Closed
@Centril Centril mentioned this pull request Mar 17, 2020
Merged
bors added a commit that referenced this pull request Mar 17, 2020
@bors
Auto merge of #70072 - Centril:rollup-722hooh, r=Centril
660326e 
Rollup of 7 pull requests

Successful merges:

 - #68746 (Make macro metavars respect (non-)hygiene)
 - #69688 (Move tidy check to mingw-check)
 - #69735 (bootstrap: Use hash to determine if sanitizers needs to be rebuilt)
 - #69922 (implement zeroed and uninitialized with MaybeUninit)
 - #69956 (Ensure HAS_FREE_LOCAL_NAMES is set for ReFree)
 - #70061 (Cosmetic fixes in documentation)
 - #70064 (Update books)

Failed merges:

r? @ghost
@bors bors merged commit 8cf9e9e into rust-lang:master Mar 17, 2020
@matthewjasper matthewjasper deleted the metahygiene branch March 17, 2020 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matthewjasper @bors @craterbot @rust-timer @petrochenkov