Fix parsing ambiguous call options by Xanewok · Pull Request #862 · NomicFoundation/slang

Xanewok · 2024-02-27T17:42:42Z

Based on #853

Closes #850
Closes #861

changeset-bot · 2024-02-27T17:42:48Z

🦋 Changeset detected

Latest commit: 7308fc3

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	Patch

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

crates/codegen/grammar/src/parser_definition.rs

crates/solidity/inputs/language/src/definition.rs

OmarTawfik

I think we missed a regression here. The following command with this input succeeds on main, but fails on c09e7619:

cargo run --bin slang_solidity -- parse --version "0.6.12" input.sol

Error: Expected CloseBrace or OpenBrace or ReturnsKeyword.
     ╭─[input.sol:313:17]
     │
 313 │ ╭─▶                 try IFeeCollector(feeCollector).updateRewards(wallets, rewards) {
     ┆ ┆   
 319 │ ├─▶             }
     │ │                   
     │ ╰─────────────────── Error occurred here.
─────╯

Xanewok · 2024-03-04T13:59:14Z

Ah, you're right. We need a lookahead of Ident followed by a colon to disambiguate in favour of the named arguments, whereas the example that now fails has a function call, which only starts with an ident (and so is a partial match and thus we always recover regardless).

I'm on it.

Xanewok · 2024-03-04T15:15:16Z

This now uses a more general approach of "tokens matched threshold" that the incomplete/no match must meet in order to trigger error recovery. It should be more correct in the presence of ambiguities such as this and in the future should probably be computed from the syntax analysis rather than fed by the user. I've added the regression test to the CST snapshots.

Moreover, this uses a single parameter rather than the Opts struct before and since the approach is more general, it's okay to leave it as is for now IMO rather than pursuing the aforementioned extensive config approach above.

crates/codegen/language/internal_macros/src/derive/spanned.rs

crates/codegen/language/definition/src/model/non_terminals/field.rs

crates/solidity/testing/snapshots/cst_output/TryStatement/try_catch/input.sol

crates/solidity/outputs/cargo/slang_solidity/src/generated/kinds.rs

OmarTawfik

I think the latest implementation is rejecting empty try bodies, as it consumes the { } as call options:

try foo() {
} catch {
}

Maybe we can also add that as a test case?

Btw, if it is useful, I suggest trying to run sanctuary against your branch, since you already merged with latest main. It can quickly spot any regressions if we are changing parser infra:

https://github.com/Xanewok/slang/actions/workflows/sanctuary.yml

Xanewok · 2024-03-07T10:26:22Z

Seems to be a regression introduced in #870; I cannot mark NamedArguments as allowed to be empty, since it's otherwise accepted in an invalid position where it leads to the above misparse, consuming otherwise valid input.

Is there anything else to be done? Should this be adapted to use try_select for the NamedArguments in the AST selectors logic?

OmarTawfik · 2024-03-07T16:17:10Z

@Xanewok I see. I that case, since there are now effectively two productions, one i can be empty/zero or more, and the other cannot/one or more, what do you think about two separate items? Like NamedArguments and CallOptions?

Xanewok · 2024-03-07T17:54:09Z

@OmarTawfik yeah, this makes sense! The CallOptions would have to be arg+, while the NamedArguments would have to be arg*.

Building block for #862 and #885 Hopefully extracting this into a smaller PR can speed up its landing, as other refactors also touch the tree shape/v1 PG etc.

This allows stand-alone call options, without trailing arguments, which is legal: ```solidity msg.sender.call{value: msg.value-amountEth}; ``` Fixes NomicFoundation#850

This is required to resolve syntax ambiguities, since we can't always safely attempt recovering from lookahead of 2 and more, by default.

This needs a lookahead of two tokens (ident, colon) to disambiguate between the try block and the call options.

Xanewok · 2024-03-07T23:10:31Z

Rebased.

crates/solidity/inputs/language/src/definition.rs

OmarTawfik

Looks great! Thanks.

Co-authored-by: Omar Tawfik <15987992+OmarTawfik@users.noreply.github.com>

@AntonyBlakey

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.14.0 ### Minor Changes - [#753](#753) [`b35c763`](b35c763) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Add tree query implementation as `Query::parse` and `Cursor::query` - [#755](#755) [`8c260fc`](8c260fc) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support parsing NatSpec comments - [#908](#908) [`ab3688b`](ab3688b) Thanks [@Xanewok](https://github.com/Xanewok)! - Changed the cst.NodeType in TS to use more descriptive string values rather than 0/1 integers - [#886](#886) [`0125717`](0125717) Thanks [@Xanewok](https://github.com/Xanewok)! - Add `TokenKind::is_trivia` - [#887](#887) [`dff1201`](dff1201) Thanks [@Xanewok](https://github.com/Xanewok)! - Add support for constant function modifier removed in 0.5.0 - [#885](#885) [`a9bd8da`](a9bd8da) Thanks [@Xanewok](https://github.com/Xanewok)! - Flatten the trivia syntax nodes into sibling tokens - [#908](#908) [`ab3688b`](ab3688b) Thanks [@Xanewok](https://github.com/Xanewok)! - Add RuleNode/TokenNode::toJSON() in the TypeScript API ### Patch Changes - [#801](#801) [`ecbba49`](ecbba49) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - unreserve pragma keywords in all versions - [#869](#869) [`951b58d`](951b58d) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support dots in yul identifiers from `0.5.8` till `0.7.0` - [#890](#890) [`1ff8599`](1ff8599) Thanks [@Xanewok](https://github.com/Xanewok)! - Mark `override` as being a valid attribute only after 0.6.0 - [#800](#800) [`d1827ff`](d1827ff) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support unicode characters in string literals up to `0.7.0` - [#797](#797) [`86f36d7`](86f36d7) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix source locations for unicode characters in error reports - [#854](#854) [`4b8970b`](4b8970b) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - parse line breaks without newlines - [#844](#844) [`f62de9e`](f62de9e) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix parsing empty `/**/` comments - [#799](#799) [`303dda9`](303dda9) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - prevent parsing multiple literals under `StringExpression` before `0.5.14` - [#847](#847) [`6b6f260`](6b6f260) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - prioritize parsing `MultiLineComment` over `MultiLineNatSpecComment` - [#796](#796) [`59e1e53`](59e1e53) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `public` and `internal` to `UnnamedFunctionAttribute` till `0.5.0` - [#756](#756) [`e839817`](e839817) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix parsing `payable` primary expressions - [#851](#851) [`67dfde8`](67dfde8) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix selection order of prefix/postfix AST fields - [#857](#857) [`f677d5e`](f677d5e) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename `FieldName` to `NodeLabel` - [#852](#852) [`ca79eca`](ca79eca) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - allow parsing `ColonEqual` as two separate tokens before `0.5.5` - [#889](#889) [`ce5050f`](ce5050f) Thanks [@Xanewok](https://github.com/Xanewok)! - Support `delete` as an expression rather than a statement - [#923](#923) [`bb30fc1`](bb30fc1) Thanks [@Xanewok](https://github.com/Xanewok)! - Support arbitrary ASCII escape sequences in string literals until 0.4.25 - [#887](#887) [`dff1201`](dff1201) Thanks [@Xanewok](https://github.com/Xanewok)! - Support view and pure function modifiers only from 0.4.16 - [#800](#800) [`d1827ff`](d1827ff) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename `AsciiStringLiteral` to `StringLiteral` - [#838](#838) [`ad98d1c`](ad98d1c) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - upgrade to rust `1.76.0` - [#849](#849) [`5c42e0e`](5c42e0e) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `override` and `virtual` to `ConstructorAttribute` - [#862](#862) [`5e37ea0`](5e37ea0) Thanks [@Xanewok](https://github.com/Xanewok)! - allow call options as a post-fix expression - [#786](#786) [`0bfa6b7`](0bfa6b7) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support Yul label statements before `0.5.0` - [#839](#839) [`2d698eb`](2d698eb) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support string literals in version pragmas - [#891](#891) [`70c9d7d`](70c9d7d) Thanks [@Xanewok](https://github.com/Xanewok)! - Fix parsing `<NUMBER>.member` member access expression - [#842](#842) [`2069126`](2069126) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `private` to `UnnamedFunctionAttribute` till `0.5.0` - [#840](#840) [`7fb0d20`](7fb0d20) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - allow `var` in `TupleDeconstructionStatement` before `0.5.0` Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

