resolve: Introduce two sub-namespaces in macro namespace

[petrochenkov]

Two sub-namespaces are introduced in the macro namespace - one for bang macros and one for attribute-like macros (attributes, derives).

"Sub-namespace" means this is not a newly introduced full namespace, the single macro namespace is still in place.
I.e. you still can't define/import two macros with the same name in a single module, use imports still import only one name in macro namespace (from any sub-namespace) and not possibly two.

However, when we are searching for a name used in a ! macro call context (my_macro!()) we skip attribute names in scope, and when we are searching for a name used in attribute context (#[my_macro]/#[derive(my_macro)]) we are skipping bang macro names in scope.
In other words, bang macros cannot shadow attribute macros and vice versa.

For a non-macro analogy, we could e.g. skip non-traits when searching for MyTrait in impl MyTrait for Type { ... }.
However we do not do it in non-macro namespaces because we don't have practical issues with e.g. non-traits shadowing traits with the same name, but with macros we do, especially after macro modularization.

For #[test] and #[bench] we have a hack in the compiler right now preventing their shadowing by macro_rules! test and similar things. This hack was introduced after making #[test]/#[bench] built-in macros instead of built-in attributes (#53410), something that needed to be done from the start since they are "active" attributes transforming their inputs.
Now they are passed through normal name resolution and can be shadowed, but that's a breaking change, so we have a special hack basically applying this PR for #[test] and #[bench] only.

Soon all potentially built-in attributes will be passed through normal name resolution (#53913) and that uncovers even more cases where the strict "macro namespace is a single namespace" rule needs to be broken.
For example, with strict rules, built-in macro cfg!(...) would shadow built-in attribute #[cfg] (they are different things), standard library macro thread_local!(...) would shadow built-in attribute #[thread_local] - both of these cases are covered by special hacks in #53913 as well.
Crater run uncovered more cases of attributes being shadowed by user-defined macros (warn, doc, main, even deprecated), we cannot add exceptions in the compiler for all of them.

Regressions with user-defined attributes like #53583 and #53898 also appeared after enabling macro modularization.

People are also usually confused (#53205 (comment), #53583 (comment)) when they see conflicts between attributes and non-attribute macros for the first time.

So my proposed solution is to solve this issue by introducing two sub-namespaces and thus skipping resolutions of the wrong kind and preventing more error-causing cases of shadowing.

Fixes#53583

[rust-highfive]

r? @matthewjasper

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

[petrochenkov]

r? @alexcrichton
This also should go through FCP in theory.
cc @rust-lang/lang

[petrochenkov]

Some alternatives:

• Separate attributes and derive macros as well.
  This was not done because their conflicts didn't cause any issues in practice and weren't found by crater. Introduction of sub-namespaces is a somewhat forced measure, so it would be nice to keep their number as small as possible. (FWIW, new sub-namespaces can be introduced backward-compatibly if needed.)
• Draw the separation line between something else, e.g. legacy and non-legacy. Non-legacy bang macros can shadow attributes, but legacy macros can't. This is not so well-defined rule - all built-in macros can be migrated to token streams in the future and that would make them non-legacy, but apparently they would still keep legacy shadowing behavior? Weird.

[eddyb]

• Separate attributes and derive macros as well.
  This was not done because their conflicts didn't cause any issues in practice and weren't found by crater.

Is this because derives are typically CamelCase (i.e. a trait name), whereas attributes are snake_case?

[Centril]

@petrochenkov

So, 2 questions:

1. In [nightly regression] rustc incorrectly parses attributes as macro invocations in nightly-2018-08-18 #53583 (comment) you proposed to raise the prio of "derive helper attributes" as compared to other attr macros -- this is orthogonal since they are both forms of attribute macros -- but... what happened that?

2. Does this have any impact on the ability to call bang macros inside attribute macros? e.g. #[foo!(bar)]. I don't think it does, but it would be nice to get that confirmed.

---

@eddyb

Is this because derives are typically CamelCase (i.e. a trait name), whereas attributes are snake_case?

I would expect that to be the reason.

[petrochenkov]

Is this because derives are typically CamelCase (i.e. a trait name), whereas attributes are snake_case?

Most likely.

In #53583 (comment) you proposed to raise the prio of "derive helper attributes" as compared to other attr macros -- this is orthogonal since they are both forms of attribute macros -- but... what happened that?

Yes, that's orthogonal and still worth doing because other issues (#53481) arise due to derive helper attributes being very low-priority right now, but this PR fixes #53583 as well, just in another way.

Does this have any impact on the ability to call bang macros inside attribute macros? e.g. #[foo!(bar)]. I don't think it does, but it would be nice to get that confirmed.

In this code the macro call looks unambiguously "bang" syntactically, and the choice of namespace during resolution is driven by syntax, so this case shouldn't cause any problems.
What this PR prevents is potential ability to call attributes with bang syntax:

my_attr!{fn f(){}}

In theory, such call could can pass the input tokens to the attribute macro in the same way as #[my_attr] fn f() {} does. Not sure why this could be useful though.

[petrochenkov]

cc @dtolnay

[Centril]

Yes, that's orthogonal and still worth doing because other issues (#53481) arise due to derive helper attributes being very low-priority right now, but this PR fixes #53583 as well, just in another way.

I agree.

Not sure why this could be useful though.

So I thought about this a bit and came up with the following example of where it would be useful:

The proptest crate currently has a proptest! { .. } macro which you can call like so:

proptest!{
#[test]fn my_sorting_algo_works(xs:Vec<u8>){
...
}}

Currently, this is just a macro_rules! macro; but one could, in the future, convert this to an attribute macro instead and let users instead write:

#[proptest]fnmy_sorting_algo_works(xs:Vec<u8>){
...}

However, the proptest! macro allowed you to bundle several functions into it; with an attribute macro you can't do that anymore. It would be useful to be able to use #[proptest] both as a bang macro and wrap several functions into it, and as an attribute macro directly on a function.

I suppose that you could do this by putting the attribute on a module (particularly if it has no name, e.g. mod { .. }) or if we allowed blocks at the top level.

[petrochenkov]

@bors p=1
This PR and its follow-up #53913 need to be merged in some form before beta (which is in a few days), or backported to beta later.

[bors]

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

[alexcrichton]

This seems like a solid solution to me and a solid implementation, would someone from @rust-lang/lang like to fcp to merge?

[aturon]

@rfcbot poll lang Please review this PR for merging prior to cutting beta

[rfcbot]

Team member @aturon has asked teams: T-lang, for consensus on:

Please review this PR for merging prior to cutting beta

• @Centril
• @aturon
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @nrc
• @pnkfelix
• @scottmcm
• @withoutboats

[nikomatsakis]

@petrochenkov@Centril

On the topic of using foo! macros in decorator position, I think that we might as well disallow it. In particular, I think that you would want two input streams in decorator position (#[foo(...)] plus the tokens for the decorated thing). I imagine that in some limited cases, like proptest!, it could be handy, but that feels more like a "backwards compat hack" then something you would design to start.

In particular, I imagine that -- had decorators existed -- this is how proptest would have worked. It feels "ok" to me that they can issue a new major version to switch over at some point.

[nrc]

cc @djrenren - does this seem fine for CTFs?

[djrenren]

This looks great! The single macro namespace has been confusing and getting this landed with the stabilization of procmacro attributes seems like the right choice.

[Centril]

@nikomatsakis

In particular, I imagine that -- had decorators existed -- this is how proptest would have worked. It feels "ok" to me that they can issue a new major version to switch over at some point.

Probably yes; #[proptest] is more ergonomic than having to write proptest! { #[test]... } at least.
However, there are some cases where you might have written:

proptest!{
#![proptest_config(ProptestConfig{
fork:true,
timeout:1000,
.. ProptestConfig::default()})]fn first_test(n:u64){ ... }fn second_test(n:u64){ ... }}

