Fix the revert function parsing mechanism #1502
Conversation
🦋 Changeset detectedLatest commit: 9c327c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
ggiraldez
left a comment
ggiraldez
left a comment
There was a problem hiding this comment.
Left a couple of minor comments, but otherwise looks good to me.
| let definition_id = { | ||
| let reference = self | ||
| .binder | ||
| .find_reference_by_identifier_node_id(node.error.last().unwrap().id()); | ||
| reference.and_then(|reference| reference.resolution.as_definition_id()) | ||
| }; |
There was a problem hiding this comment.
Nit: prefer the inline expression:
let definition_id = self
.binder
.find_reference_by_identifier_node_id(node.error.last().unwrap().id())
.and_then(|reference| reference.resolution.as_definition_id());| function testBuiltIn() public pure { | ||
| revert("error"); | ||
| } | ||
|
|
||
| function testShadowed() public pure { | ||
| function(string memory) pure revert = customRevert; | ||
| revert("error"); | ||
| } | ||
|
|
||
| function customRevert(string memory) internal pure {} |
There was a problem hiding this comment.
To avoid the failing case for 0.4.11 you should remove the pure attributes, which will make the test clearer on its intent.
There was a problem hiding this comment.
Thanks, that makes it a lot better
|
|
||
| function revertFunction() public { | ||
| revert("error"); | ||
| } | ||
|
|
||
|
|
||
| function revertFunctionInCondition() public { | ||
| if (revert("error")) {} | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it may be better to move these two functions to a separate test case to avoid confusion with the name revert_stmt, maybe as revert_as_function. Or even to the revert_shadowing case above, renaming it to revert_as_function and changing the testShadowed() function to a more explicit testRevertCanBeShadowed().
There was a problem hiding this comment.
I moved them around as suggested, it's more clear. Thanks
There was a problem hiding this comment.
Nit: removing these two new empty lines would remove the unnecessary diffs in the output files for this case.
ggiraldez
left a comment
ggiraldez
left a comment
There was a problem hiding this comment.
Left another two minor suggestions, but looks great, thank you!
.changeset/spotty-ducks-behave.md
Outdated
| "@nomicfoundation/slang": minor | ||
| --- | ||
|
|
||
| Fixed the old style revert calls (`revert("oops!")`) to be parsed as a normal function call rather than a reverse statement. |
There was a problem hiding this comment.
| Fixed the old style revert calls (`revert("oops!")`) to be parsed as a normal function call rather than a reverse statement. | |
| Fixed the old style revert calls (`revert("oops!")`) to be parsed as a normal function call rather than a `revert` statement. |
|
|
||
| function revertFunction() public { | ||
| revert("error"); | ||
| } | ||
|
|
||
|
|
||
| function revertFunctionInCondition() public { | ||
| if (revert("error")) {} | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: removing these two new empty lines would remove the unnecessary diffs in the output files for this case.
Currently the old style revert function (
revert("description")) is being parsed as a revert statement, the issue with this is that it doesn't allow the binder to solve the right reference to it (the built-inreserve) or even for the user to shadow it with their ownrevertfunction.Update: I also noticed that before this change some cases were parsed differently depending on context, so: