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

resolve: Implement prelude search for macro paths, implement tool attributes #52841

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jul 30, 2018

When identifier is macro path is resolved in scopes (i.e. the first path segment - foo in foo::mac!() or foo!()), scopes are searched in the same order as for non-macro paths - items in modules, extern prelude, tool prelude (see later), standard library prelude, language prelude, but with some extra shadowing restrictions (names from globs and macro expansions cannot shadow names from outer scopes). See the comment in fn resolve_lexical_macro_path_segment for more details.

"Tool prelude" currently contains two "tool modules" rustfmt and clippy, and is searched immediately after extern prelude.
This makes the possible long-term solution for tool attributes exactly equivalent to the existing extern prelude scheme, except that --extern=my_crate making crate names available in scope is replaced with something like --tool=my_tool making tool names available in scope.

The tool_attributes feature is still unstable and #![feature(tool_attributes)] now implicitly enables #![feature(use_extern_macros)]. use_extern_macros is a prerequisite for tool_attributes, so their stabilization will happen in the same order.
If use_extern_macros is not enabled, then tool attributes are treated as custom attributes (this is temporary, anyway).

Fixes #52576
Fixes #52512
Fixes #51277
cc #52269

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

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

r? @alexcrichton
cc @nrc @djrenren

Err(Determinacy::Determined)
}
}
WhereToResolve::StdLibPrelude => {
Copy link
Contributor Author

@petrochenkov petrochenkov Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIXME: account for no_implicit_prelude.
Done.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:18:25] 
[01:18:25] failures:
[01:18:25] 
[01:18:25] ---- /checkout/src/doc/unstable-book/src/language-features/tool-attributes.md - tool_attributes::An_example (line 17) stdout ----
[01:18:25] error[E0658]: The attribute `rustfmt::skip` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
[01:18:25]   |
[01:18:25] 4 | #[rustfmt::skip]
[01:18:25]   | ^^^^^^^^^^^^^^^^
[01:18:25]   |
[01:18:25]   |
[01:18:25]   = help: add #![feature(custom_attribute)] to the crate attributes to enable
[01:18:25] thread '/checkout/src/doc/unstable-book/src/language-features/tool-attributes.md - tool_attributes::An_example (line 17)' panicked at 'couldn't compile the test', librustdoc/test.rs:332:13
[01:18:25] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:18:25] 
[01:18:25] 

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@alexcrichton
Copy link
Member

Looks great to me, thanks for the detailed comments!

It looks like tool attributes like #[rustfmt::skip] are still gated by default, and otherwise it looks like this is just fixing bugs, so r=me!

@petrochenkov
Copy link
Contributor Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jul 30, 2018

📌 Commit dd93535e028ece7f01491fd25c8369d375026f69 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 Jul 30, 2018
@Mark-Simulacrum
Copy link
Member

@bors p=1 edition critical

@bors
Copy link
Contributor

