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: reorder named arguments to match declaration order #1949

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions slither/core/expressions/call_expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,47 @@


class CallExpression(Expression): # pylint: disable=too-many-instance-attributes
def __init__(self, called: Expression, arguments: List[Any], type_call: str) -> None:
def __init__(
self,
called: Expression,
arguments: List[Any],
type_call: str,
names: Optional[List[str]] = None,
) -> None:
"""
#### Parameters
called -
The expression denoting the function to be called
arguments -
List of argument expressions
type_call -
A string formatting of the called function's return type
names -
For calls with named fields, list fields in call order.
For calls without named fields, None.
"""
assert isinstance(called, Expression)
assert (names is None) or isinstance(names, list)
super().__init__()
self._called: Expression = called
self._arguments: List[Expression] = arguments
self._type_call: str = type_call
self._names: Optional[List[str]] = names
# gas and value are only available if the syntax is {gas: , value: }
# For the .gas().value(), the member are considered as function call
# And converted later to the correct info (convert.py)
self._gas: Optional[Expression] = None
self._value: Optional[Expression] = None
self._salt: Optional[Expression] = None

@property
def names(self) -> Optional[List[str]]:
"""
For calls with named fields, list fields in call order.
For calls without named fields, None.
"""
return self._names

@property
def call_value(self) -> Optional[Expression]:
return self._value
Expand Down Expand Up @@ -62,4 +90,9 @@ def __str__(self) -> str:
if gas or value or salt:
options = [gas, value, salt]
txt += "{" + ",".join([o for o in options if o != ""]) + "}"
return txt + "(" + ",".join([str(a) for a in self._arguments]) + ")"
args = (
"{" + ",".join([f"{n}:{str(a)}" for (a, n) in zip(self._arguments, self._names)]) + "}"
if self._names is not None
else ",".join([str(a) for a in self._arguments])
)
return txt + "(" + args + ")"
79 changes: 76 additions & 3 deletions slither/slithir/convert.py
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,70 @@ def integrate_value_gas(result: List[Operation]) -> List[Operation]:
###################################################################################


def get_declared_param_names(ins: Call) -> Optional[List[str]]:
"""
Given a call operation, return the list of parameter names, in the order
listed in the function declaration.
#### Parameters
ins -
The call instruction
#### Possible Returns
List[str] -
A list of the parameters in declaration order
None -
Workaround: Unable to obtain list of parameters in declaration order
"""
if isinstance(ins, NewStructure):
return [x.name for x in ins.structure.elems_ordered if not isinstance(x.type, MappingType)]
if isinstance(ins, NewContract):
return [p.name for p in ins.contract.constructor.parameters]
if isinstance(ins, (LowLevelCall, NewElementaryType, NewArray)):
# named arguments are incompatible with these call forms
assert False
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if isinstance(ins, HighLevelCall) and isinstance(ins.function, str):
return None
if isinstance(ins, (InternalCall, LibraryCall, HighLevelCall)):
if isinstance(ins.function, Function):
return [p.name for p in ins.function.parameters]
return None
if isinstance(ins, InternalDynamicCall):
return [p.name for p in ins.function_type.params]

assert isinstance(ins, EventCall)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@kevinclancy kevinclancy Jul 24, 2023

Choose a reason for hiding this comment

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

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.

return None


def reorder_arguments(
args: List[Variable], call_names: List[str], decl_names: List[str]
) -> List[Variable]:
"""
Reorder named struct constructor arguments so that they match struct declaration ordering rather
than call ordering
E.g. for `struct S { int x; int y; }` we reorder `S({y : 2, x : 3})` to `S(3, 2)`
#### Parameters
args -
Arguments to constructor call, in call order
names -
Parameter names in call order
decl_names -
Parameter names in declaration order
#### Returns
Reordered arguments to constructor call, now in declaration order
"""
assert isinstance(args, list)
assert isinstance(call_names, list)
assert isinstance(decl_names, list)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

assert len(args) == len(call_names)
assert len(call_names) == len(decl_names)

args_ret = []
for n in decl_names:
ind = call_names.index(n)
args_ret.append(args[ind])

return args_ret


def propagate_type_and_convert_call(result: List[Operation], node: "Node") -> List[Operation]:
"""
Propagate the types variables and convert tmp call to real call operation
Expand Down Expand Up @@ -434,6 +498,11 @@ def propagate_type_and_convert_call(result: List[Operation], node: "Node") -> Li
if ins.call_id in calls_gas and isinstance(ins, (HighLevelCall, InternalDynamicCall)):
ins.call_gas = calls_gas[ins.call_id]

if isinstance(ins, Call) and (ins.names is not None):
decl_param_names = get_declared_param_names(ins)
if decl_param_names is not None:
call_data = reorder_arguments(call_data, ins.names, decl_param_names)

if isinstance(ins, (Call, NewContract, NewStructure)):
# We might have stored some arguments for libraries
if ins.arguments:
Expand Down Expand Up @@ -855,7 +924,7 @@ def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]) -> Union[Call,
if isinstance(ins.ori.variable_left, Contract):
st = ins.ori.variable_left.get_structure_from_name(ins.ori.variable_right)
if st:
op = NewStructure(st, ins.lvalue)
op = NewStructure(st, ins.lvalue, names=ins.names)
op.set_expression(ins.expression)
op.call_id = ins.call_id
return op
Expand Down Expand Up @@ -892,6 +961,7 @@ def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]) -> Union[Call,
ins.nbr_arguments,
ins.lvalue,
ins.type_call,
names=ins.names,
)
libcall.set_expression(ins.expression)
libcall.call_id = ins.call_id
Expand Down Expand Up @@ -950,6 +1020,7 @@ def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]) -> Union[Call,
len(lib_func.parameters),
ins.lvalue,
"d",
names=ins.names,
)
lib_call.set_expression(ins.expression)
lib_call.set_node(ins.node)
Expand Down Expand Up @@ -1031,6 +1102,7 @@ def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]) -> Union[Call,
ins.nbr_arguments,
ins.lvalue,
ins.type_call,
names=ins.names,
)
msgcall.call_id = ins.call_id

Expand Down Expand Up @@ -1082,7 +1154,7 @@ def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]) -> Union[Call,
return n

if isinstance(ins.called, Structure):
op = NewStructure(ins.called, ins.lvalue)
op = NewStructure(ins.called, ins.lvalue, names=ins.names)
op.set_expression(ins.expression)
op.call_id = ins.call_id
op.set_expression(ins.expression)
Expand All @@ -1106,7 +1178,7 @@ def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]) -> Union[Call,
if len(ins.called.constructor.parameters) != ins.nbr_arguments:
return Nop()
internalcall = InternalCall(
ins.called.constructor, ins.nbr_arguments, ins.lvalue, ins.type_call
ins.called.constructor, ins.nbr_arguments, ins.lvalue, ins.type_call, ins.names
)
internalcall.call_id = ins.call_id
internalcall.set_expression(ins.expression)
Expand Down Expand Up @@ -1439,6 +1511,7 @@ def look_for_library_or_top_level(
ir.nbr_arguments,
ir.lvalue,
ir.type_call,
names=ir.names,
)
lib_call.set_expression(ir.expression)
lib_call.set_node(ir.node)
Expand Down
18 changes: 17 additions & 1 deletion slither/slithir/operations/call.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,25 @@


class Call(Operation):
def __init__(self) -> None:
def __init__(self, names: Optional[List[str]] = None) -> None:
"""
#### Parameters
names -
For calls of the form f({argName1 : arg1, ...}), the names of parameters listed in call order.
Otherwise, None.
"""
assert (names is None) or isinstance(names, list)
super().__init__()
self._arguments: List[Variable] = []
self._names = names

@property
def names(self) -> Optional[List[str]]:
"""
For calls of the form f({argName1 : arg1, ...}), the names of parameters listed in call order.
Otherwise, None.
"""
return self._names

@property
def arguments(self) -> List[Variable]:
Expand Down
9 changes: 8 additions & 1 deletion slither/slithir/operations/high_level_call.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,18 @@ def __init__(
nbr_arguments: int,
result: Optional[Union[TemporaryVariable, TupleVariable, TemporaryVariableSSA]],
type_call: str,
names: Optional[List[str]] = None,
) -> None:
"""
#### Parameters
names -
For calls of the form f({argName1 : arg1, ...}), the names of parameters listed in call order.
Otherwise, None.
"""
assert isinstance(function_name, Constant)
assert is_valid_lvalue(result) or result is None
self._check_destination(destination)
super().__init__()
super().__init__(names=names)
# Contract is only possible for library call, which inherits from highlevelcall
self._destination: Union[Variable, SolidityVariable, Contract] = destination # type: ignore
self._function_name = function_name
Expand Down
10 changes: 9 additions & 1 deletion slither/slithir/operations/internal_call.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,16 @@ def __init__(
Union[TupleVariableSSA, TemporaryVariableSSA, TupleVariable, TemporaryVariable]
],
type_call: str,
names: Optional[List[str]] = None,
) -> None:
super().__init__()
# pylint: disable=too-many-arguments
"""
#### Parameters
names -
For calls of the form f({argName1 : arg1, ...}), the names of parameters listed in call order.
Otherwise, None.
"""
super().__init__(names=names)
self._contract_name = ""
if isinstance(function, Function):
self._function: Optional[Function] = function
Expand Down
13 changes: 11 additions & 2 deletions slither/slithir/operations/new_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,20 @@

class NewContract(Call, OperationWithLValue): # pylint: disable=too-many-instance-attributes
def __init__(
self, contract_name: Constant, lvalue: Union[TemporaryVariableSSA, TemporaryVariable]
self,
contract_name: Constant,
lvalue: Union[TemporaryVariableSSA, TemporaryVariable],
names: Optional[List[str]] = None,
) -> None:
"""
#### Parameters
names -
For calls of the form f({argName1 : arg1, ...}), the names of parameters listed in call order.
Otherwise, None.
"""
assert isinstance(contract_name, Constant)
assert is_valid_lvalue(lvalue)
super().__init__()
super().__init__(names=names)
self._contract_name = contract_name
# todo create analyze to add the contract instance
self._lvalue = lvalue
Expand Down
11 changes: 9 additions & 2 deletions slither/slithir/operations/new_structure.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import List, Union
from typing import List, Optional, Union

from slither.slithir.operations.call import Call
from slither.slithir.operations.lvalue import OperationWithLValue
Expand All @@ -17,8 +17,15 @@ def __init__(
self,
structure: StructureContract,
lvalue: Union[TemporaryVariableSSA, TemporaryVariable],
names: Optional[List[str]] = None,
) -> None:
super().__init__()
"""
#### Parameters
names -
For calls of the form f({argName1 : arg1, ...}), the names of parameters listed in call order.
Otherwise, None.
"""
super().__init__(names=names)
assert isinstance(structure, Structure)
assert is_valid_lvalue(lvalue)
self._structure = structure
Expand Down
19 changes: 18 additions & 1 deletion slither/slithir/tmp_operations/tmp_call.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Optional, Union
from typing import List, Optional, Union

from slither.core.declarations import (
Event,
Expand All @@ -25,7 +25,15 @@ def __init__(
nbr_arguments: int,
result: Union[TupleVariable, TemporaryVariable],
type_call: str,
names: Optional[List[str]] = None,
) -> None:
# pylint: disable=too-many-arguments
"""
#### Parameters
names -
For calls of the form f({argName1 : arg1, ...}), the names of parameters listed in call order.
Otherwise, None.
"""
assert isinstance(
called,
(
Expand All @@ -42,13 +50,22 @@ def __init__(
self._called = called
self._nbr_arguments = nbr_arguments
self._type_call = type_call
self._names = names
self._lvalue = result
self._ori = None #
self._callid = None
self._gas = None
self._value = None
self._salt = None

@property
def names(self) -> Optional[List[str]]:
"""
For calls of the form f({argName1 : arg1, ...}), the names of parameters listed in call order.
Otherwise, None.
"""
return self._names

@property
def call_value(self):
return self._value
Expand Down
15 changes: 11 additions & 4 deletions slither/slithir/utils/ssa.py
Original file line number Diff line number Diff line change
Expand Up @@ -735,12 +735,17 @@ def copy_ir(ir: Operation, *instances) -> Operation:
destination = get_variable(ir, lambda x: x.destination, *instances)
function_name = ir.function_name
nbr_arguments = ir.nbr_arguments
names = ir.names
lvalue = get_variable(ir, lambda x: x.lvalue, *instances)
type_call = ir.type_call
if isinstance(ir, LibraryCall):
new_ir = LibraryCall(destination, function_name, nbr_arguments, lvalue, type_call)
new_ir = LibraryCall(
destination, function_name, nbr_arguments, lvalue, type_call, names=names
)
else:
new_ir = HighLevelCall(destination, function_name, nbr_arguments, lvalue, type_call)
new_ir = HighLevelCall(
destination, function_name, nbr_arguments, lvalue, type_call, names=names
)
new_ir.call_id = ir.call_id
new_ir.call_value = get_variable(ir, lambda x: x.call_value, *instances)
new_ir.call_gas = get_variable(ir, lambda x: x.call_gas, *instances)
Expand All @@ -761,7 +766,8 @@ def copy_ir(ir: Operation, *instances) -> Operation:
nbr_arguments = ir.nbr_arguments
lvalue = get_variable(ir, lambda x: x.lvalue, *instances)
type_call = ir.type_call
new_ir = InternalCall(function, nbr_arguments, lvalue, type_call)
names = ir.names
new_ir = InternalCall(function, nbr_arguments, lvalue, type_call, names=names)
new_ir.arguments = get_arguments(ir, *instances)
return new_ir
if isinstance(ir, InternalDynamicCall):
Expand Down Expand Up @@ -811,7 +817,8 @@ def copy_ir(ir: Operation, *instances) -> Operation:
if isinstance(ir, NewStructure):
structure = ir.structure
lvalue = get_variable(ir, lambda x: x.lvalue, *instances)
new_ir = NewStructure(structure, lvalue)
names = ir.names
new_ir = NewStructure(structure, lvalue, names=names)
new_ir.arguments = get_arguments(ir, *instances)
return new_ir
if isinstance(ir, Nop):
Expand Down
Loading