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

Stabilize `use_extern_macros` #50911

Merged
merged 2 commits into from Aug 17, 2018

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented May 20, 2018

Closes #35896

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented May 20, 2018

r? @cramertj

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented May 20, 2018

@rust-highfive rust-highfive assigned kennytm and unassigned cramertj May 20, 2018

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented May 20, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 20, 2018

⌛️ Trying commit 66dfde1 with merge ea32c49...

bors added a commit that referenced this pull request May 20, 2018

Auto merge of #50911 - petrochenkov:macuse, r=<try>
[Do not merge] Stabilize `use_extern_macros`

`![feature(use_extern_macros)]` *changes* import resolution behavior rather than guards it, so the potential for unintended regressions in non-macro imports is relatively high.
We need to pass this through crater and look at those regressions before starting to think about stabilization aka enabling `![feature(use_extern_macros)]` by default.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 20, 2018

☀️ Test successful - status-travis
State: approved= try=True

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented May 20, 2018

cc @rust-lang/infra Crater run (check only) requested.

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented May 21, 2018

Crater run (check only) started.

@aturon aturon referenced this pull request May 22, 2018

Closed

Tracking issue for RFC 1566: Procedural macros #38356

24 of 31 tasks complete
@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented May 24, 2018

Hi @kennytm (crater requester, PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-50911/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented May 24, 2018

6 regressions.

  • verilog: Seems to be spurious ("Read-only file system" cc @pietroalbini).
  • npbot: This PR seems to have broken its use of structopt-^0.2.5 via quicli-0.2.0. Legit.
  • The rest: All due to #[macro_use] no longer needed, which causes #[deny(warnings)] to error out. Legit but expected.
Relevant source code in npbot-1.0.0
[dependencies]
structopt = "0.2.5"
#[macro_use]
extern crate quicli;

// ...

use structopt::StructOpt;
use quicli::prelude::*;

// ...

#[derive(StructOpt)]
pub(crate) struct Opt {
    #[structopt(short = "d", long = "debug", help = "only use the local, println!() sink")]
    debug: bool,
    #[structopt(long = "verbose", short = "v", parse(from_occurrences))]
    verbose: u8,

    // for the SoundTouch source
    #[structopt(long = "soundtouch-host")]
    soundtouch_host: Option<String>,

    // for the last.fm source
    #[structopt(long = "lastfm-api-key")]
    lastfm_api_key: Option<String>,
    #[structopt(long = "lastfm-username")]
    lastfm_username: Option<String>,
}

Note the lack of #[macro_use] extern crate structopt.

Error log
May 22 22:49:57.928 INFO kablam! error[E0658]: The attribute `structopt` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
May 22 22:49:57.928 INFO kablam!   --> src/main.rs:41:5
May 22 22:49:57.928 INFO kablam!    |
May 22 22:49:57.928 INFO kablam! 41 |     #[structopt(short = "d", long = "debug", help = "only use the local, println!() sink")]
May 22 22:49:57.928 INFO kablam!    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
May 22 22:49:57.928 INFO kablam!    |
May 22 22:49:57.929 INFO kablam!    = help: add #![feature(custom_attribute)] to the crate attributes to enable
May 22 22:49:57.929 INFO kablam! 
May 22 22:49:57.929 INFO kablam! error[E0658]: The attribute `structopt` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
May 22 22:49:57.929 INFO kablam!   --> src/main.rs:43:5
May 22 22:49:57.929 INFO kablam!    |
May 22 22:49:57.929 INFO kablam! 43 |     #[structopt(long = "verbose", short = "v", parse(from_occurrences))]
May 22 22:49:57.929 INFO kablam!    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
May 22 22:49:57.929 INFO kablam!    |
May 22 22:49:57.929 INFO kablam!    = help: add #![feature(custom_attribute)] to the crate attributes to enable
May 22 22:49:57.929 INFO kablam! 
May 22 22:49:57.929 INFO kablam! error[E0658]: The attribute `structopt` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
May 22 22:49:57.929 INFO kablam!   --> src/main.rs:47:5
May 22 22:49:57.929 INFO kablam!    |
May 22 22:49:57.929 INFO kablam! 47 |     #[structopt(long = "soundtouch-host")]
May 22 22:49:57.929 INFO kablam!    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
May 22 22:49:57.929 INFO kablam!    |
May 22 22:49:57.929 INFO kablam!    = help: add #![feature(custom_attribute)] to the crate attributes to enable
May 22 22:49:57.929 INFO kablam! 
May 22 22:49:57.929 INFO kablam! error[E0658]: The attribute `structopt` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
May 22 22:49:57.929 INFO kablam!   --> src/main.rs:51:5
May 22 22:49:57.929 INFO kablam!    |
May 22 22:49:57.929 INFO kablam! 51 |     #[structopt(long = "lastfm-api-key")]
May 22 22:49:57.929 INFO kablam!    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
May 22 22:49:57.929 INFO kablam!    |
May 22 22:49:57.929 INFO kablam!    = help: add #![feature(custom_attribute)] to the crate attributes to enable
May 22 22:49:57.929 INFO kablam! 
May 22 22:49:57.930 INFO kablam! error[E0658]: The attribute `structopt` is currently unknown to the compiler and may have meaning added to it in the future (see issue #29642)
May 22 22:49:57.930 INFO kablam!   --> src/main.rs:53:5
May 22 22:49:57.930 INFO kablam!    |
May 22 22:49:57.930 INFO kablam! 53 |     #[structopt(long = "lastfm-username")]
May 22 22:49:57.930 INFO kablam!    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
May 22 22:49:57.930 INFO kablam!    |
May 22 22:49:57.930 INFO kablam!    = help: add #![feature(custom_attribute)] to the crate attributes to enable
May 22 22:49:57.930 INFO kablam! 
May 22 22:49:57.931 INFO kablam! error: aborting due to 5 previous errors
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented May 24, 2018

I'll look what happens with npbot.

MacroBinding::Global(binding)
} else {
return None;
};

if !self.use_extern_macros {
if let Some(scope) = possible_time_travel {

This comment has been minimized.

@petrochenkov

petrochenkov May 27, 2018

Contributor

FIXME: possible_time_travel is now unused

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented May 28, 2018

It looks like npbot legitimately encounters a case of indeterminate resolution when macro expansion cannot progress and generates dummy fragments to recover.

So the error should be something like "cannot determine resolution for the derive macro StructOpt".

However, due to a scary hole in our macro resolution checking such dummy expansions can happen silently without generating any errors.
So we are seeing only secondary errors about unknown structopt attributes instead of the actual error about unresolved StructOpt.
I'll prepare a fix.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented May 29, 2018

Fixed in #51145

@petrochenkov petrochenkov force-pushed the petrochenkov:macuse branch from 66dfde1 to df02c89 May 30, 2018

@petrochenkov petrochenkov changed the title [Do not merge] Stabilize `use_extern_macros` Stabilize `use_extern_macros` May 30, 2018

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented May 30, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 17, 2018

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

@bors bors merged commit 674a5db into rust-lang:master Aug 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 19, 2018

🎊

Thanks so much for helping this over the finish line @petrochenkov!

bors added a commit that referenced this pull request Aug 21, 2018

Auto merge of #53471 - petrochenkov:biattr2, r=oli-obk
resolve: Some macro resolution refactoring

Work towards completing #50911 (comment)

The last commit also fixes #53269 by not using `def_id()` on `Def::Err` and also fixes #53512.

bors added a commit that referenced this pull request Sep 3, 2018

Auto merge of #53913 - petrochenkov:biattr4, r=<try>
resolve: Future proof resolutions for potentially built-in attributes

Based on #53778

This is not full "pass all attributes through name resolution", but a more conservative solution.
If built-in attribute is ambiguous with any other macro in scope, then an error is reported.
TODO: Explain what complications arise with the full solution.

cc #50911 (comment)
Closes #53531

@jojolepro jojolepro referenced this pull request Sep 9, 2018

Closed

Specs-derive #452

bors added a commit that referenced this pull request Sep 10, 2018

Auto merge of #54093 - petrochenkov:noinner, r=alexcrichton
Feature gate non-builtin attributes in inner attribute position

Closes item 3 from #50911 (comment)

bors added a commit that referenced this pull request Sep 11, 2018

Auto merge of #53913 - petrochenkov:biattr4, r=alexcrichton
resolve: Future proof resolutions for potentially built-in attributes

This is not full "pass all attributes through name resolution", but a more conservative solution.
If built-in attribute is ambiguous with any other macro in scope, then an error is reported.

What complications arise with the full solution - #53913 (comment).

cc #50911 (comment)
cc #52269
Closes #53531

bors added a commit that referenced this pull request Sep 17, 2018

Auto merge of #54277 - petrochenkov:afterder, r=alexcrichton
Temporarily prohibit proc macro attributes placed after derives

... and also proc macro attributes used together with `#[test]`/`#[bench]`.

Addresses item 6 from #50911 (comment).

The end goal is straightforward predictable left-to-right expansion order for attributes.
Right now derives are expanded last regardless of their relative ordering with macro attributes and right now it's simpler to temporarily prohibit macro attributes placed after derives than changing the expansion order.
I'm not sure whether the new beta is already released or not, but if it's released, then this patch needs to be backported, so the solution needs to be minimal.

How to fix broken code (derives):
- Move macro attributes above derives. This won't change expansion order, they are expanded before derives anyway.

Using attribute macros on same items with `#[test]` and `#[bench]` is prohibited for similar expansion order reasons, but this one is going to be reverted much sooner than restrictions on derives.

How to fix broken code (test/bench):
- Enable `#![feature(plugin)]` (don't ask why).

r? @ghost
@softprops

This comment has been minimized.

Copy link

softprops commented Oct 26, 2018

The 1.30.0 release notes for #[macro_export(local_inner_macros)] link here.

Where can I find more documentation on this feature?

So far I've tried local_inner_macros in two cases where a crate's macros depended on anothers and it didn't work in either case.

ilianaw/rust-crowbar#46 (comment)

dtolnay/mashup#16

@petrochenkov

This comment has been minimized.

@softprops

This comment has been minimized.

Copy link

softprops commented Oct 26, 2018

@petrochenkov thanks! I'll take a look and report back if it looks like what I'm trying to do, re-export transitive dependency crates macros` is actually what this new feature supports.

@softprops

This comment has been minimized.

Copy link

softprops commented Oct 26, 2018

@petrochenkov l need to do a bit more testing but I worked it out, at least enough to get a fork of rust crowbar to compile. I still need to test a crate that uses that fork before I can call this 👍 'd

It may not be clear to new rust users that for foreign macros, you'll also need namespace the macro's path. In my case I just needed to prefix crowbars use of cpython's macros with cpython:: in the docs you linked to and users will find you only use local macros in examples using $crate::. Assuming macros authors are already going to be a bit more advanced than the beginner rust users they may be able to discover this for themselves. I'd recommend adding at least one foreign macro example for the sake of beginner/advanced user inclusivity. Albeit this would be much more painful for the example of supporting old rust versions because if forces users to copy and paste the entire macro srcs from other crates. The format_args is probably the simplest case for a macro. A more complex examples will be much more awkward. For now, asking users to use the latest stable works well enough!

@softprops

This comment has been minimized.

Copy link

softprops commented Oct 26, 2018

found a potential bug but I'm not sure if its by design

when experimenting with the $crate:: path prefix with #[macro_export(local_inner_macros)] and with #[macro_export] I found this interesting inconsistency gotcha with the way that exported macro can be used.

with #[macro_export(local_inner_macros)] I can do the following

extern crate crowbar;
crowbar::lambda!(...)

however with #[macro_export] I get the following here using the same path references

extern crate crowbar;
crowbar::lambda!(...)
error: cannot find macro `lambda!` in this scope

interestingly though I can do the following with #[macro_export]

extern crate crowbar;
use crowbar::lambda;
lambda!(...)

this feels a bit inconsistent in the way the newer system attempts to line up with what you can do with path references for non macro items.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 26, 2018

@softprops
I suspect that the lambda macro is recursive and calls itself by relative name (lambda!(...)), in other words, it's its own helper.

This way the first iteration crowbar::lambda!(...) is resolved successfully, but the next iterations cannot be resolved without bringing lambda in scope at use-site or using local_inner_macros at def-site.

@softprops

This comment has been minimized.

Copy link

softprops commented Oct 26, 2018

@petrochenkov 100%. that was it. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment