rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.88k stars 12.67k forks source link

No message when importing two crates defining a macro with the same name #32911

Closed Nemo157 closed 4 years ago

Nemo157 commented 8 years ago

If you have two crates that define an identically named macro and #[macro_use] both of them then there will be no warning/error about the conflict, instead it's simply the second to be imported that is used.

Example using modules (playground):

#[macro_use]
mod foo {
    macro_rules! hello {
        () => ("hello");
    }
}

#[macro_use]
mod bar {
    macro_rules! hello {
        () => ("goodbye");
    }
}

fn main() {
    println!(hello!());
}

This is unlikely to produce issues in practice, but could potentially be really, really annoying to try and track down if it does. For example, say an update to one of your modules introduces a new utility macro that has #[doc(hidden)] on it1 which just so happens to exactly match up type-wise with an existing macro you were using, albeit with slightly different runtime behaviour (like the hello! macro does above); your tests don't cover the cases that cause the different behaviour so they all pass, then months later you get a ticket about it and spend hours trying to work out why the original macro seems to be doing something completely different to what its code says it does.


1Bad practice yes, but oh so likely to happen at some point.

LeoTestard commented 8 years ago

I work on this one. :)

LeoTestard commented 8 years ago

In fact I don't know if we should warn on this. For macros imported from external crates, it doesn't make much sense indeed not to, since the shadowed macro will never be visible. But for local macros imported from modules flagged with #[macro_use] it's not that simple. macro_rules! definitions are macro invocations themselves, which means that macros are registered (and thus exported) during the expansion process. So we can have code like this:

#[macro_use] mod foo {
    macro_rules! baz {
        () => ("foo")
    }
}

fn foo() {
    println!("{}", baz!());
}

#[macro_use] mod bar {
    macro_rules! baz {
        () => ("bar")
    }
}

fn main() {
    foo();
    println!("{}", baz!());
}

which will print:

foo
bar

as expected. This is legit. You can see it just like let. You can shadow a binding, but it might have been used already and it can still be used because of lexical capture. Our current approach to macro expansion keeps a stack of frame which corresponds to module, each with its list of visible macros. This works a bit like lexical scoping. The thing is that we do not warn when shadowing a let-binding. What we do however is warning about unused variable. We currently do not warn about unused macro definitions but that would maybe be a more pertinent approach. I'd be surprised if we did not already have an issue somewhere for that.

Maybe a reasonable approach would be to implement such a shadowing check for macros imported from extern crates and, eventually, to check for unused locally-defined macros?

cc @nrc @kmcallister

Nemo157 commented 8 years ago

The big difference I see between shadowing of macros and let bindings is scope, the original let binding and any shadows of it will (likely) fit on a single page of code so it's easy to see that it has happened and which binding is the current one. Macros, especially when imported via #[macro_use], can be declared in many different files; and there's no way at the use site to work out which of the multiple declarations is the current one without looking through all parent modules and their imports manually.

Maybe it is by design, but I would hope that no-one is actually using this feature on purpose.

LeoTestard commented 8 years ago

Mhhh right, you've got a point. But I think a check for unused macro definitions would still be sufficient to catch all errorneours cases.

nrc commented 8 years ago

Given that macros are order-dependent (and thus the current situation is at least a little bit sensible) and that this would be a breaking change, I'm not desperate to see it fixed.

steveklabnik commented 4 years ago

Triage: over three years later, seems like nobody has run into this, and doesn't seem like there's even consensus that this is a bug that should be fixed. I'm going to close this. Thank you!