bors commented Aug 1, 2018

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout premacro (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self premacro --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging src/test/ui/macro-path-prelude-pass.rs
Auto-merging src/test/ui/macro-path-prelude-fail-1.rs
Auto-merging src/librustc_resolve/macros.rs
CONFLICT (content): Merge conflict in src/librustc_resolve/macros.rs
Auto-merging src/librustc_resolve/lib.rs
Auto-merging src/librustc_resolve/build_reduced_graph.rs
Automatic merge failed; fix conflicts and then commit the result.

@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 Aug 1, 2018
@pietroalbini
Copy link
Member

This is also blocking edition crater runs.

resolve/expansion: Implement tool attributes
@petrochenkov
Copy link
Contributor Author

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Aug 1, 2018

📌 Commit c3e5421 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2018
@bors
Copy link
Contributor

bors commented Aug 1, 2018

⌛ Testing commit c3e5421 with merge e53bca8baf30e0686c3b2b82af3db0be0d592a64...

@pietroalbini
Copy link
Member

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Aug 2, 2018

📌 Commit c3e5421 has been approved by alexcrichton

@pietroalbini
Copy link
Member

@bors p=1

@alexcrichton
Copy link
Member

@bors: rollup-

@bors
Copy link
Contributor

bors commented Aug 2, 2018

⌛ Testing commit c3e5421 with merge 40e4b6e...

bors added a commit that referenced this pull request Aug 2, 2018
resolve: Implement prelude search for macro paths, implement tool attributes

When identifier is macro path is resolved in scopes (i.e. the first path segment - `foo` in `foo::mac!()` or `foo!()`), scopes are searched in the same order as for non-macro paths - items in modules, extern prelude, tool prelude (see later), standard library prelude, language prelude, but with some extra shadowing restrictions (names from globs and macro expansions cannot shadow names from outer scopes). See the comment in `fn resolve_lexical_macro_path_segment` for more details.

"Tool prelude" currently contains two "tool modules" `rustfmt` and `clippy`, and is searched immediately after extern prelude.
This makes the [possible long-term solution](https://github.com/rust-lang/rfcs/blob/master/text/2103-tool-attributes.md#long-term-solution) for tool attributes exactly equivalent to the existing extern prelude scheme, except that `--extern=my_crate` making crate names available in scope is replaced with something like `--tool=my_tool` making tool names available in scope.

The `tool_attributes` feature is still unstable and `#![feature(tool_attributes)]` now implicitly enables `#![feature(use_extern_macros)]`. `use_extern_macros` is a prerequisite for `tool_attributes`, so their stabilization will happen in the same order.
If `use_extern_macros` is not enabled, then tool attributes are treated as custom attributes (this is temporary, anyway).

Fixes #52576
Fixes #52512
Fixes #51277
cc #52269
@bors
Copy link
Contributor

bors commented Aug 2, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 40e4b6e to master...

@bors bors merged commit c3e5421 into rust-lang:master Aug 2, 2018
alexcrichton added a commit to rust-lang/rust-clippy that referenced this pull request Aug 3, 2018
@Manishearth
Copy link
Member

This breaks Servo in a weird way. I'm unable to reproduce this out of tree

https://travis-ci.org/servo/servo-with-rust-nightly/jobs/411851355

error[E0659]: `warn` is ambiguous
   --> ports/servo/glutin_app/window.rs:733:21
    |
733 |                     warn!("Failed to swap window buffers ({}).", err);
    |                     ^^^^
    |
note: `warn` could refer to the name imported here
   --> ports/servo/non_android_main.rs:12:1
    |
12  | #[macro_use] extern crate log;
    | ^^^^^^^^^^^^
note: `warn` could also refer to the name defined here
   --> ports/servo/glutin_app/window.rs:733:21
    |
733 |                     warn!("Failed to swap window buffers ({}).", err);
    |                     ^^^^
    = note: macro-expanded macro imports do not shadow

any idea what's going on? seems like a regression

@Manishearth
Copy link
Member

cc @pietroalbini might want to be careful about that regression before cutting a beta ^^

@petrochenkov
Copy link
Contributor Author

I'll look what happens.

@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 5, 2018
@petrochenkov
Copy link
Contributor Author

@Manishearth
This is an expected breakage.
The general rule sounds like "names from globs and macro expansions cannot shadow names from outer scopes when macro paths are resolved".
Without this rule macro expansion would almost always get stuck in scopes with unexpanded macros or globs without being able to proceed.

In this case macro expanded name (macro_rules macro warn from #[macro_use] extern crate log; inside include!() expansion) shadows a name from outer scope (built-in attribute warn from the language prelude), and this happens during resolution of the macro path warn in warn!("Failed to swap window buffers ({}).", err);.

Minimized reproduction:

macro_rules! my_include {() => {
    #[macro_use] extern crate log;
}}

my_include!();

fn main() {
    warn!("");
}

In this case, since breakage affects stable channel, built-in attributes can be somehow special-cased to avoid the error, or perhaps some more general solution can be found, but I'd prefer to see crater results before proceeding with a fix.

@petrochenkov
Copy link
Contributor Author

Since #53072 doesn't include this PR, we need a separate crater run for it.

@Mark-Simulacrum
Copy link
Member

Queued a crater run for this PR (check only) here: #53089

@petrochenkov petrochenkov removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 5, 2018
@nnethercote
Copy link
Contributor

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 22, 2018
Stabilize a few secondary macro features

- `tool_attributes` - closes rust-lang#44690
- `proc_macro_path_invoc` - this feature was created due to issues with tool attributes (rust-lang#51277), those issues are now fixed (rust-lang#52841)
- partially `proc_macro_gen` - this feature was created due to issue rust-lang#50504, the issue is now fixed (rust-lang#51952), so proc macros can generate modules. They still can't generate `macro_rules` items though due to unclear hygiene interactions.
bors added a commit that referenced this pull request Aug 23, 2018
Stabilize a few secondary macro features

- `tool_attributes` - closes #44690
- `proc_macro_path_invoc` - this feature was created due to issues with tool attributes (#51277), those issues are now fixed (#52841)
- partially `proc_macro_gen` - this feature was created due to issue #50504, the issue is now fixed (#51952), so proc macros can generate modules. They still can't generate `macro_rules` items though due to unclear hygiene interactions.
iliekturtles added a commit to iliekturtles/uom that referenced this pull request Sep 13, 2018
Resolves rustc error E0659 in the base.rs example caused by
[rust-lang/rust](https://github.com/rust-lang/rust) pull request
[#52841](rust-lang/rust#52841) "resolve:
Implement prelude search for macro paths, implement tool attributes."
iliekturtles added a commit to iliekturtles/uom that referenced this pull request Sep 13, 2018
Resolves nightly rustc error E0659 in the base.rs example caused by
[rust-lang/rust](https://github.com/rust-lang/rust) pull request
[#52841](rust-lang/rust#52841) "resolve:
Implement prelude search for macro paths, implement tool attributes."
@petrochenkov petrochenkov deleted the premacro branch June 5, 2019 16:10
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