This reuses the same configuration for both tests; if you instead put #[proptest] on each fn you have to duplicate the config as well.

With #[proptest], I think you might do this instead:

constCONFIG = ProptestConfig{ fork:true,timeout:1000, ..ProptestConfig::default()};#[proptest(CONFIG)]mod tests {// Ideally we would be able to remove `tests` from here and just write `mod { .. }` instead.fnfirst_test(n:u64){ ...}fnsecond_test(n:u64){ ...}}

which seems just as, if not more ergonomic.

All in all; I think this alternative is good enough to give up the ability to use proptest! as #[proptest], albeit I'm not excited about making a choice. But I'll tick my box :)

[petrochenkov]

I've updated #53913 with a mini-version of this PR that affects only the built-in attribute check and nothing else, so it is now unblocked.
If that PR lands before beta, then this PR becomes quite small and can be easily backported.

[Centril]

@petrochenkov it is a known issue, but file this at https://github.com/anp/rfcbot-rs please. :)

[cramertj]

I've checked my box above, but with the reservation that we need to remember to re-resolve this discussion when it comes time to stabilize macro macros, as I think it'd be nice to leave the door open to implementing attribute-like-macros using the macro-by-example syntax.

[petrochenkov]

it'd be nice to leave the door open to implementing attribute-like-macros using the macro-by-example syntax

