Rust: Multiple bindings of the same metavariable in a macro rule aren't detected until matching.

[alercah]

The following macro definition compiles:

macro_rules! bad {
  ($a:expr, $a:expr) => {}
}

Any attempt to match against this macro will result in an error, because the metavariable cannot be bound twice. There is no reason for the compiler to accept this definition as a result, since it always represents a bug, except for backwards compatibility. It should be fixed, either by assuring it won't break the ecosystem or by deprecating the behaviour and denying it the next edition.

[alercah]

Example:

macro_rules! ex {
    ($a:expr, $a:expr) => { };
}

fn main() {
    //ex!(1, 2);
}

This compiles fine. But uncommenting the invocation gives an error "duplicated bind name: 'a'". See https://play.rust-lang.org/?version=beta&mode=debug&edition=2018&gist=5990e9c7a1553622afbe0a7fa929d195.

[Centril]

cc @petrochenkov @rust-lang/lang

I'm not sure if this is by design or not, but it seems like a bug to me so I've marked it as such.

[petrochenkov]

Probably an accidental omission.
(Disclaimer: I never touched the code doing MBE matching and transcription.)

[Centril]

@petrochenkov do you know who to cc by any chance?

[petrochenkov]

do who to cc by any chance?

Spirits of ancestors at this point.

@mark-i-m could be familiar with that code though after implementing ?.

[cramertj]

Worth noting that if we fix this it needs a crater run, since this would cause previously-compile-able-but-unusable macro_rules variants to stop compiling, e.g.:

macro_rules! my_macro {
    (one_arg $e:expr) => { ... },
    (two_arg $e:expr, $e:expr) => { ... },
}

my_macro!(one_arg, 5);

This would all work fine, so long as no one was testing the two_arg case.

[nikomatsakis]

My impression is that this is an oversight. We could fix it, or we could just add a lint. The latter seems like a good starting point and is what I personally would do.

[mark-i-m]

I would prefer to fix it over an edition boundary and warn for now.

[mark-i-m]

I opened #57617, for the technical parts, but we need to sort out policy before I can finish it.

[Centril]

Let's do a crater run before setting out any policy. I think this is clearly a bug tho and thus an edition boundary seems excessive.

[alercah]

Waiting for an edition boundary has no value if the goal is to eventually make it deny-by-default across all editions.

[mark-i-m]

No I meant that we error only in the new edition. I agree that we should do a crater run first, but I expect this to be cause a bunch of breakage.

[mark-i-m]

I will try to fix up my PR later do we can do a crater run

[cramertj]

What's the purpose of introducing it as an error rather than a deny-by-default lint?

[Centril]

Let's not speculate about what to do without facts... :)

[mark-i-m]

Crater run is done https://github.com/rust-lang/rust/pull/57617#issuecomment-455341547

[mark-i-m]

1 regression from crates.io and two more from rust's test suite.

[Centril]

OK we definitely don't need to use an edition boundary for 1 regression... 1-2 release cycle should be enough imo.

[mark-i-m]

Cross-posting from the PR: I have implemented a future incompatibility lint for this with the goal of later turning it into a hard-error. What's next?