macro parses let keyword as pattern and fails + masked regression

[Ezrashaw]

I tried this code:

macro_rules! let_let_oof {
    ($p:pat) => {
        let_let_oof! { let let $p }
    };
    (let let $p:pat) => {
        fn a() {
           let $p = 5; 
        } 
    }
}

let_let_oof! { x }

I expected the code to compile and the macro to expand as follows:

fn a() {
  let x = 5;
}

Instead, compilation failed:

error: expected pattern, found `let`
  --> bad-code2.rs:3:24
   |
3  |         let_let_oof! { let let $p }
   |                        ^^^ help: remove the unnecessary `let` keyword
...
12 | let_let_oof! { x }
   | ------------------ in this macro invocation
   |
   = note: this error originates in the macro `let_let_oof` (in Nightly builds, run with -Z macro-backtrace for more info)

error: expected identifier, found keyword `let`
  --> bad-code2.rs:3:28
   |
3  |         let_let_oof! { let let $p }
   |                            ^^^ expected identifier, found keyword
...
12 | let_let_oof! { x }
   | ------------------ in this macro invocation
   |
   = note: this error originates in the macro `let_let_oof` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

Meta

rustc --version --verbose:

rustc 1.70.0-nightly (8a73f50d8 2023-03-11)
binary: rustc
commit-hash: 8a73f50d875840b8077b8ec080fa41881d7ce40d
commit-date: 2023-03-11
host: x86_64-unknown-linux-gnu
release: 1.70.0-nightly
LLVM version: 15.0.7

Occurs on latest stable and nightly.

Additional context

I believe there are two issues here:

1. (error 2) The parser consumes let as an identifier and then fails because it is not a valid identifier; Parser::can_be_ident_pat (see compiler/rustc_parse/parser/pat.rs:811) returns true on keywords and then the caller (compiler/rustc_parse/parser/pat.rs:398) attempts to parse an ident pattern and promptly fails because it sees a keyword.
2. (error 1) On top of this (and thus no code has been broken), a regression occurred with the introduction of the following code (see compiler/rustc_parse/parser/pat.rs:345) which eagerly consumes tokens without being behind a Parser::may_recover gate (this is similar to Breaking change in macro_rules ty fragment parsing in version 1.68 #107796, that regression I caused, right? cc @compiler-errors):

if self.token.is_keyword(kw::Let) && self.look_ahead(1, |tok| tok.can_begin_pattern()) {
    self.bump();
    self.sess.emit_err(RemoveLet { span: lo });
    lo = self.token.span;
}

If this is correct, I'd like to PR it please :).

[compiler-errors]

Yeah, feel free to gate the recovery behavior behind Parser::may_recover.

[Ezrashaw]

That still leaves the other error. Is that error intended behavior? I think not, although it's been present since (at least) 1.0.0.

[fmease]

That still leaves the other error.

You could consider adding && !self.token.is_keyword(kw::Let) below the similar kw::In case in can_be_ident_pat (for the same reason as mentioned in the comment over there). This might incur a minor performance cost in the happy path though and it would change the error message slightly: We would no longer suggest r#let but we don't do so either with let in = … compared to all other reserved identifiers. That's a bit unfortunate.

Btw, I don't think your let_let_oof! code can compile anyway with the current (macro) parsing infrastructure. Replace let let in both places with a different incorrect pattern prefix, e.g. for for, for or struct and it doesn't compile either. I suspect the parser is not really tuned to support these kinds of macro match arms where the second arm is expected to only fire when the first one failed to parse a malformed snippet according to a given Rust subgrammar (like pat or item).

The expression parser is quite good at that though, the pattern and item parsers not so much it seems. I think the let_let_oof! invocation should compile at some point (see the snippets in #103534 that do exactly this kind of matching) but it might require larger changes to the parser. #103534 is only tangentially related. It actually exploits this “fairness” of the expression parser to demonstrate the importance of .may_recover(). The issue talks about the whole parser but only provides examples containing expressions since I failed to find examples involving e.g. patterns or items because the parsers of the latter two are quite unrelenting as already mentioned.

[Ezrashaw]

You could consider adding && !self.token.is_keyword(kw::Let) below the similar case.

Yeah we could do that but it would become unsustainable. Ultimately, when not in a macro context, not suggesting let r#int = ... or equivalent is very unfortunate. What probably needs to happen for this case is that can_be_ident_pat returns false for keywords when in a macro context. Those suggestions are perfectly valid outside macros and arbitrarily removing them is probably not a good idea.

I'm honestly not particularly inclined to fix this because it has existed since 1.0.0. I agree that to fix all these little issues with macros will require significant changes and I don't think it's worth it right now. However, I will lump the let let masked regression fix into a PR that deals with some of the code around there.

@rustbot P-low