To support the new [Custom Storage Layout](https://docs.soliditylang.org/en/v0.8.29/contracts.html#custom-storage-layout) language feature, `ContractDefinition` nodes will no longer have an optional `InheritanceSpecifier` child directly, but will hold a list of `ContractSpecifier` children, that can either be `InheritanceSpecifier` or `StorageLayoutSpecifier`. Closes NomicFoundation#1282 Additionally, the new grammar resurfaces the same issue as NomicFoundation#862 for call options during error recovery, so I fixed the parser codegen as well.

To support the new [Custom Storage Layout](https://docs.soliditylang.org/en/v0.8.29/contracts.html#custom-storage-layout) language feature, `ContractDefinition` nodes will no longer have an optional `InheritanceSpecifier` child directly, but will hold a list of `ContractSpecifier` children, that can either be `InheritanceSpecifier` or `StorageLayoutSpecifier`. Closes #1282 Additionally, the new grammar resurfaces the same issue as #862 for call options during error recovery, so I fixed the parser codegen as well.

@AntonyBlakey

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.14.0 ### Minor Changes - [#753](NomicFoundation/slang#753) [`b35c763`](NomicFoundation/slang@b35c763) Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Add tree query implementation as `Query::parse` and `Cursor::query` - [#755](NomicFoundation/slang#755) [`8c260fc`](NomicFoundation/slang@8c260fc) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support parsing NatSpec comments - [#908](NomicFoundation/slang#908) [`ab3688b`](NomicFoundation/slang@ab3688b) Thanks [@Xanewok](https://github.com/Xanewok)! - Changed the cst.NodeType in TS to use more descriptive string values rather than 0/1 integers - [#886](NomicFoundation/slang#886) [`0125717`](NomicFoundation/slang@0125717) Thanks [@Xanewok](https://github.com/Xanewok)! - Add `TokenKind::is_trivia` - [#887](NomicFoundation/slang#887) [`dff1201`](NomicFoundation/slang@dff1201) Thanks [@Xanewok](https://github.com/Xanewok)! - Add support for constant function modifier removed in 0.5.0 - [#885](NomicFoundation/slang#885) [`a9bd8da`](NomicFoundation/slang@a9bd8da) Thanks [@Xanewok](https://github.com/Xanewok)! - Flatten the trivia syntax nodes into sibling tokens - [#908](NomicFoundation/slang#908) [`ab3688b`](NomicFoundation/slang@ab3688b) Thanks [@Xanewok](https://github.com/Xanewok)! - Add RuleNode/TokenNode::toJSON() in the TypeScript API ### Patch Changes - [#801](NomicFoundation/slang#801) [`ecbba49`](NomicFoundation/slang@ecbba49) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - unreserve pragma keywords in all versions - [#869](NomicFoundation/slang#869) [`951b58d`](NomicFoundation/slang@951b58d) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support dots in yul identifiers from `0.5.8` till `0.7.0` - [#890](NomicFoundation/slang#890) [`1ff8599`](NomicFoundation/slang@1ff8599) Thanks [@Xanewok](https://github.com/Xanewok)! - Mark `override` as being a valid attribute only after 0.6.0 - [#800](NomicFoundation/slang#800) [`d1827ff`](NomicFoundation/slang@d1827ff) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support unicode characters in string literals up to `0.7.0` - [#797](NomicFoundation/slang#797) [`86f36d7`](NomicFoundation/slang@86f36d7) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix source locations for unicode characters in error reports - [#854](NomicFoundation/slang#854) [`4b8970b`](NomicFoundation/slang@4b8970b) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - parse line breaks without newlines - [#844](NomicFoundation/slang#844) [`f62de9e`](NomicFoundation/slang@f62de9e) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix parsing empty `/**/` comments - [#799](NomicFoundation/slang#799) [`303dda9`](NomicFoundation/slang@303dda9) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - prevent parsing multiple literals under `StringExpression` before `0.5.14` - [#847](NomicFoundation/slang#847) [`6b6f260`](NomicFoundation/slang@6b6f260) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - prioritize parsing `MultiLineComment` over `MultiLineNatSpecComment` - [#796](NomicFoundation/slang#796) [`59e1e53`](NomicFoundation/slang@59e1e53) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `public` and `internal` to `UnnamedFunctionAttribute` till `0.5.0` - [#756](NomicFoundation/slang#756) [`e839817`](NomicFoundation/slang@e839817) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix parsing `payable` primary expressions - [#851](NomicFoundation/slang#851) [`67dfde8`](NomicFoundation/slang@67dfde8) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix selection order of prefix/postfix AST fields - [#857](NomicFoundation/slang#857) [`f677d5e`](NomicFoundation/slang@f677d5e) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename `FieldName` to `NodeLabel` - [#852](NomicFoundation/slang#852) [`ca79eca`](NomicFoundation/slang@ca79eca) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - allow parsing `ColonEqual` as two separate tokens before `0.5.5` - [#889](NomicFoundation/slang#889) [`ce5050f`](NomicFoundation/slang@ce5050f) Thanks [@Xanewok](https://github.com/Xanewok)! - Support `delete` as an expression rather than a statement - [#923](NomicFoundation/slang#923) [`bb30fc1`](NomicFoundation/slang@bb30fc1) Thanks [@Xanewok](https://github.com/Xanewok)! - Support arbitrary ASCII escape sequences in string literals until 0.4.25 - [#887](NomicFoundation/slang#887) [`dff1201`](NomicFoundation/slang@dff1201) Thanks [@Xanewok](https://github.com/Xanewok)! - Support view and pure function modifiers only from 0.4.16 - [#800](NomicFoundation/slang#800) [`d1827ff`](NomicFoundation/slang@d1827ff) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename `AsciiStringLiteral` to `StringLiteral` - [#838](NomicFoundation/slang#838) [`ad98d1c`](NomicFoundation/slang@ad98d1c) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - upgrade to rust `1.76.0` - [#849](NomicFoundation/slang#849) [`5c42e0e`](NomicFoundation/slang@5c42e0e) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `override` and `virtual` to `ConstructorAttribute` - [#862](NomicFoundation/slang#862) [`5e37ea0`](NomicFoundation/slang@5e37ea0) Thanks [@Xanewok](https://github.com/Xanewok)! - allow call options as a post-fix expression - [#786](NomicFoundation/slang#786) [`0bfa6b7`](NomicFoundation/slang@0bfa6b7) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support Yul label statements before `0.5.0` - [#839](NomicFoundation/slang#839) [`2d698eb`](NomicFoundation/slang@2d698eb) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support string literals in version pragmas - [#891](NomicFoundation/slang#891) [`70c9d7d`](NomicFoundation/slang@70c9d7d) Thanks [@Xanewok](https://github.com/Xanewok)! - Fix parsing `<NUMBER>.member` member access expression - [#842](NomicFoundation/slang#842) [`2069126`](NomicFoundation/slang@2069126) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `private` to `UnnamedFunctionAttribute` till `0.5.0` - [#840](NomicFoundation/slang#840) [`7fb0d20`](NomicFoundation/slang@7fb0d20) Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - allow `var` in `TupleDeconstructionStatement` before `0.5.0` Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

OmarTawfik mentioned this pull request Feb 28, 2024

allow call options as a post-fix expression #853

Closed

This was linked to issues Feb 29, 2024

FunctionCallOptions without ArgumentsDeclaration #850

Closed

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

Closed

Xanewok force-pushed the call-options-recovery-fix branch from 13aa1e1 to 4c896e7 Compare February 29, 2024 13:33

Xanewok marked this pull request as ready for review February 29, 2024 13:34

Xanewok requested a review from a team as a code owner February 29, 2024 13:35

Xanewok commented Feb 29, 2024

View reviewed changes

crates/codegen/grammar/src/parser_definition.rs Outdated Show resolved Hide resolved

Xanewok commented Feb 29, 2024

View reviewed changes

crates/solidity/inputs/language/src/definition.rs Outdated Show resolved Hide resolved

OmarTawfik approved these changes Feb 29, 2024

View reviewed changes

OmarTawfik requested changes Mar 1, 2024

View reviewed changes