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

Create return nodes for named implicit returns #1880

Closed
wants to merge 45 commits into from

Conversation

webthethird
Copy link
Contributor

@webthethird webthethird commented May 2, 2023

Resolves issues #1450 and #1719, and includes the previously failing test from #1818, which now passes.

Creates an artificial return node iff a function has a named return variable declared in its signature. Finds all leaf nodes in the CFG which are not return nodes, and links them to the artificial return node. The return node is given an Identifier expression with a referencedDeclaration pointing to the variable declaration in the signature.

Once this return node is in place and correctly linked, subsequent analyses resolve the data dependency issue for named variables without any further changes, i.e., to Slither's IR generation. The goal being to implement the simplest solution to that problem.

Depends on #1886

@webthethird
Copy link
Contributor Author

@0xalpharush @montyly what is the best way to go about updating all of the expected results for the CI tests that are affected by this change?
I think it's mostly printer tests that are failing, because now some functions have an additional node in their CFGs and some additional IR to go with the new node. There's also a test_ast_parsing which is failing on a bunch of examples, as is to be expected since we're adding the new node in some cases.
It might be best to have one of you check the failed tests to make sure they are in fact expected.

@0xalpharush
Copy link
Contributor

The printers are failing because of an assertion failure:

  File "/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/site-packages/slither/solc_parsing/declarations/function.py", line 321, in analyze_content
    self._fix_implicit_return(body)
  File "/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/site-packages/slither/solc_parsing/declarations/function.py", line 1301, in _fix_implicit_return
    return_node.add_unparsed_expression(
  File "/opt/hostedtoolcache/Python/3.8.16/x64/lib/python3.8/site-packages/slither/solc_parsing/cfg/node.py", line 30, in add_unparsed_expression
    assert self._unparsed_expression is None
AssertionError

Printer failed
yul-0.4.11.sol-0.5.17-compact.zip

I would check locally that the updates are correct, but you can run python tests/e2e/solc_parsing/test_ast_parsing.py --overwrite to update the tests.

@webthethird
Copy link
Contributor Author

Since we decided on closing #1886 in favor of leaving the empty functions flagged as implemented and just adding an implicit RETURN node after the ENTRY_POINT, we will need to update the expected result files for the CFG printer test to account for the new nodes.

Also, @montyly you mentioned:

However if we do so, we should add the information that the node was not explicit in Solidity, as this information can be useful for further analyses

Do you have any thoughts on what the best way to capture this information? I imagine we wouldn't want to create a new NodeType, since there must be lots of places where if(node.type == NodeType.RETURN) is used, and we want implicit returns to still register as returns.

@webthethird
Copy link
Contributor Author

@0xalpharush could you take a look at this test failure and let me know if you can tell why it's failing? I think practically all of the assertion failures in the 2 failing checks below are due to the new return nodes, and can be resolved by updating the expected CFGs. But I can't think of any reason why a reentrancy detector would have failed.

@webthethird
Copy link
Contributor Author

It may have something to do with the fact that DAO.splitDAO has a named return variable but never uses it.

@webthethird
Copy link
Contributor Author

It looks like each THROW node is getting a lint to the implicit return, which it probably shouldn't.

@webthethird

This comment was marked as resolved.

@webthethird webthethird marked this pull request as ready for review May 16, 2023 15:57
@webthethird
Copy link
Contributor Author

All checks are passing, including the implicit-returns parser tests I added. This one is ready for review!

@webthethird webthethird marked this pull request as draft May 18, 2023 18:16
@webthethird webthethird marked this pull request as ready for review May 18, 2023 21:27
webthethird added a commit to webthethird/slither that referenced this pull request May 24, 2023
@0xalpharush
Copy link
Contributor

replaced by #2326

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants