-
-
Notifications
You must be signed in to change notification settings - Fork 793
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: type check for constants #2603
fix: type check for constants #2603
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2603 +/- ##
==========================================
- Coverage 86.59% 86.53% -0.06%
==========================================
Files 91 92 +1
Lines 9438 9472 +34
Branches 2362 2403 +41
==========================================
+ Hits 8173 8197 +24
- Misses 777 785 +8
- Partials 488 490 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tserg thanks for the contribution! I think this PR will be cleaner if you reuse existing machinery for parsing types, and then follow the convention of putting type objects (e.g. some kind of TypeDefinition
) into node._metadata["type"]
vyper/ast/folding.py
Outdated
return new_node.from_node(old_node, value=new_node.value) | ||
new_node = new_node.from_node(old_node, value=new_node.value) | ||
if type: | ||
new_node._metadata["type"] = type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't node._metadata["type"]
usually some kind of type object, not a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is usually a TypeDefinition
. My initial thinking was that TypeDefinition
objects are assigned at a later stage under semantics
, and it could be inappropriate/messy/premature to import those classes and helper functions into the ast
module. However, as you pointed out, the inconsistency/messiness ends up in the _metadata["type"]
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right that's very interesting, I forgot type checking happens after constant folding. We should consider switching the order of the phases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried moving the type check phase before constant folding. The good news is that it resolves the issue being addressed in this PR. The bad news is that there are about another 100 test cases that fail.
Some of the type-checking issues raised are due to:
- Values for built-in constants such as
ZERO_ADDRESS
not being injected, and therefore not available during type checking. - Values for user-defined constants in cases where they are used as subscript or loop ranges not being injected, and therefore not available during type checking.
The remaining issues will require more investigation (e.g. type mismatches, method id must be folded, missing "type" key in metadata).
At the moment, it seems that switching phases is more complex than originally thought. I was thinking that we may need to split up the constant folding phase, but even that may be insufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, we should not use the "type"
key because having "type"
be polymorphic is really messy. We should think about a more long-term solution but for now I think we can attach a "constant_annotation"
, which we can inspect during the annotation / type checking phase. Also I would just attach the entire annotation (AST node) instead of converting it to a string.
Update: I tried to refactor the parametrizing of distinct integer types for the negative test cases to reduce duplication and potentially in view of a much bigger list of integer types. However, it seems that |
This pull request introduces 1 alert when merging 63eb93b into 2735f02 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging a96340c into 2735f02 - view on LGTM.com new alerts:
|
vyper/semantics/validation/utils.py
Outdated
propagated_type = propagated_type.value_type | ||
|
||
for _, t in enumerate(types_list): | ||
if type(propagated_type) is type(t): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
direct type comparison always jumps out at me .. can you help me understand what the intention of this loop is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is to ensure that the propagated type is one of the possible types retrieved for that node so as to maintain some logical consistency with what the get_possible_types_from_node
function is already doing, and also potentially as a failsafe in some edge case where the propagated type may not be one of the possible types retrieved.
In this approach, we would need to do a type comparison because the definition classes are created as different objects even if they both represent the same base type.
The test suite still passes if we replace this loop with the following statement:
types_list = [propagated_type]
If this is the preferred option, then we can shift this segment for constant typedef propagation to the beginning of the function so that the propagated type is taken to be the default possible type and returned immediately. There would be no need to retrieve the possible types for this node. We can view this function as then differentiating between nodes with constant typedef propagation metadata and nodes without.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative to type comparison in the existing approach is to compare the __repr__()
string representations.
previously the constant folding stage would forget the annotated types of user-defined constants, letting the typechecker treat them like normal literals. this would allow code like the following to compile: ``` MY_CONSTANT: constant(uint256) = 1 @external def foo() -> uint8: return MY_CONSTANT ``` this commit propagates the annotated types of user-defined constants so that they get typechecked properly.
What I did
Fix #2592.
Based on the issue, the following code compiles when it should not:
While looking into this, I discovered two other similar code snippets that also compile when they should not:
The bug does not appear to be limited to constant arrays, however. The following code snippet also compiles when it should not:
How I did it
Propagate the type definition of a constant when it is folded (i.e. replaced by a new AST node) by appending this information as a string to the new node's
_metadata
attribute using thetype
key. This value is then used to narrow down the possible type of a node from a list of possible types.How to verify it
See tests.
Description for the changelog
Fix type checking for constants
Cute Animal Picture