Parse error recovery is obversable by macros in several cases

@rustbot

Introduction

In multiple cases, macro fragment specifiers like expr and stmt match more token streams than they should as a consequence of the parser trying to recover from obviously invalid Rust code to provide better immediate and subsequent error messages to the user.

Why Is This a Concern?

The user should be allowed to assume that a fragment specifier only matches valid Rust code, anything else would make the fragment specifier not live up to its name and as a result render it useless (to exaggerate).

One use of macros is the ability to define embedded / internal domain-specific languages (DSLs). Part of that is defining new syntax which might not necessarily be valid Rust syntax. Declarative macros allow users to create several macro rules / matchers enabling relatively fine-grained matching on tokens. Obviously, when writing those rules, macro authors need to know what a given fragment specifier accepts in order to confidently determine which specific rule applies for a given input. If the grammar used by a fragment specifier is actually larger than promised and basically unknown (implementation-defined to be precise), this becomes an impossible task.

Not only that. If we don't do anything, the grammars matched by fragment specifiers will keep changing over time as more and more recovery code gets added. This breaks Rust's backward compatibility guarantees! Macro calls that used to compile at some fixed point in time might potentially no longer compile in a future version of the compiler. In fact, backward compatibility has already been broken multiple times in the past without notice by (some) PRs introducing more error recovery.

Examples

There might be many more cases than listed below but it takes a lot of time experimenting and looking through the parser. I'll try to extend the list over time.

Expressions

macro_rules! check {
    ($e:expr) => {};

    // or `===`, `!==`, `<>`, `<=>`, `or` instead of `and`
    ($a:literal and $b:literal) => {};
    ($a:literal AND $b:literal) => {};

    (not $a:literal) => {};
    (NOT $a:literal) => {};
}

check! { 0 AND 1 } // passes (all good)
check! { 0 and 1 } // ⚠️ FAILS but should pass! "`and` is not a logical operator"

check! { NOT 1 } // passes (all good)
check! { not 1 } // ⚠️ FAILS but should pass! "unexpected `1` after identifier"

stderr

error: `and` is not a logical operator
  --> src/lib.rs:13:12
   |
13 | check! { 0 and 1 } // ⚠️ FAILS but should pass! "invalid comparison operator"
   |            ^^^ help: use `&&` to perform logical conjunction
   |
   = note: unlike in e.g., Python and PHP, `&&` and `||` are used for logical operators

error: unexpected `1` after identifier
  --> src/lib.rs:16:14
   |
16 | check! { not 1 } // ⚠️ FAILS but should pass! "unexpected `1` after identifier"
   |          ----^
   |          |
   |          help: use `!` to perform bitwise not

Statements

macro_rules! check {
    ($s:stmt) => {};

    // or `auto` instead of `var`
    (var $i:ident) => {};
    (VAR $i:ident) => {};
}

check! { VAR x } // passes (all good)
check! { var x } // ⚠️ FAILS but should pass! "invalid variable declaration" (+ ICE, #103529)

stderr

error: invalid variable declaration
  --> src/lib.rs:10:10
   |
10 | check! { var x } // ⚠️ FAILS but should pass! "invalid variable declaration" (+ ICE, #103529)
   |          ^^^
   |
help: write `let` instead of `var` to introduce a new variable
   |
10 | check! { let x } // ⚠️ FAILS but should pass! "invalid variable declaration" (+ ICE, #103529)
   |          ~~~

Other Fragments (e.g. Items, Types)

[no known cases at the time of this writing (active search ongoing)]

Editorial Notes

editorial notes

I used to list some more cases above (which some of you might have noticed) but I've since removed them as they've turned out to be incorrect. Here they are:

macro_rules! check {
    ($e:expr) => {};

    ($a:literal++) => {};
    ($a:literal@@) => {};

    ($n:ident : $e:expr) => {};
}

check! { 1@@ } // passes (all good)
check! { 1++ } // FAILS but would fail ANYWAY! "Rust has no postfix increment operator"
// ^ without the recovery code, we would try to parse a binary expression (operator `+`) and fail at the 2nd `+`

check! { struct : loop {} } // passes (all good)
check! { main : loop {} } // FAILS but would fail ANYWAY! "malformed loop label"
// ^ without the recovery code, we would try to parse type ascription and fail at `loop {}` which isn't a type

Related issue: #90256 (concerning procedural macros).

@rustbot label A-macros A-diagnostics A-parser T-compiler

We probably should be a lot more conservative with recoveries in macros. This reminds me of #103224.

I will put up a PR to add some more info the parser to make these checks easier in the future.

@Nilstrieb and I discussed this a bit.

Nils will add a flag to the parser state to identify when we want recovery to be enabled, and only do recovery when this is set to true. Then all parser recoveries that exist currently should eventually be gated behind this flag, so that errors are properly bubbled up within macros, so that we call fall ahead to subsequent matcher branches.

For example, given this:

macro_rules! blah {
    ($expr:expr) => {};
    (not $expr:expr) => {};
}

fn main() {
    blah!(not 1);
}

We should not eagerly recover not as ! when parsing not 1, at least not until no other match arms correctly match the macro input.

In the case that no arms match successfully, we can retry all of the match arms with recovery enabled, which allows something like this:

macro_rules! blah {
    ($expr:expr) => {};
    (let) => {};
}

fn main() {
    blah!(not 1);
}

... to continue to give us a useful error message:

error: unexpected `1` after identifier
 --> <source>:7:15
  |
7 |     blah!(not 1);
  |           ----^
  |           |
  |           help: use `!` to perform bitwise not

error: aborting due to previous error

This brings up a fun question. Should this code compile?

macro_rules! what {
    ($e:expr) => { compile_error!("no") };
    ($($tt:tt)*) => {};
}

fn wh() {
    what! {
        match 1;
    }
}

My knowledge about macros (and the two sentences the reference says about this) indicates that this should work. The first arm fails, but the second matches. But this doesn't compile, as the parser .emit()s a diagnostic on this broken code (via some cool lookahead and recovery).

So we not only have to gate all recovery directly, but also all emissions of diagnostics behind the flag. There are currently 165 references to .emit() in rustc_parse, so that's gonna be some work. But I believe that this is worth it, as the current behavior just makes no sense. Given enough awareness around compiler contributors, I think we can make sure that no new recovery is added without gating it behind the flag.

I believe it should be compile, and that my changes described above and discussed in your comment ("So we not only have to gate all recovery directly, but also all emissions of diagnostics behind the flag.") would be sufficient.

Perhaps it's worthwhile to nominate this for lang team discussion, since that code you linked @Nilstrieb hasn't compiled since before 1.0 😅

Question for the lang team:

Should this code compile?

macro_rules! what {
    ($e:expr) => { compile_error!("no") };
    ($($tt:tt)*) => {};
}

fn wh() {
    what! {
        match 1;
    }
}

Is it generally okay to "pare back" existing recovery behavior to make sure that something like this compiles?

macro_rules! blah {
    ($expr:expr) => {};
    (not $expr:expr) => {};
}

fn main() {
    blah!(not 1);
}

--

This may even be more of a "implementation" question than a lang question, to perhaps this nomination is moot.

I think the core problem is that macro_rules are insufficiently future-proofed, leading to other problems too, like #86730.

My ideal solution (if we can find a way to do it without breaking the world) would be to make the second arm in

macro_rules! check {
    ($e:expr) => {};
    (not $a:literal) => {};
}

be useless, as anything it could potentially match would go into the first arm instead, and then produce an error if it's not actually a valid expr under the relevant edition rules. (TBH I don't even know if $e:expr here uses the macro definition's edition or the caller's edition when parsing.) This would probably

Otherwise for anything we try to add to the expr grammar there's an example like this that's a breaking change.

After all, if people really want special handing for something, it would be better for them to put that first, like

macro_rules! check {
    (not $a:literal) => {};
    ($e:expr) => {};
}

so that if one day we did add not false as supported syntax, it would keep working.

(Maybe that implies that for each macro matcher we'd have some "start set", like can_begin_expr? And then it's only adding something new to the matcher's start set that would be a breaking change, for which we'd need an edition change or a new matcher.)

We discussed this in today's @rust-lang/lang meeting.

We'd like both of these examples to work. Making the one with compile_error work seems like it'll take more work, and may need a more detailed proposal. Making the one with not work seems like a bugfix that lang doesn't need to review and approve.

Rollup merge of rust-lang#104335 - Nilstrieb:macrowo, r=compiler-errors

I wonder if we should take a different approach for fixing this issue instead of using .may_recover(). Bear in mind that I am not 100% familiar with the parser & macro matching internals, so the following idea might not be workable.

What about changing PResult / adding a flag to the parser to keep track of whether a recovery occured. If it did then the token stream would not actually match a given macro matcher (hmm, but that would probably mean we need to backtrack somehow, unclear). This might introduce a performance penalty.

This way, we can still show a custom error message without making the syntactically malformed input well-formed. Right now for example, m! { fn<>() } (where m expects a ty) yields the error expected ( found < instead of function pointer types may not have generic parameters (#104223) as we use .may_recover() for this case.

Just throwing my thoughts out there.

In some cases this may work, but not always. Looking at the not 0 example, the parser needs to be edited. The parser looked ahead and eagerly recovers, emits an error and returns success. So even if the matcher then knew that recovery occurred (and the parser would somehow need to keep track of that, so we'd still need to annotate recovery) the session has the emitted error anyways.

But I agree that may_recover isn't great, but I haven't found anything better yet.

Rollup merge of rust-lang#104335 - Nilstrieb:macrowo, r=compiler-errors

Auto merge of rust-lang#115785 - fmease:fix-pat-regression, r=wesleyw…

Rollup merge of rust-lang#153624 - fmease:ping-me-on-parser-changes, …

Unrolled build for #153624

Navigation Menu

Repository navigation

Parse error recovery is obversable by macros in several cases #103534

Introduction

Why Is This a Concern?

Examples

Expressions

Statements

Other Fragments (e.g. Items, Types)

Editorial Notes

5 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Parse error recovery is obversable by macros in several cases #103534

Description

Introduction

Why Is This a Concern?

Examples

Expressions

Statements

Other Fragments (e.g. Items, Types)

Editorial Notes

Activity

compiler-errors commented on Oct 25, 2022

Noratrieb commented on Oct 25, 2022

compiler-errors commented on Oct 25, 2022

Noratrieb commented on Oct 25, 2022

compiler-errors commented on Oct 25, 2022

compiler-errors commented on Oct 25, 2022

scottmcm commented on Oct 26, 2022

5 remaining items

joshtriplett commented on Nov 1, 2022

fmease commented on Nov 19, 2022

Noratrieb commented on Nov 20, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions