Merge proc_macro_ expansion feature gates as proc_macro_hygiene

[jebrosen]

Merges proc_macro_mod, proc_macro_expr, proc_macro_non_items, and proc_macro_gen into a single feature: proc_macro_hygiene. These features are not all blocked on implementing macro hygiene per se, but rather on interactions with hygiene that have not been entirely resolved.

[rust-highfive]

r? @nikomatsakis

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

[rust-highfive]

The job x86_64-gnu-llvm-3.9 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.

travis_time:start:tidy
tidy check
[00:04:13] * 553 error codes
[00:04:13] * highest error code: E0710
[00:04:13] Expected a gate test for the feature 'macros_2'.
[00:04:13] Hint: create a failing test file named 'feature-gate-macros_2.rs'
[00:04:13]       in the 'ui' test suite, with its failures due to
[00:04:13]       missing usage of #![feature(macros_2)].
[00:04:13] Hint: If you already have such a test and don't want to rename it,
[00:04:13]       you can also add a // gate-test-macros_2 line to the test file.
[00:04:13] tidy error: Found 1 features without a gate test.
[00:04:14] some tidy checks failed
[00:04:14] 
[00:04:14] 
[00:04:14] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:14] 
[00:04:14] 
[00:04:14] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:14] Build completed unsuccessfully in 0:00:54
[00:04:14] Build completed unsuccessfully in 0:00:54
[00:04:14] Makefile:79: recipe for target 'tidy' failed
[00:04:14] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:08b03d28
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:014e3d1a:start=1530922926684824381,finish=1530922926691254464,duration=6430083
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0032d840
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:023af003
$ dmesg | grep -i kill

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)

[Mark-Simulacrum]

Cc @alexcrichton

[alexcrichton]

Hm I'd personally prefer to leave these as smaller feature gates in the sense that we started as one large proc_macro feature gate but it wasn't realistic to stabilize everything all at once so we scaled it back. Each feature indicates something that will likely require a separate RFC and/or separate discussion to stabilize, and otherwise bucketing all of them gives either the false impression of "when anything is stabilized everything is stabilized" or that there's so much functionality inside it'll never be stable

[SergioBenitez]

@alexcrichton Continuing down this path makes both developing and using crates with proc-macros (with their full abilities) very painful. For instance, once Rocket migrates to using proc-macros universally, users will need to enable at least four of these features to use Rocket at all. This is, in fact, the impetus for this PR.

Accepting this PR has no implications on whether everything has to be stabilized at once or not. All of the existing features will continue to exist, and you can continue to subset features into different gates. Crates asking users to enable 'macros_2' can stop doing so as soon as the subset of features the crate requires are stable.

I see no downsides to this PR, aside from needing to maintain the feature flag, but I do a see a strong win in terms of usability for both end-users and developers.

[alexcrichton]

@SergioBenitez I agree from an author's point of view that it's much easier to just write macro_2, but it's also much easier to purely write #![nightly] and get everything as well! There's a line somewhere at "where should the granularity of opt-in be" and I personally feel like "by unit of stabilization" makes the most sense in terms of signal to noise. It provides signal to us of what to stabilize as well as signal to users as to how many features they're actually opting-in to. It can indeed look like noise if you're opting in to a ton of features, but I see what as somewhat intentional personaly.

In any case though @nikomatsakis is listed for review here, and mine isn't the only opinion!

[jebrosen]

My concern with making the granularity of opt-in "by unit of stabilization" is that those units keep changing. For example, last time I tried it locally, #52081 broke proc_macro2, apparently only by adding the proc_macro_diagnostic feature. If the additional features were documented I wouldn't be bothered so much, but most of the features point to #38356 where there is usually no discussion, tracking, or explanation of them.

[alexcrichton]

@jebrosen that breakage (and recent breakage) has been from splitting up the proc_macro feature, which I don't think has happened a huge amount historically (FWIW)

[bors]

