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

macros: hygienize use of core/std in builtin macros #46550

Merged
merged 4 commits into from
Dec 13, 2017

Conversation

jseyfried
Copy link
Contributor

Today, if a builtin macro wants to access an item from core or std (depending #![no_std]), it generates ::core::path::to::item or ::std::path::to::item respectively (c.f. fn std_path() in libsyntax/ext/base.rs).

This PR refactors the builtin macros to instead always emit $crate::path::to::item here. That is, the def site of builtin macros is taken to be in extern crate core; or extern crate std;. Since builtin macros are macros 1.0 (i.e. mostly unhygienic), changing the def site can only effect the resolution of $crate.

r? @nrc

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2017
@nrc
Copy link
Member

nrc commented Dec 8, 2017

@bors: r+

@bors
Copy link
Contributor

bors commented Dec 8, 2017

📌 Commit 6f8df8c has been approved by nrc

@bors
Copy link
Contributor

bors commented Dec 8, 2017

⌛ Testing commit 6f8df8c with merge f40e75d38b2de81b9cf034d4f8c64b003ae79361...

@bors
Copy link
Contributor

bors commented Dec 8, 2017

💔 Test failed - status-travis

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Dec 9, 2017

📌 Commit d4b7c2c has been approved by nrc

@bors
Copy link
Contributor

bors commented Dec 9, 2017

⌛ Testing commit d4b7c2c963266d22f6661fb0febceba20621788b with merge 504235de64dce04ff0d720bec0a16f2dc8bb55f8...

@bors
Copy link
Contributor

bors commented Dec 9, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Dec 9, 2017

The pretty tests produced identifier paths without its crate (::clone::Clone instead of core::clone::Clone), legit.

...
[01:37:23] ---- [pretty] run-pass/uninit-empty-types.rs stdout ----
[01:37:23] 	
[01:37:23] error: pretty-printed source (expanded) does not typecheck
[01:37:23] status: exit code: 101
[01:37:23] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "-" "-Zno-trans" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/uninit-empty-types.pretty-out" "--target=x86_64-unknown-linux-gnu" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/uninit-empty-types.stage2-x86_64-unknown-linux-gnu.pretty.aux" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers"
[01:37:23] stdout:
[01:37:23] ------------------------------------------
[01:37:23] 
[01:37:23] ------------------------------------------
[01:37:23] stderr:
[01:37:23] ------------------------------------------
[01:37:23] error[E0433]: failed to resolve. Did you mean `core::clone`?
[01:37:23]   --> <anon>:26:8
[01:37:23]    |
[01:37:23] 26 | impl ::clone::Clone for Foo {
[01:37:23]    |        ^^^^^ Did you mean `core::clone`?
[01:37:23] 
[01:37:23] warning: unused import: `std::prelude::v1::*`
[01:37:23]  --> <anon>:4:5
[01:37:23]   |
[01:37:23] 4 | use std::prelude::v1::*;
[01:37:23]   |     ^^^^^^^^^^^^^^^^^^^
[01:37:23]   |
[01:37:23]   = note: #[warn(unused_imports)] on by default
[01:37:23] 
[01:37:23] warning: unused `#[macro_use]` import
[01:37:23]  --> <anon>:5:1
[01:37:23]   |
[01:37:23] 5 | #[macro_use]
[01:37:23]   | ^^^^^^^^^^^^
[01:37:23] 
[01:37:23] error: cannot continue compilation due to previous error
[01:37:23] 
[01:37:23] 
[01:37:23] ------------------------------------------
[01:37:23] 
[01:37:23] thread '[pretty] run-pass/uninit-empty-types.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2770:8
[01:37:23] 
[01:37:23] 
[01:37:23] failures:
[01:37:23]     [pretty] run-pass/associated-types-binding-in-where-clause.rs
[01:37:23]     [pretty] run-pass/deriving-clone-enum.rs
[01:37:23]     [pretty] run-pass/deriving-clone-generic-enum.rs
[01:37:23]     [pretty] run-pass/deriving-clone-generic-struct.rs
[01:37:23]     [pretty] run-pass/deriving-clone-generic-tuple-struct.rs
[01:37:23]     [pretty] run-pass/deriving-clone-struct.rs
[01:37:23]     [pretty] run-pass/deriving-clone-tuple-struct.rs
[01:37:23]     [pretty] run-pass/deriving-enum-single-variant.rs
[01:37:23]     [pretty] run-pass/deriving-in-macro.rs
[01:37:23]     [pretty] run-pass/deriving-meta-multiple.rs
[01:37:23]     [pretty] run-pass/deriving-meta.rs
[01:37:23]     [pretty] run-pass/deriving-via-extension-hash-struct.rs
[01:37:23]     [pretty] run-pass/issue-14399.rs
[01:37:23]     [pretty] run-pass/issue-15689-2.rs
[01:37:23]     [pretty] run-pass/issue-19037.rs
[01:37:23]     [pretty] run-pass/issue-21402.rs
[01:37:23]     [pretty] run-pass/issue-6341.rs
[01:37:23]     [pretty] run-pass/uninit-empty-types.rs
[01:37:23] 
[01:37:23] test result: �[31mFAILED�(B�[m. 2792 passed; 18 failed; 39 ignored; 0 measured; 0 filtered out

@kennytm kennytm 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 Dec 9, 2017
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Dec 11, 2017

📌 Commit 3bade7f has been approved by nrc

@kennytm kennytm 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 Dec 11, 2017
@arielb1
Copy link
Contributor

arielb1 commented Dec 11, 2017

This feels like it's ignoring a bit too many tests, which means that pretty might be very broken. If this is not fixed quickly it could remain broken for quite some time.

@bors r-

@kennytm kennytm 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 Dec 11, 2017
@jseyfried
Copy link
Contributor Author

jseyfried commented Dec 11, 2017

Ok, was going to write fix in separate PR -- I'll make sure to avoid regressing these tests then though.

@jseyfried
Copy link
Contributor Author

jseyfried commented Dec 11, 2017

Pretty is already "broken" for $crate:: paths in the sense that the output doesn't resolve; this PR just makes it worse by using more $crate:: paths.

I can fix the $crate:: case in most cases by using fn eliminate_crate_var (librustc_resolve/macros.rs) as we do for macros 1.1. However, there is still the issue that pretty doesn't play well with hygiene.

@arielb1
Copy link
Contributor

arielb1 commented Dec 12, 2017

@jseyfried

Sure I know about that, I just don't want to let all the pretty tests bitrot before that gets fixed

@jseyfried
Copy link
Contributor Author

jseyfried commented Dec 13, 2017

I avoided regressing any pretty tests by printing $crate::.. paths from builtin macros as ::core::.. or ::std::.. where appropriate.

@bors r=nrc

@bors
Copy link
Contributor

bors commented Dec 13, 2017

📌 Commit 85d19b3 has been approved by nrc

@rust-lang rust-lang deleted a comment from bors Dec 13, 2017
@rust-lang rust-lang deleted a comment from bors Dec 13, 2017
@kennytm kennytm 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 Dec 13, 2017
@kennytm
Copy link
Member

kennytm commented Dec 13, 2017

@bors p=1 — increase priority to unblock #46551.

@bors
Copy link
Contributor

bors commented Dec 13, 2017

⌛ Testing commit 85d19b3 with merge 3dfbc88...

bors added a commit that referenced this pull request Dec 13, 2017
macros: hygienize use of `core`/`std` in builtin macros

Today, if a builtin macro wants to access an item from `core` or `std` (depending `#![no_std]`), it generates `::core::path::to::item` or `::std::path::to::item` respectively (c.f. `fn std_path()` in `libsyntax/ext/base.rs`).

This PR refactors the builtin macros to instead always emit `$crate::path::to::item` here. That is, the def site of builtin macros is taken to be in `extern crate core;` or `extern crate std;`. Since builtin macros are macros 1.0 (i.e. mostly unhygienic), changing the def site can only effect the resolution of `$crate`.

r? @nrc
@bors
Copy link
Contributor

bors commented Dec 13, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 3dfbc88 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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