Regression in error message quality for macro_rules involving $:ident

[dtolnay]

Between nightly-2020-02-28 and nightly-2020-02-29 I observed that macro errors which used to point to a specific problematic identifier token now point equivocally to the entire macro invocation. See dtolnay/tt-call@364468e.

The issue minimizes to:

macro_rules! nothing_expected {
    () => {};
}

macro_rules! repro {
    ($ident:ident) => {
        nothing_expected!($ident);
    };
}

repro!(T);

Before; points to T:

error: no rules expected the token `T`
  --> src/main.rs:11:8
   |
1  | macro_rules! nothing_expected {
   | ----------------------------- when calling this macro
...
11 | repro!(T);
   |        ^ no rules expected this token in macro call

After; does not point to T:

error: no rules expected the token `T`
  --> src/main.rs:7:27
   |
1  | macro_rules! nothing_expected {
   | ----------------------------- when calling this macro
...
7  |         nothing_expected!($ident);
   |                           ^^^^^^ no rules expected this token in macro call
...
11 | repro!(T);
   | ---------- in this macro invocation
   |
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

The applicable commit range is 6d69cab...0eb878d.
The most relevant looking PR in that range is #69384. @petrochenkov @Centril

[petrochenkov]

It still points to the specific problematic identifier

7  |         nothing_expected!($ident);
   |                           ^^^^^^ no rules expected this token in macro call

, and now it's "in context", closer to the source of the syntax error.

Another case to illustrate the change

// Code
macro_rules! m { ($i: ident) => {
    struct S $i
}}

m!(i);

fn main() {}

// Error before
error: expected `where`, `{`, `(`, or `;` after struct name, found `i`
 --> src/main.rs:5:4
  |
5 | m!(i);
  |    ^ expected `where`, `{`, `(`, or `;` after struct name

// Error after
error: expected `where`, `{`, `(`, or `;` after struct name, found `i`
 --> src/main.rs:2:14
  |
2 |     struct S $i
  |              ^^ expected `where`, `{`, `(`, or `;` after struct name

, the "after" variant looks better to me.

[dtolnay]

@petrochenkov that's true if the macro definition is in the same crate. If it's from a different crate like in dtolnay/tt-call@364468e, all you see is an error on the entire invocation:

  |
5 | m!(i);
  | ^^^^^^

This is less helpful particularly if there are many equal i identifiers in a larger input.

I think I am on board with the change for local macros but would prefer to have the old behavior for external macros. For external macros I think it will be better to steer toward localizing the macro expansion error as much as possible within a potentially large input, even if some external macro rule (which does not get shown in the diagnostic) may be "more at fault".

[pnkfelix]

would be good to know when this was injected. tagging E-needs-bisect.

[pnkfelix]

triage: needs-bisect. Prioritizing as P-medium, based on assumption that the quality of diagnostics here can regress slightly and then get fixed in a future release if need be.

[petrochenkov]

I'm pretty sure #69384 is the reason, like it was mentioned above.

I'm interested in the effect of #66364 on this diagnostic, but in the end we should probably show both locations since none of them is unambiguously better.

[LeSeulArtichaut]

cargo-bisect-rustc reports a regression in 0eb878d

@rustbot modify labels: -E-needs-bisection