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

[nightly regression] rustc incorrectly parses attributes as macro invocations in nightly-2018-08-18 #53583

Closed
jchlapinski opened this issue Aug 22, 2018 · 17 comments
Assignees
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name/path resolution done by `rustc_resolve` specifically P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@jchlapinski
Copy link

jchlapinski commented Aug 22, 2018

In the newest nightly all attributes with names identical to existing macros are parsed as macro invocation (note the lack of ! at the front of dummy in the example below).
Previous version nightly-2018-08-17 works as expected.
This breaks compilation of some of our custom derives, using same name for attributes and macros, which was previously allowed.

I tried this code:

macro_rules! dummy {
    ($x:expr) => {
        println!("dummy: {}", $x)
    }
}

#[dummy("text")] // this should not be regarded as a `dummy` macro invocation, should it?
struct SampleStruct {

}

fn main() {
    dummy!("text");
}

I expected this to complain about unknown custom attribute dummy

Instead, I got this error:

error: macro `dummy` may not be used in attributes
 --> src/main.rs:7:1
  |
7 | #[dummy("text")]
  | ^^^^^^^^^^^^^^^^

Meta

rustc --version --verbose:

rustc 1.30.0-nightly (33b923fd4 2018-08-18)
binary: rustc
commit-hash: 33b923fd44c5c5925e635815fce68bdf1f98740f
commit-date: 2018-08-18
host: x86_64-unknown-linux-gnu
release: 1.30.0-nightly
LLVM version: 7.0
@jchlapinski jchlapinski changed the title [nightly regression] rustc incorrectly parses attributes as macro invocations in nighlty-2018-08-18 [nightly regression] rustc incorrectly parses attributes as macro invocations in nightly-2018-08-18 Aug 22, 2018
@petrochenkov petrochenkov self-assigned this Aug 22, 2018
@petrochenkov petrochenkov added A-resolve Area: Name/path resolution done by `rustc_resolve` specifically A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Aug 22, 2018
@petrochenkov
Copy link
Contributor

Currently this is expected behavior - fn-like macros, attribute macros and derive macros share single namespace and can shadow each other (similarly to types being able to shadow traits etc).
See #53205 (comment) for some motivation.

This change shouldn't affect stable Rust since attribute macros were stabilized only few days ago.

@jchlapinski
Copy link
Author

... fn-like macros, attribute macros and derive macros share single namespace and can shadow each other (similarly to types being able to shadow traits etc).

Ok, but how is this #[dummy("aaa")] a "attribute macro" instead of just an fn-like attribute? My understanding is that in order for it to be an attribute macro it should read #[dummy!("aaa")], no?

@nikomatsakis
Copy link
Contributor

@jchlapinski can you say a bit more about the actual code that is breaking for you?

cc @rust-lang/lang

@nikomatsakis nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 23, 2018
@nikomatsakis
Copy link
Contributor

Flagging this for @rust-lang/lang discussion -- do we want all macro_rules and other macros to be usable as macros in attribute position? It's certainly something we've talked about, but I thought it'd be good to "check in" on this.

@nikomatsakis
Copy link
Contributor

In particular, if this is regressing code, it may not be something we can't really do. (Especially stable code -- it's still not clear to me just what the regressed code looks like.)

@Centril
Copy link
Contributor

Centril commented Aug 23, 2018

The error message seems a bit confusing as I think it leads the user to believe that dummy was called inside the attribute (so it would have been expanded to #[println!("dummy: {}", "text")].

If you actually wanted that behavior then the invocation should have been #[dummy!("text")] which may be useful for some sort of attribute-aliasing. However; I think such a step would require some more thinking into the design and possible alternatives, etc.

However, I think @petrochenkov's reasoning is right; this is not a stable-to-nightly regression because attribute macros were not previously stable. If you use an attribute name with the same name as a macro_rules macro, then the compiler is not to blame here imo.

See #53583 (comment) for why it is a stable-to-nightly-regression. @petrochenkov's solution seems like a good one provided that attribute proc macros can be renamed to deal with conflicts.

@jchlapinski
Copy link
Author

@nikomatsakis basically we have custom derive for our diagnostic type trait Diag, which is using an fn-like attribute named diag and also a macro (defined with macro_rules) with that same name. Macro is for instantiation of out diagnostic type, where we do some backtrace generation, logging, etc.

Now we can of course change either name, however I still do not understand why using same name for attribute AND a macro would clash? By that same logic I can have a field and a method with the same name, since field access and method calls are easily discerned in parsing. Is there a different way to expand a macro, without suffixing macro name with a !?

@jchlapinski
Copy link
Author

jchlapinski commented Aug 23, 2018

@Centril I am not blaming, I am just reasoning on why those things are sharing the same namespace. In my mind an attribute is something completely different than a macro. same logic for fields and methods as in my comment above.

@Centril
Copy link
Contributor

Centril commented Aug 23, 2018

@jchlapinski

@Centril I am not blaming, I am just reasoning on why those things are sharing the same namespace.

Oh; I didn't mean that you are blaming the compiler devs for breaking your code or something, just that I don't think there has been breakage of our stability promises in this case :)

@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 23, 2018

basically we have custom derive for our diagnostic type trait Diag, which is using an fn-like attribute named diag and also a macro (defined with macro_rules) with that same name.

Ah, I see this is an inert attribute white-listed by a derive macro, not an attribute macro.
Then this is indeed a regression affecting stable.

My preferred solution would be to increase priority of derive helper attributes during resolution (item 6 in #50911 (comment)), rather than introducing more namespaces.

macro diag() {}

#[derive(Diag)] // introduces inert attribute `diag` into scope that shadows `diag`s from outer scopes
#[diag] // refers to the inert attribute
struct S;

@jchlapinski
Copy link
Author

@petrochenkov In short, thanks, that solution would solve our problem, and of course it is one way to do it.

But please, if I could take a few more minutes of Your time, explain why You keep referring to attributes, written as #[attribute(...)] as "attribute macros"? Are those really "macros" in rust? A macro is something that gets expanded into code in place it is invoked (with !), exactly as macro_rules defined macros work. An attribute is just an annotation without any inherent influence on the executable code, unless something will use that annotation to generate some code (like for example custom derive). As such those thing (attributes and macros) are completely different and should not shadow one another in any way, regardless whether macros can be expanded in attributes or not. Am I missing something?

@jchlapinski
Copy link
Author

@petrochenkov Nevermind, please ignore. The magic name "inert attribute" have provided me with some results to read on. Thanks

@nikomatsakis
Copy link
Contributor

@jchlapinski I know you said ignore, but I'll just expand briefly ;) in short, yes, we are aiming to allow macros to be used in those positions which would edit or alter the struct definition and expand it. #[derive] is one such macro, but it happens to not modify the struct definition but rather just produce extra items alongside (impls, specifically). Think "decorators" in Python, in any case.

@nikomatsakis
Copy link
Contributor

That said, I myself am not entirely clear on the current contours of this design. So perhaps I will pose a few questions that I would like clarified:

  • can you actually use a macro-rules macro in this position, or is it an error if a decorator resolves to a macro-rules macro?
    • presumably proc macros can be used
  • can macros beyond derive macros add these "whitelisted attributes"?

@nrc
Copy link
Member

nrc commented Aug 23, 2018

do we want all macro_rules and other macros to be usable as macros in attribute position?

No, I don't think so. The two kinds of macros have different 'signatures' so whether decl or proc, they have to be used differently. However, they do share the same namespace.

can you actually use a macro-rules macro in this position, or is it an error if a decorator resolves to a macro-rules macro?

"This position" is item position? In which case it should be fine. I believe with new proc macros there is no distinction between decorators and modifiers, and I don't care either way about old proc macros.

can macros beyond derive macros add these "whitelisted attributes"?

I believe this is currently a special attribute of derive macros. Real proc macros have more power since they consume the input to the macro, they can decide what to allow or disallow.

@petrochenkov
Copy link
Contributor

Fixed in #54069

@pnkfelix
Copy link
Member

visited for triage. Seems under control from looking at PR #54069. P-high.

@pnkfelix pnkfelix added the P-high High priority label Sep 13, 2018
bors added a commit that referenced this issue Sep 14, 2018
resolve: Introduce two sub-namespaces in macro namespace

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name/path resolution done by `rustc_resolve` specifically P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants