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

Hygienize macros in the standard library #61629

Merged
merged 1 commit into from
Jun 13, 2019
Merged

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jun 7, 2019

Same as #55597, but for all macros in the standard library.
Nested macro calls will now call what they are intended to call rather than whatever is in the closest scope at call site.
Technically this is a breaking change, so crater run would probably be useful.


One exception that is not hygienized is calls to panic!(...).
Macros defined in libcore do not want to call core::panic.
What they really want to call is either std::panic or core::panic depending on no_std settings.
EDIT: After some thought, recursive calls to panic from panic itself probably do want to use $crate (UPDATE: done).

Calling std::panic from macros defined in std and "whatever panic is in scope" from macros defined in libcore is probably even worse than always calling "whatever panic is in scope", so I kept the existing code.

The only way to do the std/core switch correctly that I'm aware of is to define a built-in panic macro that would dispatch to std::panic or core::panic using compiler magic.
Then standard library macros could delegate to this built-in macro.
The macro could be named panic too, that would fix #61567.
(This PR doesn't do that.)


cc #56389
cc #61567
Fixes #61699
r? @alexcrichton

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

@bors try

@bors
Copy link
Contributor

bors commented Jun 7, 2019

⌛ Trying commit b5e00d8122499c60159d3e0e54017ce8bbc35cc1 with merge e212b7d492c96d3837691ba29d32d5f468933fb8...

@bors
Copy link
Contributor

bors commented Jun 7, 2019

☀️ Try build successful - checks-travis
Build commit: e212b7d492c96d3837691ba29d32d5f468933fb8

@petrochenkov
Copy link
Contributor Author

@craterbot run

@craterbot
Copy link
Collaborator

👌 Experiment pr-61629 created and queued.
🤖 Automatically detected try build e212b7d492c96d3837691ba29d32d5f468933fb8
🔍 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 craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2019
@craterbot
Copy link
Collaborator

🚧 Experiment pr-61629 is now running on agent aws-1.

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

@pietroalbini
Copy link
Member

@petrochenkov wait, does this need to be a cargo test run?

@petrochenkov
Copy link
Contributor Author

@pietroalbini
Yes.
The idea is that user code could previously shadow standard macros with its own macros with the same interface but different behavior.
This PR removes that possibility thus changing runtime behavior.

@matklad
Copy link
Member

matklad commented Jun 9, 2019

Can this replace __rustc_unstable_column with crate::column as well?

https://github.com/rust-lang/rust/blob/b5e00d8122499c60159d3e0e54017ce8bbc35cc1/src/libcore/macros.rs#L12

@petrochenkov
Copy link
Contributor Author

@matklad
column is a built-in macro, not standard library macro, so $crate::column won't work.

To call built-in macros reliably with $crate::mac!(...) we need to reexport them from the standard library first.
With 2018 edition and uniform paths it is generally possible, but there's a bug preventing reexports of built-in macros specifically.

@craterbot
Copy link
Collaborator

🎉 Experiment pr-61629 is completed!
📊 22 regressed and 11 fixed (60951 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 Jun 12, 2019
@alexcrichton
Copy link
Member

This is looking good to me, and the crate run there looks all largely spurious to me, so I think we could probably go ahead and land this?

@petrochenkov
Copy link
Contributor Author

Rebased/squashed.

Recursive calls from panic to itself are also hygienized.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 12, 2019

📌 Commit eb09daa has been approved by alexcrichton

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 12, 2019
@Centril
Copy link
Contributor

Centril commented Jun 12, 2019

@bors rollup

Centril added a commit to Centril/rust that referenced this pull request Jun 12, 2019
Hygienize macros in the standard library

Same as rust-lang#55597, but for all macros in the standard library.
Nested macro calls will now call what they are intended to call rather than whatever is in the closest scope at call site.
Technically this is a breaking change, so crater run would probably be useful.

---

One exception that is not hygienized is calls to `panic!(...)`.
Macros defined in libcore do not want to call `core::panic`.
What they really want to call is either `std::panic` or `core::panic` depending on `no_std` settings.
EDIT: After some thought, recursive calls to `panic` from `panic` itself probably do want to use `$crate` (UPDATE: done).

Calling `std::panic` from macros defined in std and "whatever `panic` is in scope" from macros defined in libcore is probably even worse than always calling "whatever `panic` is in scope", so I kept the existing code.

The only way to do the std/core switch correctly that I'm aware of is to define a built-in panic macro that would dispatch to `std::panic` or `core::panic` using compiler magic.
Then standard library macros could delegate to this built-in macro.
The macro could be named `panic` too, that would fix rust-lang#61567.
(This PR doesn't do that.)

---
cc rust-lang#56389
cc rust-lang#61567
Fixes rust-lang#61699
r? @alexcrichton
Centril added a commit to Centril/rust that referenced this pull request Jun 12, 2019
Hygienize macros in the standard library

Same as rust-lang#55597, but for all macros in the standard library.
Nested macro calls will now call what they are intended to call rather than whatever is in the closest scope at call site.
Technically this is a breaking change, so crater run would probably be useful.

---

One exception that is not hygienized is calls to `panic!(...)`.
Macros defined in libcore do not want to call `core::panic`.
What they really want to call is either `std::panic` or `core::panic` depending on `no_std` settings.
EDIT: After some thought, recursive calls to `panic` from `panic` itself probably do want to use `$crate` (UPDATE: done).

Calling `std::panic` from macros defined in std and "whatever `panic` is in scope" from macros defined in libcore is probably even worse than always calling "whatever `panic` is in scope", so I kept the existing code.

The only way to do the std/core switch correctly that I'm aware of is to define a built-in panic macro that would dispatch to `std::panic` or `core::panic` using compiler magic.
Then standard library macros could delegate to this built-in macro.
The macro could be named `panic` too, that would fix rust-lang#61567.
(This PR doesn't do that.)

---
cc rust-lang#56389
cc rust-lang#61567
Fixes rust-lang#61699
r? @alexcrichton
bors added a commit that referenced this pull request Jun 13, 2019
Rollup of 9 pull requests

Successful merges:

 - #60376 (Stabilize Option::xor)
 - #61398 (Stabilize copy_within)
 - #61629 (Hygienize macros in the standard library)
 - #61675 (Include frame pointer for bare metal RISC-V targets)
 - #61750 (Fix x.py install)
 - #61761 (Add an alias for x86_64-sun-solaris target tuple)
 - #61762 (rustbuild: fix libtest_stamp)
 - #61763 (ci: fix ci stats upload condition)
 - #61776 (Fix typos in error_codes)

Failed merges:

r? @ghost
@bors bors merged commit eb09daa into rust-lang:master Jun 13, 2019
@petrochenkov petrochenkov deleted the stdmac branch June 13, 2019 07:26
@pnkfelix pnkfelix added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 20, 2019
@pnkfelix
Copy link
Member

discussed at T-compiler meeting. Accepting for backport to beta.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 20, 2019
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 22, 2019
bors added a commit that referenced this pull request Jun 26, 2019
[beta] Rollup backports

Rolled up:

* [beta] Comment out dev key #61700

Cherry picked:

* Dont ICE on an attempt to use GAT without feature gate #61118
* Fix cfg(test) build for x86_64-fortanix-unknown-sgx #61503
* Handle index out of bound errors during const eval without panic #61598
* Hygienize macros in the standard library #61629
* Fix ICE involving mut references #61947
* rustc_typeck: correctly compute `Substs` for `Res::SelfCtor`. #61896
* Revert "Set test flag when rustdoc is running with --test option" #61199
* create a "provisional cache" to restore performance in the case of cycles #61754

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Aug 16, 2019
Hygienize use of built-in macros in the standard library

Same as rust-lang#61629, but for built-in macros.

Closes rust-lang#48781
r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
9 participants