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

Regression importing log’s warn! macro inside of include!()ed file #53205

Closed
SimonSapin opened this issue Aug 8, 2018 · 4 comments · Fixed by #53270
Closed

Regression importing log’s warn! macro inside of include!()ed file #53205

SimonSapin opened this issue Aug 8, 2018 · 4 comments · Fixed by #53270
Assignees
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

Cargo.toml

[package]
name = "foo"
version = "0.0.0"

[dependencies]
log = "0.4"

src/main.rs

include!("real_main.rs");

src/real_main.rs

#[macro_use] extern crate log;

fn main() {
    warn!("!!!");
}

Code like this used to build correctly:

$ rustup run --install nightly-2018-08-02 rustc -V 
rustc 1.29.0-nightly (97085f9fb 2018-08-01)
$ rustup run --install nightly-2018-08-02 cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/foo`

And now produces an error whose message seems wrong: it points at the call site of a macro as a "name also defined here":

$ rustup run --install nightly-2018-08-03 rustc -V
rustc 1.30.0-nightly (7e8ca9f8b 2018-08-03)
$ rustup run --install nightly-2018-08-03 cargo run
   Compiling foo v0.0.0 (file:https:///tmp/foo)
error[E0659]: `warn` is ambiguous
 --> src/real_main.rs:4:5
  |
4 |     warn!("!!!");
  |     ^^^^
  |
note: `warn` could refer to the name imported here
 --> src/real_main.rs:1:1
  |
1 | #[macro_use] extern crate log;
  | ^^^^^^^^^^^^
note: `warn` could also refer to the name defined here
 --> src/real_main.rs:4:5
  |
4 |     warn!("!!!");
  |     ^^^^
  = note: macro-expanded macro imports do not shadow

error: aborting due to previous error

For more information about this error, try `rustc --explain E0659`.
error: Could not compile `foo`.

To learn more, run the command again with --verbose.

Commit range between these two nightlies: 97085f9...7e8ca9f

#52841 is the only PR in the range that seems relevant. CC @petrochenkov, @alexcrichton

@SimonSapin SimonSapin added 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 8, 2018
@SimonSapin
Copy link
Contributor Author

#52841 (comment)

This is an expected breakage.
[…]
In this case macro expanded name (macro_rules macro warn from #[macro_use] extern crate log; inside include!() expansion) shadows a name from outer scope (built-in attribute warn from the language prelude)
[…]
built-in attributes can be somehow special-cased to avoid the error, or perhaps some more general solution can be found

It took me a few reads to find the key word "attribute" in this explanation, which refers to the lint-control one used for example as #[warn(unsafe_code)].

Why do function-like macros shadow attributes? (Even proc macro ones.) Shouldn’t they be in separate namespaces?

At the very least, the error message here is wrong. But avoiding the breakage entirely would be nice.

@petrochenkov
Copy link
Contributor

Waiting on #53089

bors-servo pushed a commit to servo/servo that referenced this issue Aug 9, 2018
Work around a rustc regression in macro name resolution

rust-lang/rust#53205

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21367)
<!-- Reviewable:end -->
@petrochenkov
Copy link
Contributor

an error whose message seems wrong: it points at the call site of a macro as a "name also defined here

Yeah, the diagnostics weren't properly adjusted for #52841

Why do function-like macros shadow attributes? (Even proc macro ones.) Shouldn’t they be in separate namespaces?

Well, this is an interesting question.
Macros are being modularized only now and the plan was to start conservatively with one namespace to avoid proliferation of namespaces.
Approach like this was okay for other namespaces, e.g. so far I haven't seen complaints about impl A for B { ... } not skipping A and searching further if A is a module, type or something else that's not a trait.

If the annoying cases like this continue to arise, we can move to "sub-namespaces" during relative path resolution (attributes are ignored when resolving non-attributes and vice versa, but use macro_name; still imports a single name for the whole macro namespace), or full-blown namespaces where use macro_name can import three (!!!) macros of different kinds at the same time.


It's also important to note that this particular case shouldn't even be a restricted shadowing error!

It's an error only because the restricted shadowing rules are very conservative and coarse-grained - we report an error if "macro-expanded macro shadows any other macro", but what we really want is to error on "more macro-expanded macro shadows less macro-expanded macro".
This is especially important for servo-like use cases when all the code is inside a macro (e.g. include!(...)).
In other words, this doesn't work right now, but it eventually should:

macro_rules! my_include {() => {
    macro m() {} // 1
    
    {
        macro m() {} // 2
        
        m!(); // ERROR `m` is ambiguous (1 or 2)
    }
}}

fn main() {
    my_include!();
}

bors-servo pushed a commit to servo/servo that referenced this issue Aug 9, 2018
Work around a rustc regression in macro name resolution

rust-lang/rust#53205

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21367)
<!-- Reviewable:end -->
@nikomatsakis
Copy link
Contributor

Visiting for triage. Let's mark as P-high, but @petrochenkov seems to be on this.

@nikomatsakis nikomatsakis added the P-high High priority label Aug 9, 2018
bors-servo pushed a commit to servo/servo that referenced this issue Aug 9, 2018
Work around a rustc regression in macro name resolution

rust-lang/rust#53205

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21367)
<!-- Reviewable:end -->
bors added a commit that referenced this issue Aug 13, 2018
Fix a few regressions from enabling macro modularization

The first commit restores the old behavior for some minor unstable stuff (`rustc_*` and `derive_*` attributes) and adds a new feature gate for arbitrary tokens in non-macro attributes.

The second commit fixes #53205

The third commit fixes #53144.
Same technique is used as for other things blocking expansion progress - if something causes indeterminacy too often, then prohibit it.
In this case referring to crate-local macro-expanded `#[macro_export]` macros via module-relative paths is prohibited, see comments in code for more details.

cc #50911
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, ..) P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants