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 from stable - macro_rules macros collide with custom derive attributes #53898

Closed
Arnavion opened this issue Sep 2, 2018 · 1 comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name resolution regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.

Comments

@Arnavion
Copy link

Arnavion commented Sep 2, 2018

rustc 1.30.0-nightly (f8d34596f 2018-08-30)
binary: rustc
commit-hash: f8d34596ff74da91e0877212a9979cb9ca13eb7e
commit-date: 2018-08-30
host: x86_64-pc-windows-msvc
release: 1.30.0-nightly
LLVM version: 7.0

On nightly, building https://github.com/Arnavion/derive-error-chain/tree/v0.11.1/derive-error-chain-tests fails with:

error: macro `error_chain` may not be used in attributes
  --> src\main.rs:49:2
   |
49 |     #[error_chain(error = "Error", result_ext = "ResultExt", result = "Result")]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

derive-error-chain is a custom derive that registers an attribute named error_chain

#[proc_macro_derive(ErrorChain, attributes(error_chain))]

If the crate that uses the custom derive also has the regular macro_rules macro named error_chain from the error-chain crate in scope, it fails with that error.

#[macro_use]
extern crate derive_error_chain;
#[macro_use]
extern crate error_chain; // Brings `error_chain!` into scope

#[derive(Debug, ErrorChain)]
#[error_chain(error = "Error", result_ext = "ResultExt", result = "Result")] // Uses the attribute registered by the custom derive.
pub enum ErrorKind {
	Msg(String),
}

Stable 1.28 does not have this problem.

I mentioned this in #38356 (comment) and #38356 (comment). @alexcrichton responded to the second one that it would not be an acceptable regression. Since it fails now without needing to enable any features, I assume this is will regress stable eventually, hence I'm opening this issue.

@petrochenkov
Copy link
Contributor

Duplicate of #53583

@petrochenkov petrochenkov marked this as a duplicate of #53583 Sep 2, 2018
@petrochenkov petrochenkov added A-resolve Area: Name resolution 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 Sep 2, 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 resolution regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

No branches or pull requests

2 participants