fix: reorder named arguments to match declaration order

[kevinclancy]

Fixes #1906

A function call with explicitly named parameters, of the form f({argName1: arg1, ...}), may list the parameters in an order different from that of the function's declaration. In this case, the IR should reorder the arguments to match the function declaration order before generating IR.

In practice, I have only seen named argument calls for struct constructors, so I suspect they are the only form that is documented. For calls to built-in functions, contract constructors, and dynamic internal function calls, named arguments produce confusing type checking errors. For all other forms of function calls, named arguments work, but I've never seen them used in a real project.

This PR implements named argument reordering for the following forms of function calls:

• struct constructors (most important)
• static internal calls

For the following forms of function calls, the order of the declaration arguments is not directly available during IR generation, so the PR did not implement reordering for them:

• external calls (HighLevelCall)
• library calls
• event calls

The get_declared_param_names function returns None for these unimplemented call forms.

Example

contract ReorderStruct {
    struct S {
        int[] a;
        uint x;
    }

    S[] public s;
    function test() external {
        S memory s = S({x: 2, a: new int[](2)});
    }
}

Output Before:

INFO:Printers:Contract ReorderStruct
        Function ReorderStruct.test() (*)
                Expression: s = S({x:2,a:new int256[](2)})
                IRs:
                        TMP_1 = new int256[](2)
                        TMP_2 = new S(2,TMP_1)
                        s(ReorderStruct.S) := TMP_2(ReorderStruct.S)

Output After

INFO:Printers:Contract ReorderStruct
        Function ReorderStruct.test() (*)
                Expression: s = S({x:2,a:new int256[](2)})
                IRs:
                        TMP_1 = new int256[](2)
                        TMP_2 = new S(TMP_1,2)
                        s(ReorderStruct.S) := TMP_2(ReorderStruct.S)

[0xalpharush]

@kevinclancy Unless I'm mistaken, the output before/after look the same in the PR description but maybe it's a mistake?
EDIT: Nvm, I was wrong

[montyly]

This is a great addition, thanks @kevinclancy

Could we add a few unit tests?

[kevinclancy]

Sure, @montyly . I just added a couple of tests.

[0xalpharush]

I think these can be removed since the function signature is typed and indicates this

[0xalpharush]

What is this assertion for? Should EventCall return an order?

[kevinclancy]

Suppose that someone were to add a new subclass of Call in the future. In this case, we would want to add another clause to this function to handle the new subclass. If we forget to do that, this assertion will get hit, at which point it will be obvious that we missed a case.

The assertion communicates that we intend for this function to exhaustively handle all subclasses of Call without implicitly folding any unmentioned subclasses into the code handling EventCall.

Another way to write this would be:

if isinstance(ins, HighLevelCall):
  ...
... more cases here ...
else isinstance(ins, EventCall):
  ...
else:
  assert False 

Ideally, the EventCall case would return an order, but I'm not sure how to obtain the correct order. We can create a new issue for this mentioning that parameter reordering is still broken for EventCall, NewContract, and HighLevelCalls that do not have a function field.

[0xalpharush]

Can we update the function signature to only be subclasses of call which are accepted and remove this? We can add an assertion like assert isinstance(ins, NewStructure, NewContract... ), too

[kevinclancy]

It might be better to add assert not isinstance(LowLevelCall, NewElementaryType, NewArray) at the top of the get_declared_param_names function.

We could also make the assert False case return None instead. Of course, it would be a hassle to get test coverage for this.