Skip to content

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
Collaborator

bors commented Dec 8, 2017

📌 Commit 6f8df8c has been approved by nrc

@bors
Copy link
Collaborator

bors commented Dec 8, 2017

⌛ Testing commit 6f8df8c with merge f40e75d38b2de81b9cf034d4f8c64b003ae79361...

@bors
Copy link
Collaborator

bors commented Dec 8, 2017

💔 Test failed - status-travis

@jseyfried jseyfried force-pushed the cleanup_builtin_hygiene branch from 6f8df8c to d4b7c2c Compare December 9, 2017 00:43
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Collaborator

bors commented Dec 9, 2017

📌 Commit d4b7c2c has been approved by nrc

@bors
Copy link
Collaborator

bors commented Dec 9, 2017

⌛ Testing commit d4b7c2c963266d22f6661fb0febceba20621788b with merge 504235de64dce04ff0d720bec0a16f2dc8bb55f8...

@bors
Copy link
Collaborator

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 jseyfried force-pushed the cleanup_builtin_hygiene branch from d4b7c2c to 3bade7f Compare December 11, 2017 06:09
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Collaborator

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 jseyfried force-pushed the cleanup_builtin_hygiene branch from 3bade7f to 57f373f Compare December 12, 2017 21:15
@jseyfried jseyfried force-pushed the cleanup_builtin_hygiene branch from 57f373f to 85d19b3 Compare December 13, 2017 06:32
@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
Collaborator

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
Collaborator

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
Collaborator

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