☔ The latest upstream changes (presumably #52081) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

r? @alexcrichton

[nikomatsakis]

In any case though @nikomatsakis is listed for review here, and mine isn't the only opinion!

Heh, not any more! 😛

[mikeyhew]

@alexcrichton

I agree from an author's point of view that it's much easier to just write macro_2, but it's also much easier to purely write #![nightly] and get everything as well! There's a line somewhere at "where should the granularity of opt-in be" and I personally feel like "by unit of stabilization" makes the most sense in terms of signal to noise. It provides signal to us of what to stabilize as well as signal to users as to how many features they're actually opting-in to. It can indeed look like noise if you're opting in to a ton of features, but I see what as somewhat intentional personally.

Writing #![nightly] seems like a far cry from what is being proposed here. In this case, all of the features are part of a single user-facing feature — or maybe two features, procedural macros and declarative macros. I guess maybe all of those features mean something to whoever implemented them in the compiler, but for anyone trying to use the new macro stuff, either for Rocket, or just to test them out, they're some complicated incantation that you have to write at the top of your crate.

By the way, can someone explain what all of those features do? None of them have explanations in the unstable book, and all six of the proc_macro ones point to the same tracking issue.

[alexcrichton]

Yes I'm not saying that this PR is equivalent to #![nightly], only pointing out that there's a spectrum of possibilities here and it's not clear where we should fall. Clearly #![nightly] is not granular enough and this PR is proposing that the current units are too granluar.

I think one of the problems is actually pointed out by @mikeyhew in that naively we should have a feature per "user facing feature" but these are all very different user facing features:

• proc_macro_diagnostic - this is the runtime support in the proc_macro crate for producing diagnostics. This is blocked on designing and stabilizing the API here.
• proc_macro_quote - this is the quote! macro provided by the proc_macro crate. This is blocked on stabilizing the macro and/or deciding whether to include it.
• proc_macro_span - this is a suite of APIs to introspect a Span in proc_macro and inspect various fields like byte position, parent, etc. (also change resolution/hygiene). This is blocked on designing and stabilizing the API here.
• use_extern_macros - this enables macros to be used with the module system, using use statements to import them. This is blocked on fixing final bugs (but should be stable soon).
• proc_macro_path_invoc - this enables multi-segment invocations inside of attributes like #[foo::bar]. This is blocked on designing the semantics about how paths work with respect to later tokens in the attribute and what goes where.
• proc_macro_mod - this enables procedural macros to generate modules. This was blocked on making hygiene "make sense" in some situations, and may be fixed now (but we'd want an explicit decision to ungate it).
• proc_macro_expr - this gates invoking procedural macros in expression position. This is blocked on larger scale hygiene changes to make this feasible for the ecosystem at large.
• proc_macro_non_items - this prevents procedural macros from expanding to non-item items (like expressions and statements). This is blocked on the same thing as proc_macro_expr and can probably be folded in with it.
• proc_macro_gen - this, like proc_macro_mod, gates modules being generated, and can probably be merged with proc_macro_mod
• decl_macro - this gates the macro keyword, and is blocked on a whole bunch of things.

My point is that almost all of these are entirely separate APIs and actually entirely separate user-facing features. A small handful can be merged but I don't think it's useful to lump these all into one feature. It's basically an impossibility for these to all be stabilized at once.

[jebrosen]

I looked more closely at the individual gates and have some more thoughts on this. It ended up being a bit of a brain dump instead of any particular argument, so bear with me.

For a macro crate author: proc_macro_diagnostic, proc_macro_qoute, and proc_macro_span are proc_macro library features that a proc_macro crate must enable in order to use them. decl_macro must be enabled to define decl macros. I think it's probably fair to require macro crate authors to list these out separately, but it will be frustrating if more changes happen to them.

For someone using a proc_macro crate, things are a lot muddier. use_extern_macros is relatively understandable and hopefully stabilizing soon. proc_macro_path_invoc appears to be a style choice and opt-in for the macro consumer. But the requirements for proc_macro_mod, proc_macro_expr, proc_macro_non_items, and proc_macro_gen are more likely imposed on a consumer crate because of something in the macro crate.

According to my understanding of the errors defined in libsyntax/ext/expand.rs and what activates them:

• proc_macro_mod - allows applying proc_macro attributes to a module.
• proc_macro_expr - allows applying proc_macro attributes to a statement or expression.
• proc_macro_non_items - allows expanding proc_macros to non-items (expressions, patterns, types, or statements). This certainly matters for bang-style/inline macros; I can't think off the top of my head what kind of attribute macro would need to do this.
• proc_macro_gen - allows expanding proc_macros to anything containing a module or a macro definition.

Whether these gates are necessary or not is always (usually?) determined by the macro, but the consumer has to enable the features that support the macros they happen to use. For example, if Rocket's macros were all reimplemented today to perform exactly as the current plugin-based ones, I believe most application crates would need proc_macro_non_items and proc_macro_gen (in addition to use_extern_macros and decl_macro).

Side note: if use_extern_macros stabilizes, I suspect consumer crates might not need decl_macro in order to use a decl_macro.

[alexcrichton]

I think that all makes sense to me @jebrosen! One possibility that may imply to me is that we could have fine-grained gates for definitions of procedural macros but not for the invocation of procedural macros. That should help improve the user experience I believe?

[jebrosen]

I think I have to take back what I said about macros implying the gates they need, at least in general.
For example, consider a macro that replaces every "foo" with "bar" when it appears in any Ident. The answer to "which feature gates have to be enabled to use this macro?" is "it depends on the macro input, which is up to the macro consumer".

On the other hand, Rocket's current collection of macros are all targeted at specific use cases: expanding to a vec! call with some name replacement, expanding to other macro calls, attributes that expand to item and/or macro definitions, etc. For these macros, the question "which feature gates have to be enabled to use this macro?" mostly have a clear answer on a case-by-case basis.

From a "wants" perspective, it might be nice for macro authors to be able to opt-in to allow proc_macro_non_items on individual macro definition for macros that are intended to be used that way, and also for macro users to opt-in to proc_macro_non_items in other instances. But that might be too unwieldy to implement to be worth it. It also slightly erodes the sense of "users realize what they're getting into when they enable all these features", but on the other hand downstream crates don't usually have to enable features that upstream crates use anyway.

[SergioBenitez]

I think that all makes sense to me @jebrosen! One possibility that may imply to me is that we could have fine-grained gates for definitions of procedural macros but not for the invocation of procedural macros. That should help improve the user experience I believe?

That sounds great to me.

[alexcrichton]

@jebrosen it's true yeah that a macro's definition doesn't always imply where it's going to be expanded, but as you've found I think it's safe to assume that most macros have a specific context they're expected to expand to, so I think a lot of macros could benefit from the strategy of "the macro is tagged with features it can expand to"

[SergioBenitez]

@alexcrichton Do I understand correctly that you would be amenable to merging a PR that:

1. Requires proc-macro crate authors to enable fine-grained features like proc_macro_non_items, proc_macro_gen, and decl_macro in order to use the corresponding functionality.
2. Does not require consumers of proc-macro crates to enable detailed features. Instead, consumer crates can enable macros_2 to use any proc-macro or decl-macro.

[alexcrichton]

@SergioBenitez correct yeah! Something like that I think would reasonably inform users about the dangers of using all these features while hopefully not being too cumbersome as well

[jebrosen]

I'm trying another formulation of this:

macros_2 enables Macros 2.0 features that would otherwise need to be enabled individually on macro consumer crates "because an error message said I had to"

• proc_macro_mod, proc_macro_gen, proc_macro_non_items, proc_macro_expr: Consumer gate, but often necessitated by a particular proc_macro. Included in the macros_2 feature.
• proc_macro_path_invoc: Consumer gate, but no proc_macro should necessitate its use. Not included.
• proc_macro_diagnostic, proc_macro_quote, proc_macro_span: proc_macro crates can opt-in to these unstable library APIs. Not included.
• decl_macro: Only the defining crate needs the feature; consumer crates are already covered by the stabilization of use_extern_macros. Not included.

Note that this proposal does not even touch on "tagging" macros with whitelisted expansion types, which I think is a better ideal solution but is also unreasonably complex to implement in the compiler.

Under this proposal, Rocket application crates would need to enable #![feature(macros_2, decl_macro)], which is shorter than both the ~4 individual proc_macro features and over the current compiler plugin incantations. Rocket's current plugin-based macros expand to decl_macro definitions, which is why the consumer crates need decl_macro in this case.

@SergioBenitez how do you feel about leaving decl_macro out for these reasons?

[petrochenkov]

After #53459 lands, it'll probably no longer worth it introducing an umbrella feature on the consumer side.

[jebrosen]

As far as I can tell, #53459 doesn't address any of the features imposed on consumer crates by the macro author, which is what this feature request has become. Only proc_macro_gen is being partially stabilized; proc_macro_gen for macro generation, proc_macro_mod, proc_macro_non_items, and proc_macro_expr are still gated.

[petrochenkov]

Could you move the features into the "removed" list instead of just removing, and also add "subsumed by proc_macro_hygiene" as the removal reason?

[petrochenkov]

@bors r-
#52121 (comment)

[bors]

💔 Test failed - status-travis

[rust-highfive]

The job dist-x86_64-apple-alt 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.

[00:03:41]       Memory: 8 GB
[00:03:41]       Boot ROM Version: VMW71.00V.0.B64.1704110547
[00:03:41]       Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
[00:03:41]       SMC Version (system): 2.8f0
[00:03:41]       Serial Number (system): VMP9tpMcNrQz
[00:03:41] 
[00:03:42] hw.ncpu: 4
[00:03:42] hw.byteorder: 1234
[00:03:42] hw.memsize: 8589934592
---
uploading "74503eea294e92cb93055163eb77f1d1c4d40dea/rust-nightly-x86_64-apple-darwin.pkg" with {:content_type=>"application/octet-stream", :acl=>"public-read"}
uploading "74503eea294e92cb93055163eb77f1d1c4d40dea/rust-docs-nightly-x86_64-apple-darwin.tar.xz" with {:content_type=>"application/x-xz", :acl=>"public-read"}
uploading "74503eea294e92cb93055163eb77f1d1c4d40dea/rust-docs-nightly-x86_64-apple-darwin.tar.gz" with {:content_type=>"application/gzip", :acl=>"public-read"}
travis_fold:end:dpl.3
/Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/seahorse/client/plugins/raise_response_errors.rb:15:in `call': Encountered a `SocketError` while attempting to connect to: (Aws::Errors::NoSuchEndpointError)
  https://rust-lang-ci2.s3.us-west-1.amazonaws.com/rustc-builds-alt/74503eea294e92cb93055163eb77f1d1c4d40dea/rust-docs-nightly-x86_64-apple-darwin.tar.xz
This is typically the result of an invalid `:region` option or a
poorly formatted `:endpoint` option.
* Avoid configuring the `:endpoint` option directly. Endpoints are constructed
  from the `:region`. The `:endpoint` option is reserved for connecting to
  non-standard test endpoints.
* Not every service is available in every region.
* Never suffix region names with availability zones.
  Use "us-east-1", not "us-east-1a"
Known AWS regions include (not specific to this service):
ap-northeast-1
ap-northeast-2
ap-south-1
ap-southeast-1
ap-southeast-2
ca-central-1
eu-central-1
eu-west-1
eu-west-2
eu-west-3
sa-east-1
us-east-1
us-east-2
us-west-2
cn-north-1
cn-north-1
cn-northwest-1
us-gov-west-1
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/aws-sdk-core/plugins/s3_sse_cpk.rb:19:in `call'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/aws-sdk-core/plugins/s3_dualstack.rb:24:in `call'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/aws-sdk-core/plugins/s3_accelerate.rb:34:in `call'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/aws-sdk-core/plugins/jsonvalue_converter.rb:20:in `call'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/aws-sdk-core/plugins/idempotency_token.rb:18:in `call'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/aws-sdk-core/plugins/param_converter.rb:20:in `call'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/seahorse/client/plugins/response_target.rb:21:in `call'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/seahorse/client/request.rb:70:in `send_request'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-core-2.11.140/lib/seahorse/client/base.rb:207:in `block (2 levels) in define_operation_methods'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-resources-2.11.140/lib/aws-sdk-resources/services/s3/file_uploader.rb:42:in `block in put_object'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-resources-2.11.140/lib/aws-sdk-resources/services/s3/file_uploader.rb:49:in `open_file'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-resources-2.11.140/lib/aws-sdk-resources/services/s3/file_uploader.rb:41:in `put_object'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-resources-2.11.140/lib/aws-sdk-resources/services/s3/file_uploader.rb:34:in `upload'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/aws-sdk-resources-2.11.140/lib/aws-sdk-resources/services/s3/object.rb:252:in `upload_file'
 from /Users/travis/.rvm/gems/ruby-2.4.2/gems/dpl-s3-1.10.2/lib/dpl/provider/s3.rb:114:in `block (2 levels) in upload_multithreaded'
failed to deploy

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)

[rust-highfive]

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.

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)

[SergioBenitez]

@petrochenkov Looks like we're good.

[bors]

☔ The latest upstream changes (presumably #54767) made this pull request unmergeable. Please resolve the merge conflicts.

[petrochenkov]

@bors r+

[bors]

📌 Commit d3c902f has been approved by petrochenkov

[bors]

⌛ Testing commit d3c902f with merge 766e21c...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 766e21c to master...

[rust-highfive]

📣 Toolstate changed by #52121!

Tested on commit 766e21c.
Direct link to PR: #52121

💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → test-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).