Attempt error recovery from partial matches in separated lists by Xanewok · Pull Request #595 · NomicFoundation/slang

Xanewok · 2023-09-17T20:31:53Z

Based on #593.

Implements a dedicated SeparatedHelper (no state, can change to a free function) that implements recovering from partial matches in separated lists.

There are 3 ways we can recover:

a separatee matches but unexpected tokens follow before a separator
a separatee is partially matched
a separatee can't be parsed

Ideally we should recover in all three cases, however because the the trailing separator is not mandatory (nor legal), cases 1. and 3. are tricky. In case 2 The separated-by lists are not always in a delimited group (e.g. IdentifierPath) and so there's no sure way to bound the recovery, as the unexpected tokens (i.e. not a separator) can be already part of the outer parse.

For instance, 0.0.0 || 0.0.0 (case 1) or 0.0. || 0.0.0 (case 3) might attempt to recover by skipping the || 0 part. Until we constrain the error recovery with a la FOLLOW for the recovering parser, we can't reliably recover errors for now in these cases.

Parsing *some* input in when the first element is optional shouldn't be accepted as empty.

This changed because of the NomicFoundation#586 that was merged in the meantime.

To reduce ident gluing in the codegen.

In preparation for the custom recovery logic, to make sure that we don't regress anything by introducing the custom SeparatedBy helper/parser.

changeset-bot · 2023-09-17T20:31:56Z

🦋 Changeset detected

Latest commit: 3c4146e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@nomicfoundation/slang	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Xanewok · 2023-09-17T21:38:34Z

@OmarTawfik looks like a spurious sccache failure:

 error: failed to run `rustc` to learn about target-specific information

Caused by:
  process didn't exit successfully: `/home/runner/work/slang/slang/bin/sccache rustc - --crate-name ___ --print=file-names --warn unused_crate_dependencies --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg` (exit status: 2)
  --- stderr
  sccache: error: Timed out waiting for server startup. Maybe the remote service is unreachable?
  Run with SCCACHE_LOG=debug SCCACHE_NO_DAEMON=1 to get more information

OmarTawfik · 2023-09-18T21:21:27Z

@OmarTawfik looks like a spurious sccache failure:

Looks like it recovered on retry. Please let me know if you see it again and I can look into it.

…rated

Xanewok · 2023-09-20T11:56:46Z

Updated and CI is green

AntonyBlakey

I think in general that when testing for resynchronization we should prove the recovery. Currently our code samples don't show this because they don't have any following context. We can f2f this.

…rated

Xanewok · 2023-09-21T14:20:55Z

I added a test with error recovery for the identifier path lists (so nested separated-by lists) in the function definition and in the individual expressions of the following body, which shows that we recover individually everywhere:

https://github.com/NomicFoundation/slang/pull/595/files#diff-6ac3c03a1f9a12cdb00f8a531141082f21c485d841f2001d778202385d5ab4d3

function all() override(some.ident unexpected tokens, ISomeInterface, Other) public {
  msg.sender.call{do: 1, arg: 1 }();
  msg.sender.call{, empty: 1, parse: 2 }();
  msg.sender.call{arg: 1, missing_expr: , no_semicolon, , }();
  msg.sender.call{arg: 1 unexpected tokens, not: 2, recovered, yet: 3, }();

}

function empty() override(some.ident, /* empty */, other.arg.here, and.here);

function nested_lists() override(some.ident, next.do.that, other.while, next.one, final, ultimate);
function nested_lists() override(some., next.arg, next.one, ultimate);

Do you want me to also fill the bodies of these functions for completeness' sake and/or find a more complete example for the recovery implemented here?

AntonyBlakey · 2023-09-25T03:59:42Z

Do you want me to also fill the bodies of these functions for completeness' sake and/or find a more complete example for the recovery implemented here?

No, I just wanted to show that we recovered and continued.

…rated

Xanewok · 2023-09-25T12:17:35Z

Updated

@Xanewok

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @nomicfoundation/slang@0.10.0 ### Minor Changes - [#595](#595) [`1a258c4`](1a258c4) Thanks [@Xanewok](https://github.com/Xanewok)! - Attempt error recovery when parsing incomplete lists - [#564](#564) [`e326a06`](e326a06) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Parsing operators with missing operands should no longer panic - [#564](#564) [`e326a06`](e326a06) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Inline parse rules are no longer exposed to the API. - [#564](#564) [`e326a06`](e326a06) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Scanners are no longer available as methods - use next_token instead - [#564](#564) [`e326a06`](e326a06) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Scanners are now grouped into contexts to deal with contextual scanning ### Patch Changes - [#601](#601) [`cbd2a79`](cbd2a79) Thanks [@Xanewok](https://github.com/Xanewok)! - Attempt parser error recovery between bracket delimiters - [#599](#599) [`4bbad48`](4bbad48) Thanks [@Xanewok](https://github.com/Xanewok)! - Use correct versions for the `revert` and `global` keywords - [#561](#561) [`cb6a138`](cb6a138) Thanks [@Xanewok](https://github.com/Xanewok)! - Add preliminary documentation for the `solidity_language` Rust package Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

@Xanewok

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and publish to npm yourself or [setup this action to publish automatically](https://github.com/changesets/action#with-publishing). If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @nomicfoundation/slang@0.10.0 ### Minor Changes - [#595](NomicFoundation/slang#595) [`1a258c4`](NomicFoundation/slang@1a258c4) Thanks [@Xanewok](https://github.com/Xanewok)! - Attempt error recovery when parsing incomplete lists - [#564](NomicFoundation/slang#564) [`e326a06`](NomicFoundation/slang@e326a06) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Parsing operators with missing operands should no longer panic - [#564](NomicFoundation/slang#564) [`e326a06`](NomicFoundation/slang@e326a06) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Inline parse rules are no longer exposed to the API. - [#564](NomicFoundation/slang#564) [`e326a06`](NomicFoundation/slang@e326a06) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Scanners are no longer available as methods - use next_token instead - [#564](NomicFoundation/slang#564) [`e326a06`](NomicFoundation/slang@e326a06) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Scanners are now grouped into contexts to deal with contextual scanning ### Patch Changes - [#601](NomicFoundation/slang#601) [`cbd2a79`](NomicFoundation/slang@cbd2a79) Thanks [@Xanewok](https://github.com/Xanewok)! - Attempt parser error recovery between bracket delimiters - [#599](NomicFoundation/slang#599) [`4bbad48`](NomicFoundation/slang@4bbad48) Thanks [@Xanewok](https://github.com/Xanewok)! - Use correct versions for the `revert` and `global` keywords - [#561](NomicFoundation/slang#561) [`cb6a138`](NomicFoundation/slang@cb6a138) Thanks [@Xanewok](https://github.com/Xanewok)! - Add preliminary documentation for the `solidity_language` Rust package Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Xanewok and others added 10 commits September 8, 2023 21:51

fix: Don't ignore IncompleteMatch in OneOrMoreHelper

26f8fb9

Parsing *some* input in when the first element is optional shouldn't be accepted as empty.

Merge branch 'main' into repetition-dont-match-optional-incomplete

317fb1b

Bless tests

f23047b

This changed because of the NomicFoundation#586 that was merged in the meantime.

refactor: Split SeparatedBy into a dedicated helper type

170258e

refactor: Use Lexer directly in SeparatedHelper

54e633c

To reduce ident gluing in the codegen.

refactor: Implement Lexer::delimiters directly in the trait

63acd08

refactor: Separate the skip with nested delims logic

68b4662

refactor: Naive direct implementation of SeparatedBy

918d17d

In preparation for the custom recovery logic, to make sure that we don't regress anything by introducing the custom SeparatedBy helper/parser.

tests: Add regression tests for the SeparatedBy error recovery

63a34bb

feat: Recover from incomplete separated matches

1b7665f

Xanewok marked this pull request as ready for review September 17, 2023 20:31

Xanewok requested a review from a team as a code owner September 17, 2023 20:31

Xanewok added 2 commits September 17, 2023 22:53

refactor: Remove duplicate peek_token_after_trivia function

3c496f1

Add a changeset file

ddfd506

Xanewok force-pushed the error-recovery-separated branch from c67216b to ddfd506 Compare September 17, 2023 20:53

Merge remote-tracking branch 'upstream/main' into error-recovery-sepa…

c122f29

…rated

AntonyBlakey suggested changes Sep 21, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into error-recovery-sepa…

6570a2f

…rated

AntonyBlakey approved these changes Sep 25, 2023

View reviewed changes

Xanewok added 2 commits September 25, 2023 14:13

Merge remote-tracking branch 'upstream/main' into error-recovery-sepa…

a29cde2

…rated

Bless tests

3c4146e

Xanewok mentioned this pull request Sep 25, 2023

Improve parsing error recovery "skipahead" #600

Open

AntonyBlakey added this pull request to the merge queue Sep 26, 2023

Merged via the queue into NomicFoundation:main with commit 1a258c4 Sep 26, 2023

github-actions bot mentioned this pull request Sep 22, 2023

Bump Slang Version #590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt error recovery from partial matches in separated lists#595

Attempt error recovery from partial matches in separated lists#595
AntonyBlakey merged 16 commits intoNomicFoundation:mainfrom
Xanewok:error-recovery-separated

Xanewok commented Sep 17, 2023

Uh oh!

changeset-bot bot commented Sep 17, 2023 •

edited

Loading

Uh oh!

Xanewok commented Sep 17, 2023

Uh oh!

OmarTawfik commented Sep 18, 2023

Uh oh!

Xanewok commented Sep 20, 2023

Uh oh!

AntonyBlakey left a comment •

edited

Loading

Uh oh!

Xanewok commented Sep 21, 2023

Uh oh!

AntonyBlakey commented Sep 25, 2023

Uh oh!

Xanewok commented Sep 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Xanewok commented Sep 17, 2023

Uh oh!

changeset-bot bot commented Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Xanewok commented Sep 17, 2023

Uh oh!

OmarTawfik commented Sep 18, 2023

Uh oh!

Xanewok commented Sep 20, 2023

Uh oh!

AntonyBlakey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok commented Sep 21, 2023

Uh oh!

AntonyBlakey commented Sep 25, 2023

Uh oh!

Xanewok commented Sep 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Sep 17, 2023 •

edited

Loading

AntonyBlakey left a comment •

edited

Loading