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

or choosing secondary option, over recovered first option #677

Open
KrosFire opened this issue Oct 7, 2024 · 1 comment
Open

or choosing secondary option, over recovered first option #677

KrosFire opened this issue Oct 7, 2024 · 1 comment
Labels
1.0 Features that should be implemented for the 1.0 release error-recovery An issue related to chumsky's error recovery API

Comments

@KrosFire
Copy link

KrosFire commented Oct 7, 2024

I'm building an lsp, so error recovery is very important for me. I wanted to recover a function call that would look something like that foo(

My parser would look like this:

function_call_parser()
  .or(var_parser())
  .or(parentheses_parser())

function_call_parser correctly recovers from this error and maps to proper value, but The final result would be a combination of a var foo with parentheses () showing an error form parentheses parser.

I've looked at the Or struct implementation and I think this is the problem:

if a_res.0.is_empty() {
    if let (a_errors, Ok(a_out)) = a_res {
        return (a_errors, Ok(a_out));
    }
}

stream.revert(pre_state);

#[allow(deprecated)]
let b_res = debugger.invoke(&self.1, stream);
let b_state = stream.save();

if b_res.0.is_empty() {
    if let (b_errors, Ok(b_out)) = b_res {
        return (b_errors, Ok(b_out));
    }
}

If I understand this correctly, if the second parser doesn't produce an error, while the first does, we choose the result of the second parser even tho the first parser may have parsed correctly.

While this may be a valid behaviour it's not consistent with what the docs say:
"If both parser succeed, the output of the first parser is guaranteed to be prioritised over the output of the second."

Is there some other util I can use, or should I create my own? I can create the fix as it seems pretty easy. What do you think?

@zesterer
Copy link
Owner

zesterer commented Oct 8, 2024

When I wrote the docs I meant 'succeed' to mean 'succeed without errors', but I realise that this is quite ambiguous. I also agree that the existing behaviour produces inconsistent results with error recovery enabled vs disabled.

The ideal situation would be to have chumsky guarantee that the path that minimises errors will be taken, but this turns out to be difficult to implement in the presence of repeated.

Although it is quite a substantial change in behaviour, I agree that - for now, at least - consistency is preferable and so I'm happy to accept a PR. If possible, could you change the relevant docs to say "If both parsers produce an output..."?

@zesterer zesterer added 1.0 Features that should be implemented for the 1.0 release error-recovery An issue related to chumsky's error recovery API labels Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Features that should be implemented for the 1.0 release error-recovery An issue related to chumsky's error recovery API
Projects
None yet
Development

No branches or pull requests

4 participants
@zesterer @KrosFire and others