Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Macro rules backtracking #33840

Closed

Conversation

LeoTestard
Copy link
Contributor

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
Copy link
Collaborator

r? @sfackler

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

@LeoTestard
Copy link
Contributor Author

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned sfackler May 24, 2016
@nagisa
Copy link
Member

nagisa commented May 25, 2016

Potential O(n²)?

@LeoTestard
Copy link
Contributor Author

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.

This allows to cancel an error with only an immutable access to the `Handler`. Mutable access is already not required to emit an error anyway.
@LeoTestard LeoTestard force-pushed the macro-rules-backtracking branch 2 times, most recently from b9eb282 to 0d8dae4 Compare May 31, 2016 10:22
match name {
"tt" => {
p.quote_depth += 1; //but in theory, non-quoted tts might be useful
let res: ::parse::PResult<'a, _> = p.parse_token_tree();
let res = token::NtTT(P(panictry!(res)));
let res = token::NtTT(P(try!(res)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: prefer ? to try!

@LeoTestard
Copy link
Contributor Author

LeoTestard commented Jun 7, 2016

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

@nrc
Copy link
Member

nrc commented Jun 8, 2016

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

@rust-highfive rust-highfive assigned pnkfelix and unassigned nrc Jun 8, 2016
let nt = match parse_nt(&mut rust_parser, span,
&ident.name.as_str()) {
Ok(nt) => Rc::new(MatchedNonterminal(nt)),
Err(diag) => return Failure(diag)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@LeoTestard
Copy link
Contributor Author

LeoTestard commented Jun 9, 2016

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
Copy link
Contributor

@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
Copy link
Contributor

bors commented Jun 28, 2016

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

@alexcrichton
Copy link
Member

ping, @LeoTestard looks like this needs a rebase?

@LeoTestard
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor

brson commented Nov 9, 2016

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

@LeoTestard
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor

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

@pnkfelix pnkfelix closed this Jan 4, 2017
@pnkfelix pnkfelix added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 4, 2017
@LeoTestard
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.