[Bug]: When variable is reused in two multi-return assignments, original tuple index is reused

[kevinclancy]

Describe the issue:

When a variable is used to store a result of a multi-return function call and then later used to store the result of a different multi-return function call, the index of the first UNPACK operation is used for the second UNPACK. Instead, the index of the second UNPACK should be based on where the variable was listed on the lhs of the assignment.

Code example to reproduce the issue:

contract Test {
    function threeRet(int z) internal returns(int, int, int) {
        return (1,2,3);
    }
    function twoRet(int z) internal returns(int, int) {
        return (3,4);
    }

    function test() external returns(int) {
        (int a, int b, int c) = threeRet(3);
        (a, c) = twoRet(b);
        return b;
    }
}

Version:

0.9.3

Relevant log output:

INFO:Printers:Contract Test
        Function Test.threeRet(int256) (*)
                Expression: (1,2,3)
                IRs:
                        RETURN 1,2,3
        Function Test.twoRet(int256) (*)
                Expression: (3,4)
                IRs:
                        RETURN 3,4
        Function Test.test() (*)
                Expression: (a,b,c) = threeRet(3)
                IRs:
                        TUPLE_0(int256,int256,int256) = INTERNAL_CALL, Test.threeRet(int256)(3)
                        a(int256)= UNPACK TUPLE_0 index: 0 
                        b(int256)= UNPACK TUPLE_0 index: 1 
                        c(int256)= UNPACK TUPLE_0 index: 2 
                Expression: (a,c) = twoRet(b)
                IRs:
                        TUPLE_1(int256,int256) = INTERNAL_CALL, Test.twoRet(int256)(b)
                        a(int256)= UNPACK TUPLE_1 index: 0 
                        c(int256)= UNPACK TUPLE_1 index: 2 
                Expression: b
                IRs:
                        RETURN b

Note that we unpack index 2 into `c` for the second call, but the result of `twoRet` doesn't even have a component with index 2.

[kevinclancy]

I imagine this is caused by this code in expression_to_slithir.py:

                        # The following test is probably always true?
                        if (
                            isinstance(left[idx], LocalVariableInitFromTuple)
                            and left[idx].tuple_index is not None
                        ):
                            index = left[idx].tuple_index

I wonder why this code exists. Can we use idx as index?

[duckki]

I wonder what the if-statement was supposed to do. Removing the if-statement doesn't break any tests (as in make test based on the commit 79ff12a5e290a0695826f276bf848b1165e5d57a).

[duckki]

Besides, recording tuple_index on variable itself is flawed.

Example:

    function test() public {
        (uint a, ) = i.f();
        (, a) = i.f();
    }

Wrong IR:

	Function Debug.test() (*)
		Expression: (a) = i.f()
		IRs:
			TUPLE_0(uint256,uint256) = HIGH_LEVEL_CALL, dest:i(I), function:f, arguments:[]
			a(uint256)= UNPACK TUPLE_0 index: 0
		Expression: (None,a) = i.f()
		IRs:
			TUPLE_1(uint256,uint256) = HIGH_LEVEL_CALL, dest:i(I), function:f, arguments:[]
			a(uint256)= UNPACK TUPLE_1 index: 0

Note that the both UNPACK uses index 0, while the second UNPACK should've use index 1.
The tuple_index is really up to the assignment expression, but the variable.

UPDATE:
I noticed that (uint a, ,) = ... has a special AST where LHS is just a single variable, while (a, ,) = ... has a list of (optional) lvalues. So, it makes sense to record the expected tuple index on the LocalVariableInitFromTuple instance.
After all, it seems the right thing to remove the if-statement in the normal lvalue list handling.

[kevinclancy]

I think tuple_index is needed for the initial assignment as well. When expression_to_slither encounters tuple-based variable declarations, all of nulls representing unassigned returns are missing. I think this is caused by this line, which filters out padded variables but shouldn't.