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

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Dec 10, 2022

What I did

Fix #3180.

Previously, constancy information was propagated to the folded node as part of the type definition. With #2974, constancy information is now contained in VarInfo. Therefore, we need to additionally propagate VarInfo.

I am not sure if there are any rules as to which nodes can have a type in its metadata, and which nodes can have a varinfo in its metadata. In the approach I have taken, only varinfo is propagated in the metadata. The tradeoff is that this requires us to check for varinfo in get_possible_types_from_node, which I am unsure if it wrongly conflates varinfo with type.

Alternatively, we can also propagate both varinfo and type in the metadata. While this is duplication, it would also mean that we do not need to check for varinfo in get_possible_types_from_node.

How I did it

Create VarInfo object during constant folding, and pass it to the folded node so that it can be retrieved for analysis under the existing validate_modification via visit_Expr in local semantic analysis.

How to verify it

See tests.

Commit message

fix: guard against `pop` and `append` for constant dynamic arrays

Description for the changelog

Guard against pop and append for constant dynamic arrays

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@@ -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.

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2022

Codecov Report

Merging #3189 (75094d3) into master (dcc230c) will increase coverage by 35.24%.
The diff coverage is 88.88%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff             @@
##           master    #3189       +/-   ##
===========================================
+ Coverage   53.43%   88.67%   +35.24%     
===========================================
  Files          86       86               
  Lines       10800    10809        +9     
  Branches     2453     2456        +3     
===========================================
+ Hits         5771     9585     +3814     
+ Misses       4493      821     -3672     
+ Partials      536      403      -133     
Impacted Files Coverage Δ
vyper/ast/folding.py 92.68% <84.61%> (+41.42%) ⬆️
vyper/semantics/analysis/utils.py 91.95% <100.00%> (+45.40%) ⬆️

... and 67 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tserg tserg marked this pull request as ready for review December 11, 2022 03:21
@charles-cooper charles-cooper added this to the v0.3.8 milestone May 14, 2023
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

i think generally, we want to use ExprInfo here rather than VarInfo, we might not be dealing with a variable for some reason.

Comment on lines 220 to 227
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.

@tserg tserg closed this Jan 4, 2024
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.

Using pop or append on constant DynArray leads to CompilerPanic
3 participants