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

fix: Preserve padding in tuple-based declarations #48

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

kevinclancy
Copy link

Notes

Previously, in a declaration of the form (,uint a, uint b) = ... the leading "padded" position was removed from the AST during a pre-processing step that converted declarations into assignments. The caused the variables a and b to get assigned to components 0 and 1 instead of 1 and 2.

This PR preserves the padding during the AST transformation.

Testing

  • Run make test and verify that tests succeed
  • Run ./evaluate.sh run 100 in tool-eval and verify that projects succeed.

Try viewing the IR for this test file:

pragma solidity ^0.8.13;

contract Other {
    struct S {
        uint z;
        //uint[] a;
        uint b;        uint c;
        uint d;
    }

    mapping(uint => S) public M;
}

contract Test {
    Other other;

    function test() external {
        (uint z, , uint c, uint d) = other.M(3);
        (z , c, , d) = other.M(3);
    }
}

Related Issue

https://github.com/CertiKProject/slither-task/issues/539
See also: crytic#1913

@kevinclancy kevinclancy requested a review from duckki May 31, 2023 17:14
Copy link

@duckki duckki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change looked good.
The test clearly showed the problem and the change fixed it.

@duckki duckki merged commit 4c0264b into certik Jun 1, 2023
@chenhuitao chenhuitao deleted the multi-decl-padding branch March 18, 2024 10:41
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