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

fix(frontend): Remove chumsky fork, attempt 2 #1213

Closed
wants to merge 8 commits into from

Conversation

pczarn
Copy link
Contributor

@pczarn pczarn commented Apr 25, 2023

Related issue

Resolves #853

Description

Summary of changes

Replace recovier_via of the chumsky fork with skip_parser from chumsky 0.9.2.

Dependency additions / changes

- chumsky = { git = "https://github.com/jfecher/chumsky", rev = "ad9d312" }
+ chumsky = "0.9.2"

Test additions / changes

None.

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

Comment on lines 129 to 135
if self.reason.as_deref() != Some("recovery failure")
&& other.reason.as_deref() != Some("recovery failure")
{
// We compare spans, except for errors for recovery, because the span for recovery does not match
assert_eq!(self.span, other.span);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a workaround: since spans of the recovery failure alternative do not match the spans of other alternatives, only check that spans match if neither error comes from a recovery failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure whether we should drop recovery failures during this merging. They are meant to never be shown to the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

They should likely be dropped just in case, although I'm not aware of any time in which we ever merge a recovery error with a normal user-facing error.

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Looks good so far. A few suggestions:

@pczarn
Copy link
Contributor Author

pczarn commented Apr 30, 2023

@jfecher

Here is the description of the problem I encountered when looking into the CI failure: With recovery, empty lambda arguments (||) fail to parse. Chumsky prefers to parse "something that is a failure to parse nothing as something recovered to be something" over "nothing". This case happens with recovery within a repetition (.repeated()) or optionality (.or_not()). An issue is filed for Chumsky: zesterer/chumsky#159

The workaround is: get two variants of a parser of a sequence: first without recovery, second with a recovery. Put them as alternatives: failing without recovery, the variant with recovery will be attempted.

@pczarn
Copy link
Contributor Author

pczarn commented Apr 30, 2023

@jfecher Recovery doesn't seem to work after adding the workaround:

Expected 1 error(s) and got 2:

Expected an expression but found |
secondary: 
Expected an end of input but found :
secondary: 

From input:   let g = |a:| a
Expected AST: let g: unspecified = |error: error| -> unspecified { plain::a }
Actual AST:   let g: unspecified = plain::a

@jfecher
Copy link
Contributor

jfecher commented May 3, 2023

@pczarn does this apply to most/all recovery or is it only not working for lambda parameters?

How would you say parser error recovery in general looks like before & after this change - I haven't yet pulled the PR to test myself. I suppose I'm alright with removing lambda parameter recovery as long as other recovery is working since it shows there may be a path to fixing it, but if recovery all around is worse then it is more difficult.

@pczarn
Copy link
Contributor Author

pczarn commented May 7, 2023

@jfecher

does this apply to most/all recovery or is it only not working for lambda parameters?

For function parameter recovery: Unaffected since parameter types are mandatory, so there is always at least a colon present and no need to worry about an empty parameter list. We do not currently recover for garbage with a missing colon.

For block statement recovery: unclear why an empty block is unaffected.

No other places where a recovery is covered by optionality. Only one more place where a recovery is used.

@jfecher
Copy link
Contributor

jfecher commented Jul 17, 2023

Closing this since it has become stale. I can revisit it after confirming there have been bugfixes to chumsky to fix the changed parser recovery strategies.

@jfecher jfecher closed this Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove chumsky fork
3 participants