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: guard against pop and append for constant dynamic arrays #3189

Closed
wants to merge 14 commits into from
20 changes: 20 additions & 0 deletions tests/parser/syntax/test_constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,26 @@ def hello() :
""",
ImmutableViolation,
),
(
"""
a: constant(DynArray[uint128, 2]) = [0]

@external
def foo():
a.pop()
""",
ImmutableViolation,
),
(
"""
a: constant(DynArray[uint128, 2]) = [0]

@external
def foo():
a.append(1)
""",
ImmutableViolation,
),
]


Expand Down
46 changes: 33 additions & 13 deletions vyper/ast/folding.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from vyper.ast import nodes as vy_ast
from vyper.builtins.functions import DISPATCH_TABLE
from vyper.exceptions import UnfoldableNode, UnknownType
from vyper.semantics.types.base import VyperType
from vyper.semantics.analysis.base import DataLocation, VarInfo
from vyper.semantics.types.utils import type_from_annotation
from vyper.utils import SizeLimits

Expand Down Expand Up @@ -180,16 +180,26 @@ def replace_user_defined_constants(vyper_module: vy_ast.Module) -> int:

# Extract type definition from propagated annotation
type_ = None
var_info = None
try:
type_ = type_from_annotation(node.annotation)
var_info = VarInfo(
type_,
decl_node=node,
location=DataLocation.CODE,
is_constant=node.is_constant,
is_public=node.is_public,
is_immutable=node.is_immutable,
)

except UnknownType:
# handle user-defined types e.g. structs - it's OK to not
# propagate the type annotation here because user-defined
# types can be unambiguously inferred at typechecking time
pass

changed_nodes += replace_constant(
vyper_module, node.target.id, node.value, False, type_=type_
vyper_module, node.target.id, node.value, False, var_info=var_info
)

return changed_nodes
Expand All @@ -198,18 +208,27 @@ def replace_user_defined_constants(vyper_module: vy_ast.Module) -> int:
# TODO constant folding on log events


def _replace(old_node, new_node, type_=None):
def _replace(old_node, new_node, var_info=None):
if isinstance(new_node, vy_ast.Constant):
new_node = new_node.from_node(old_node, value=new_node.value)
if type_:
new_node._metadata["type"] = type_
if var_info is not None:
new_node._metadata["varinfo"] = var_info
return new_node
elif isinstance(new_node, vy_ast.List):
base_type = type_.value_type if type_ else None
list_values = [_replace(old_node, i, type_=base_type) for i in new_node.elements]
base_type_varinfo = None
if var_info is not None:
base_type_varinfo = VarInfo(
typ=var_info.typ.value_type,
location=DataLocation.CODE,
is_constant=var_info.is_constant,
is_public=var_info.is_public,
is_immutable=var_info.is_immutable,
decl_node=var_info.decl_node,
)
Copy link
Member

Choose a reason for hiding this comment

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

fyi - when i kind of complicated code inlined like this, i'd prefer to see a helper function to do this which is closer to the definition of VarInfo, it's too easy to update the fields of VarInfo and then forget to update all uses of the constructor. in particular, this looks like a good use case for VarInfo.copy_with_type(type_). but as i mentioned in #3189 (review), we probably want to use ExprInfo instead anyway.

list_values = [_replace(old_node, i, var_info=base_type_varinfo) for i in new_node.elements]
new_node = new_node.from_node(old_node, elements=list_values)
if type_:
new_node._metadata["type"] = type_
if var_info is not None:
new_node._metadata["varinfo"] = var_info
return new_node
elif isinstance(new_node, vy_ast.Call):
# Replace `Name` node with `Call` node
Expand All @@ -231,7 +250,7 @@ def replace_constant(
id_: str,
replacement_node: Union[vy_ast.Constant, vy_ast.List, vy_ast.Call],
raise_on_error: bool,
type_: Optional[VyperType] = None,
var_info: Optional[VarInfo] = None,
) -> int:
"""
Replace references to a variable name with a literal value.
Expand All @@ -247,8 +266,9 @@ def replace_constant(
`Call` nodes are for struct constants.
raise_on_error: bool
Boolean indicating if `UnfoldableNode` exception should be raised or ignored.
type_ : VyperType, optional
Type definition to be propagated to type checker.
var_info : VarInfo, optional
Type definition plus associated metadata like constancy attributes to be
propagated to type checker.

Returns
-------
Expand Down Expand Up @@ -286,7 +306,7 @@ def replace_constant(

try:
# note: _replace creates a copy of the replacement_node
new_node = _replace(node, replacement_node, type_=type_)
new_node = _replace(node, replacement_node, var_info=var_info)
except UnfoldableNode:
if raise_on_error:
raise
Expand Down
1 change: 0 additions & 1 deletion vyper/codegen/stmt.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,6 @@ def parse_Call(self):
"append",
"pop",
):
# TODO: consider moving this to builtins
Copy link
Collaborator Author

@tserg tserg Dec 10, 2022

Choose a reason for hiding this comment

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

I think this is an outdated comment since pop and append were moved from builtins to codegen.

darray = Expr(self.stmt.func.value, self.context).ir_node
args = [Expr(x, self.context).ir_node for x in self.stmt.args]
if self.stmt.func.attr == "append":
Expand Down
7 changes: 7 additions & 0 deletions vyper/semantics/analysis/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ def __init__(self):
self.namespace = get_namespace()

def get_expr_info(self, node: vy_ast.VyperNode) -> ExprInfo:
# Early termination if variable info is propagated in metadata
varinfo = node._metadata.get("varinfo")
if varinfo is not None:
return ExprInfo.from_varinfo(varinfo)

t = self.get_exact_type_from_node(node)

# if it's a Name, we have varinfo for it
Expand Down Expand Up @@ -136,6 +141,8 @@ def get_possible_types_from_node(self, node, include_type_exprs=False):
A list of type objects
"""
# Early termination if typedef is propagated in metadata
if "varinfo" in node._metadata:
return [node._metadata["varinfo"].typ]
if "type" in node._metadata:
return [node._metadata["type"]]

Expand Down