I think this door is still open.
It's very easy to put some attribute on a macro item that would reclassify it as an attribute macro in all observable senses.

#[this_is_attr]macro mmm(){ ... }#[mmm]// OKstructS;

[cramertj]

@petrochenkov The question, I suppose, is whether you'd ever want a macro that worked in both positions.

[petrochenkov]

@cramertj
Yep, that's the primary drawback - losing ability to use a single macro in both positions (as also discussed in a few comments above).

[petrochenkov]

Wait, actually no.
Nothing prevents introducing a class of macros that are never skipped during name search and can be used in both ! and #[] positions, they just need to be somehow marked explicitly at def-site:

#[this_is_both_bang_and_attr]macro mac(){ ... }#[mac]// OKstructS;mac!();// OK

In fact proc macro stubs behave exactly like this right now, because I was lazy and didn't refactor MacroKind::ProcMacroStub to keep its macro kind.

[joshtriplett]

I'm a little concerned about doing this halfway, such that shadowing gets ignored but you can't actually have different macros of both types with the same name in the same module. Or have I misunderstood?

[petrochenkov]

@joshtriplett

shadowing gets ignored but you can't actually have different macros of both types with the same name in the same module. Or have I misunderstood?

Yes, that's correct.

I'm a little concerned about doing this halfway

I'd say it's half-bad vs bad in this case, since the primary motivation is compatibility and there's not much need in two new separate namespaces in isolation.

[joshtriplett]

As long as functional proc macros (foo!) and macro_rules macros stay in the same namespace, I can live with derive/attribute macros going in a different namespace, since they have to for compatibility with what has already been released in stable.

(Checked my box.)

[nikomatsakis]

Discussed briefly in the @rust-lang/lang meeting:

General consensus was that we ought to split the namespace for "attribute macros" (aka decorators) like #[foo] from "expression macros" like foo!. That seems cleaner than having one namespace with this kind of unusual special behavior.

However, if this is forwards compatible with that universe -- and we believe it is -- then we should land this now and we can revisit the rest of it later.

[petrochenkov]

@nikomatsakis@Centril
rfcbot poll has consensus, but polls don't start FCP.
Together with #54069 (comment) I assume someone just needs to r+?

[aturon]

@petrochenkov Yep!

@bors r+

[bors]

📌 Commit beb3b5d has been approved by aturon

[bors]

⌛ Testing commit beb3b5d with merge f789b6b...

[bors]

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