Fix parsing ambiguous call options

[Xanewok]

Based on #853

Closes #850
Closes #861

[changeset-bot]

🦋 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

[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]

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]

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.

[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]

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]

@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]

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

[Xanewok]

Rebased.

[OmarTawfik]

Looks great! Thanks.