Skip to content

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

Merged
bors merged 4 commits intorust-lang:masterfrom
jseyfried:cleanup_builtin_hygiene
Dec 13, 2017
Merged

macros: hygienize use of core/std in builtin macros#46550
bors merged 4 commits intorust-lang:masterfrom
jseyfried:cleanup_builtin_hygiene

Conversation

@jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Dec 7, 2017

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

@jseyfried jseyfried mentioned this pull request Dec 7, 2017
Merged
@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

jseyfried commented Dec 9, 2017

@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.

Details 
...
[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

jseyfried commented Dec 11, 2017

@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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
@bors
Auto merge of #46550 - jseyfried:cleanup_builtin_hygiene, r=nrc
3dfbc88 
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...

@bors bors merged commit 85d19b3 into rust-lang:master Dec 13, 2017
@bors bors mentioned this pull request Dec 13, 2017
Merged
@jseyfried jseyfried deleted the cleanup_builtin_hygiene branch December 13, 2017 18:58
@xorl xorl mentioned this pull request Dec 14, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nrc nrc

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

@jseyfried @nrc @bors @kennytm @arielb1