Incorrect diagnostic span on syntax error in macro invoc inside trait, impl, extern blocks

[abonander]

A syntax error produced by a macro invocation in a trait {} or impl {} block will have an incorrect span for its second label:

Playground

macro_rules! m {
    () => {
        let
    }
}

trait Foo {
    m!();
}

Produces this error:

error: expected one of `async`, `const`, `extern`, `fn`, `type`, or `unsafe`, found `let`
 --> src/lib.rs:3:9
  |
1 | macro_rules! m {
  | - expected one of `async`, `const`, `extern`, `fn`, `type`, or `unsafe` here
2 |     () => {
3 |         let
  |         ^^^ unexpected token
...
8 |     m!();
  |     ----- in this macro invocation

The "expected one of ... here" line is pointing to line 1 but it should be elaborating on the "unexpected token" label, or just replacing it. The behavior is identical for impl {} blocks.

This appears to be due to some faulty logic in Parser::expect_one_of() which isn't robust in the face of macro invocations. If it can't get lines for either span from the sourcemap it should assume they're the same (in fact I'm not sure why it emits a separate "unexpected token" label).

Discovered in #54833 but not introduced by it; when that's merged extern {} blocks will show this behavior too (as seen in issue-54441.stderr on that PR).

I'll try to look into this sometime this weekend but I'm also willing to mentor on it, though like I mentioned I'm not sure why the error handling logic in Parser::expect_one_of() is so complex to begin with.

cc @petrochenkov

[estebank]

This is because when looking for the prior span in the current context in order to check wether the prior span is in a different line the code doesn't account for the case where we're in a macro context, making self.prev_span == DUMMY_SP, which is followed by the first char in the source code. Because of this, we can change the following code:

            match (cm.lookup_line(self.span.lo()), cm.lookup_line(sp.lo())) {
                (Ok(ref a), Ok(ref b)) if a.line == b.line => {
                    // When the spans are in the same line, it means that the only content between
                    // them is whitespace, point at the found token in that case:
                    //
                    // X |     () => { syntax error };
                    //   |                    ^^^^^ expected one of 8 possible tokens here
                    //
                    // instead of having:
                    //
                    // X |     () => { syntax error };
                    //   |                   -^^^^^ unexpected token
                    //   |                   |
                    //   |                   expected one of 8 possible tokens here
                    err.span_label(self.span, label_exp);
                }
                _ => {
                    err.span_label(sp, label_exp);
                    err.span_label(self.span, "unexpected token");
                }
            }

to

            match (cm.lookup_line(self.span.lo()), cm.lookup_line(sp.lo())) {
                (Ok(ref a), Ok(ref b)) if a.line == b.line => {
                    // When the spans are in the same line, it means that the only content between
                    // them is whitespace, point at the found token in that case:
                    //
                    // X |     () => { syntax error };
                    //   |                    ^^^^^ expected one of 8 possible tokens here
                    //
                    // instead of having:
                    //
                    // X |     () => { syntax error };
                    //   |                   -^^^^^ unexpected token
                    //   |                   |
                    //   |                   expected one of 8 possible tokens here
                    err.span_label(self.span, label_exp);
                }
                _ if self.prev_span == syntax_pos::DUMMY_SP => {
                    // Account for macro context where the previous span might not be
                    // available to avoid incorrect output (#54841).
                    err.span_label(self.span, "unexpected token");
                }
                _ => {
                    err.span_label(sp, label_exp);
                    err.span_label(self.span, "unexpected token");
                }
            }

[abonander]

@estebank that will just print "unexpected token", no?

[estebank]

@abonander correct. If the parser kept track of macro scopes in an ideal way, the output could be

error: expected one of `async`, `const`, `extern`, `fn`, `type`, `unsafe`, or `}`, found `let`
 --> src/lib.rs:3:5
  |
3 |         let
  |         ^^^ unexpected token
...
7 | trait Foo {
  |            - expected one of 7 possible tokens here
8 |     m!();
  |     ----- in this macro invocation

but having the output be

error: expected one of `async`, `const`, `extern`, `fn`, `type`, `unsafe`, or `}`, found `let`
 --> src/lib.rs:3:5
  |
3 |         let
  |         ^^^ unexpected token
...
8 |     m!();
  |     ----- in this macro invocation

should be enough to sidestep the issue at hand.

---

There are two big problems in general with diagnostics: incorrect and confusing. The former (like this one) needs to be fixed immediately. The later, we can take our time to design 1) a better explanation and 2) how to surface more information that we don't currently have. In this case I would really like to remove the blatantly incorrect output.

[abonander]

@estebank something I'm not sure on is why we print "unexpected token" at all and not just the "expected ... here" bit, or actually vice-versa. Is it for syntax errors spanning multiple tokens/lines?

[holmgr]

Hi this sounds like a chance for me to do my first contribution, do you mind if I take it?

[abonander]

@holmgr please do! If you need help just let me know.

[holmgr]

Sorry for the time taken but I have managed to implement the change suggested by @estebank here and confirm that the output matches:

error: expected one of `async`, `const`, `extern`, `fn`, `type`, or `unsafe`, found `let`
 --> test.rs:3:9
  |
3 |         let
  |         ^^^ unexpected token
...
8 |     m!();
  |     ----- in this macro invocation

error: aborting due to previous error

I did modify the only test affected.
In that case I guess I should just open a PR referencing this issue?

[estebank]

Yes go ahead and file a pr with your change. Thanks for fixing this!

[holmgr]

The PR is now merged, so I guess we could go ahead and close this issue now