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: ensure that tuple variable's types belong to the Type class #41

Merged
merged 4 commits into from
May 16, 2023

Conversation

kevinclancy
Copy link

@kevinclancy kevinclancy commented May 15, 2023

Notes

The "types" of the tuple variable generated from a struct-destructuring assignment are actually strings. For example, the tuple variable generated from the following program has a list of strings instead of a list of types:

pragma solidity ^0.8.19;

contract OtherContract {
    struct S {
        uint x;
        uint y;
    }

    mapping(uint => S) public m;
}

contract Test {
    OtherContract c;
    function test() external {
        (uint x, uint y) = c.m(0);
    }
}

Struct destructuring is implemented in a function called _convert_to_structure_to_list in convert.py. When it encounters structure types, it destructures them and flattens the result into the outer structure's list. When it encounters elementary types, it should include the type in the list, but instead it includes the type's type field, which is a string. This PR fixes that by returning the ElementaryType instead of its type field.

Testing

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

Related Issue

https://github.com/CertiKProject/slither-task/issues/508

@kevinclancy kevinclancy requested a review from duckki May 15, 2023 22:47
@kevinclancy kevinclancy changed the title fix: fix: ensure that tuple variable's types belong to the Type class May 15, 2023
@kevinclancy kevinclancy force-pushed the struct-destruct-ty branch from a1ec02e to ab4f351 Compare May 15, 2023 22:57
@duckki
Copy link

duckki commented May 16, 2023

I think the root cause is the Function.return_type property is not returning a List[Type] (typing error). In fact, any LocalVariable has a string literal type value would be wrong, in the first place.
How about intercepting earlier like in Variable.type or Function.return_type ?

@kevinclancy
Copy link
Author

Good call. I have fixed the issue. That makes me wonder what root cause issue the current implementation of set_type is hiding:

    def set_type(self, t: Optional[Union[List, Type, str]]) -> None:
        if isinstance(t, str):
            self._type = ElementaryType(t)
            return
        assert isinstance(t, (Type, list)) or t is None
        self._type = t

@kevinclancy
Copy link
Author

I just realized that this fix doesn't handle structs with nested fields of type Contract and Enum correctly.

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.

Looks good!
We should contribute this to upstream.

@duckki duckki merged commit 7e06d46 into certik May 16, 2023
@chenhuitao chenhuitao deleted the struct-destruct-ty 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