[WIP] Macro rules backtracking

[LeoTestard]

This fixes macro_rules! to implement the wanted backtracking semantics: if an arm fails to match, the next arm is tried, and so on.

This was previously not the case because of hard errors emitted during parsing of non-terminals. Now that the parser code has been fixed not to panic but to return a DiagnosticBuilder in case of error, it's easy to catch those errors and signal that the arm failed to match.

This PR probably contains a bit of hackery, so it's flagged as WIP so that people can review it.

[rust-highfive]

r? @sfackler

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

[LeoTestard]

r? @nrc

[nagisa]

Potential O(n²)?

[LeoTestard]

No. The parsing algorithm is the same. There is actually no « backtracking » as you may understand it. The parsing algorithm works on a single arm and, on failure, will try the next arms with the same tokens. This PR only make sure all errors encountered during the parsing for an arm are not fatal but properly reported. There's no change in complexity here. In fact, the parsing algorithm could already be called on every arm if no non-terminal parsing errors occured.

[nrc]

nit: prefer ? to try!

[nrc]

why add this method? Can we not just use the field?

[LeoTestard]

Because it's private I guess. We still can make it pub...
I chosed to add a method for consistency with message below.

[nrc]

I think a pub field would be nicer (more idiomatic Rust), could change message too I guess

[LeoTestard]

Well I think I can remove the ‶WIP″ marker and submit this PR for review. cc @pnkfelix @nrc

[nrc]

lgtm with a cursory read over the code, but r? @pnkfelix for a detailed review

[LeoTestard]

Note to myself: we must not fail here, but rather just remove the current parsing item from the stack.

[LeoTestard]

Note for myself and for others: Allright we have a problem here. As I noted in the comment above, we currently only backtrack from an arm, not from a parsing item. That means that if you have a macro like this:

macro_rules! foo(
    ( $( $e:expr ),* some_other_tokens ) => ...
)

If we provide it input that doesn't match expr, you'd expect it to skip the sequence repetition, since it can appear 0 times, and instead match it against the following tokens. This is not the case, since, as I outlined in the line note, it will just fail and continue with the next arm.

This sounds easy to fix: just remove the current parsing item from the stack.
But. If we do this, we have a future-proofing issue. We thought future-proofing analysis was solved for single-arm macros. It was only because the backtracking semantics were not implemented. Now consider the following macro:

macro_rules! foo(
    ( $( $e:expr , )* $( $i:ident : $t:ty , )* ) => ...
)

Before type ascription syntax, foo!(a: i32,) should skip the first sequence repetition, since a: i32 is not an expr, and bind a to $i.
After type ascription syntax, it will bind a: i32 to $e and skip the second sequence repetition.
The thing is that, based on the current FOLLOW sets analysis for single arm macros, we can't reject this because all requirements are met: : is in FOLLW(ident), ident and expr both are in FOLLOW(,), etc.

This was not a problem before because failure to match a NT (such as expr) inside the sequence repetition would abort the parsing of the macro. But those are not the desired parser semantics.

This problem is in fact the very same as the one with multiple-arm macros:

macro_rules! foo(
    $( $e:expr ) => ... ;
    $( $i:ident : $t:ty ) => ...
)

So I believe that my analysis for multiple-arm macros could be applied to fix the issue with the current future-proofing analysis for single-arm macros. But until we sorted that out, we have two solutions for this PR:

• Don't fix it to backtrack inside a single arm, keep it that way, and merge it. We'll fix this issue later.
• Don't merge it, and wait for the future-proofing issue I described to be fixed, and then make a true fix for backtracking that addresses this issue.

(By fixing this issue, I mean: making sure the parser backtracks out of a sequence repetition.)

cc @nikomatsakis @pnkfelix

[nikomatsakis]

@LeoTestard as we said in person, this is indeed a drag, though I guess it just ups the priority of investigating the other future-proofing problem. :(

[bors]

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

[alexcrichton]

ping, @LeoTestard looks like this needs a rebase?

[LeoTestard]

@alexcrichton Yes indeed, but it's blocked anyway until we have a fix for the future-proofing issue. I don't know what we should do with this PR. Maybe we can just close it and reopen it later?

[alexcrichton]

Ah either way is fine by me, this is now active in the last 5 hours rather than 21 days, so I'm happy at least :)

[brson]

This is quite stalled. @pnkfelix @LeoTestard is there any chance of this PR landing? Should we close?

[LeoTestard]

@brson Yes, there is still a chance. I thought again about this problem after I found the problem I mentioned above, and I no longer think it's a problem. It turns out that I did not understood how the parser handled ambiguity at the time. I should be able to sum up the situation in more detail.

[steveklabnik]

@LeoTestard

I should be able to sum up the situation in more detail.

Any chance of this happening? I am tempted to just close this PR until you or someone else is ready to move forward here; this is going to be extremely out of date, right?

[nikomatsakis]

Seems reasonable to close the PR in the meantime, though @LeoTestard I'd like to hear more about what you have in mind.

[LeoTestard]

@steveklabnik The code is probably quite out of date indeed, I agree that the PR can be closed in the meantime.

@nikomatsakis Sure. I promise I'll take the time to write about it at some point, but I'm quite busy these days.