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

Ensure error recovery does not misparse in the event of ambiguous syntax #861

Closed
OmarTawfik opened this issue Feb 26, 2024 · 4 comments · Fixed by #862
Closed

Ensure error recovery does not misparse in the event of ambiguous syntax #861

OmarTawfik opened this issue Feb 26, 2024 · 4 comments · Fixed by #862
Assignees

Comments

@OmarTawfik
Copy link
Contributor

OmarTawfik commented Feb 26, 2024

even though a NamedArgument is required inside {}, it is matched successfully as a post-fix expression, and thus, failing for expecting the try body afterwards.

Originally posted by @OmarTawfik in #853 (comment)

@Xanewok
Copy link
Contributor

Xanewok commented Feb 27, 2024

@OmarTawfik this refers to the Separated parser of NamedArguments at (latest) https://github.com/NomicFoundation/slang/pull/853/files#diff-af454bb70b3f914baf6e383790278f8efd4f555a445ec8b915164e93d73cc1d1R3354, right?

@Xanewok
Copy link
Contributor

Xanewok commented Feb 27, 2024

Here's more context about the underlying issue:

It includes a delimited parser ({ NamedArguments }) that recovers on the empty content at the nearest delimiter matching the current tree, so the parse returns "Match, but recovered" (includes SKIPPED nodes).

Now, since the expression parser will happily tack that recovered group onto the current accumulator, we end up with a.b() ++ {} (recovered).

Because this syntax is generally ambiguous and is explicitly disambiguated by solc with a token lookahead to check for at least a single item (Identifier followed by a colon), we can't really attempt recovering here from an empty match as this leads to the issue where we can recover past the point where another parser (Block here) should pick up, leading to a misparse.

We already have a notion of RecoverFromNoMatch for recovery that's currently always emitted for terminators; we should surface this to a DSL and allow the users to disable error recovery in the face of ambiguous syntax such as this.

@OmarTawfik OmarTawfik changed the title Repeated parser helper fails to propagate error when single child fails Separated parser helper fails to propagate error when single child fails Feb 28, 2024
@OmarTawfik
Copy link
Contributor Author

@Xanewok fixed the title. Thanks!

@OmarTawfik OmarTawfik linked a pull request Feb 29, 2024 that will close this issue
@Xanewok Xanewok changed the title Separated parser helper fails to propagate error when single child fails Ensure error recovery does not misparse in the event of ambiguous syntax Mar 4, 2024
@Xanewok
Copy link
Contributor

Xanewok commented Mar 4, 2024

Changed the title to better reflect the underlying issue and what we're trying to fix in the short/mid term here.

@Xanewok Xanewok moved this from ⏳ Todo to 🔄 In Progress in Slang - 2024 H1 Mar 7, 2024
github-merge-queue bot pushed a commit that referenced this issue Mar 8, 2024
Based on #853 

Closes #850
Closes #861

---------

Co-authored-by: OmarTawfik <[email protected]>
@github-project-automation github-project-automation bot moved this from 🔄 In Progress to ✅ Done in Slang - 2024 H1 Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants