Better errors with bad/missing identifiers in MBEs

[EliseZeroTwo]

This PR implements better error handling around identifiers in MBEs.

It is related to #118295 which is already closed, but it fixes up issues left present by the solution for that issue.

Improved errors:

Parenthesised Ident

Previously with the code:

macro_rules! (meow) {
    () => {}
}

The compiler would output the following, incorrect error:

error: macros that expand to items must be delimited with braces or followed by a semicolon
 --> src/lib.rs:1:14
  |
1 | macro_rules! (meow) {
  |              ^^^^^^
  |
help: change the delimiters to curly braces
  |
1 | macro_rules! {meow} {
  |              ~    ~
help: add a semicolon
  |
1 | macro_rules! (meow); {
  |                    +

error: expected item, found `{`
 --> src/lib.rs:1:21
  |
1 | macro_rules! (meow) {
  |                     ^ expected item
  |
  = note: for a full list of items that can appear in modules, see <https://doc.rust-lang.org/reference/items.html>

Now it outputs:

error: expected identifier, found `(`
 --> src/lib.rs:1:14
  |
1 | macro_rules! (meow) {
  |              ^ expected identifier
  |
note: try removing the parenthesis around the name for this `macro_rules!`
 --> src/lib.rs:1:15
  |
1 | macro_rules! (meow) {
  |               ^^^^

Missing Ident

Previously with the code:

macro_rules! {}

The compiler would output the following, incorrect error:

error: cannot find macro `macro_rules` in this scope
 --> src/lib.rs:1:1
  |
1 | macro_rules! {}
  | ^^^^^^^^^^^

Now it outputs:

error: expected identifier, found `{`
 --> src/lib.rs:1:14
  |
1 | macro_rules! {}
  |              ^ expected identifier
  |
  = note: maybe you have forgotten to define a name for this `macro_rules!`

This will interfere with the stderr of the test case I introduced in #118928 that has not yet been merged, due to that test case requiring a (templated) parenthesised ident to be present.

It includes regression tests.

🖤

[rustbot]

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

[EliseZeroTwo]

In #62258 macro_rules was reallowed as an identifier for MBE definitions. This removes the possibility of proper error checking of MBEs at the parser level. Given that in the merge request @eddyb stated it could be potentially reverted in the future, I was wondering what the next step with this should be. As per the specification it appears to me as if you shouldn't be allowed to define MBEs with the macro_rules identifier.

[TaKO8Ki]

@EliseZeroTwo A test tests/ui/macros/user-defined-macro-rules.rs has failed on CI. Could you fix it?

[TaKO8Ki]

@rustbot author

[EliseZeroTwo]

@TaKO8Ki I have re-implemented the special casing of macro_rules not being allowed as an identifier for MBE definitions, and fixed the previously failing test that needed to be adjusted as a result of it (the failing test was also added in #62258 due to the change in it). All tests pass now.

[EliseZeroTwo]

@rustbot label -S-waiting-on-author +S-waiting-on-review

[EliseZeroTwo]

@rustbot author

[EliseZeroTwo]

@rustbot review

[cjgillot]

r=me with one nit

[petrochenkov]

This is a breaking language change, if I understand correctly.

[EliseZeroTwo]

This is a breaking language change, if I understand correctly.

As documentation seems to indicate that this is how it is meant to be, I did not think it was. I took a look before submitting this and also could not find published software using this, but maybe testing the change against crates.io would be a good idea anyways?

It does undo the change you implemented in #62258 which reallowed macro_rules as an identifier for macros.

[petrochenkov]

this is how it is meant to be

Well, not by me, at least.
macro_rules is reserved as minimally as possible, because the eventual goal was always retiring it in favor of macro - it isn't strictly necessary to reserve macro_rules in this context, so it's not reserved (that's similar to other weak keywords like union).

In any case, such changes should go through the language team.
(And I'm personally against it going, because the only benefit here is a minor diagnostic improvement.)

[bors]

☔ The latest upstream changes (presumably #119146) made this pull request unmergeable. Please resolve the merge conflicts.

[apiraino]

I'll try to push this conversation forward by nominating for T-lang discussion, keeping in mind some raised concerns.

@rustbot label I-lang-nominated

[traviscross]

I'm going through PRs we've been sitting on awhile. This one ended up toward the bottom of our queue on lang for essentially the reasons @petrochenkov outlined. It's a breaking change, and those always take more work, and this one has limited upside. On top of that, it reintroduces the oddity that the reservation is partial, as noted by @petrochenkov back in 2019, as this still works:

macro_rules! foo { () => (0) }

use foo as macro_rules;

fn main() {
    println!("{}", macro_rules!());
}

According to the Reference (here), macro_rules is actually specified as a legal identifier for a macro name, since we end up at this rule,

NON_KEYWORD_IDENTIFIER -> IDENTIFIER_OR_KEYWORD _except a strict or reserved keyword_

and macro_rules is a weak keyword and not a strict or reserved one.

For this proposal to have a chance, in my view, it'd need to come with documentation updates, a crater run, and a good explanation about why #62258 was done with enthusiasm and why we should feel comfortable in undoing it. Probably too, if it were to do this reservation, it should do it fully.

Given the objection from @petrochenkov and the limited upside here, I just estimate that it's rather unlikely we'd do this on the basis of what's in this PR now, so while we all always want to make our diagnostics better, and I appreciate the PR author for putting in the work here, rather than leave this sit longer, I'm just going to close it out.

cc @rust-lang/lang