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

Add more types #1624

Merged
merged 20 commits into from
Feb 15, 2023
Merged

Add more types #1624

merged 20 commits into from
Feb 15, 2023

Conversation

montyly
Copy link
Member

@montyly montyly commented Jan 24, 2023

Results of monkeytype on the AST tests + manual edits

The PR is not ready for review, but I want to test the CI before doing more changes

@montyly
Copy link
Member Author

montyly commented Jan 31, 2023

Still a lot of types are missing, but the PR is ready to be reviewed

def fix_phi(self, last_state_variables_instances, initial_state_variables_instances):
def fix_phi(
self,
last_state_variables_instances: Dict[
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be simplified to List[Union[StateIRVariable", StateIRVariable"]]


class NewArray(Expression):

# note: dont conserve the size of the array if provided
def __init__(self, depth, array_type):
def __init__(
self, depth: int, array_type: Union["TypeAliasTopLevel", "ElementaryType"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right but I'm not sure what it should be

def __init__(self, t, length):
def __init__(
self,
t: Union["TypeAliasTopLevel", "ArrayType", "FunctionType", "ElementaryType"],
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right

def __init__(
self,
t: Union["TypeAliasTopLevel", "ArrayType", "FunctionType", "ElementaryType"],
length: Optional[Union["Identifier", Literal, "BinaryOperation"]],
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be int


# pylint: disable=import-outside-toplevel
class UserDefinedType(Type):
def __init__(self, t):
def __init__(
self, t: Union["EnumContract", "StructureTopLevel", "Contract", "StructureContract"]
Copy link
Contributor

Choose a reason for hiding this comment

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

There's some confusion here about whether it's a Structure or Union[StructureTopLevel, StructureContract] between the type annotations and assertions

Tuple[Node, StateVariable, FunctionContract],
Tuple[Node, LocalVariable, FunctionContract],
Any,
]
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 remove the Any?

@@ -36,7 +40,9 @@ class IncorrectERC20InterfaceDetection(AbstractDetector):
)

@staticmethod
def incorrect_erc20_interface(signature):
def incorrect_erc20_interface(
signature: Union[Tuple[str, List[str], List[Any]], Tuple[str, List[Any], List[Any]]]
Copy link
Contributor

@0xalpharush 0xalpharush Feb 1, 2023

Choose a reason for hiding this comment

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

f.signature is defined as Tuple[str, List[str]] so perhaps that can be used

) -> List[
Union[
Any,
Tuple[LocalVariable, List[Tuple[str, StateVariable]]],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be simplified to a Union on the last second element of the tuple?

@@ -300,7 +322,7 @@ def _make_function_type(func: Function) -> FunctionType:
###################################################################################


def integrate_value_gas(result):
def integrate_value_gas(result: List[Any]) -> List[Any]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe should be Operation

@@ -1114,7 +1141,7 @@ def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]): # pylint: dis
###################################################################################


def can_be_low_level(ir):
def can_be_low_level(ir: slither.slithir.operations.high_level_call.HighLevelCall) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to use this over just HighLevelCall?

def convert_assignment(left, right, t, return_type):
def convert_assignment(
left: Union[LocalVariable, StateVariable, ReferenceVariable],
right: SourceMapping,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be SourceMapping

@@ -146,10 +167,10 @@ def __init__(self, expression, node): # pylint: disable=super-init-not-called
for ir in self._result:
ir.set_node(node)

def result(self):
def result(self) -> List[Any]:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be Operation

@montyly montyly merged commit 20a7951 into dev Feb 15, 2023
@montyly montyly deleted the dev-add-types branch February 15, 2023 13: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