-
-
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 inference and annotation of arguments for builtin functions #2817
fix: type inference and annotation of arguments for builtin functions #2817
Conversation
Annotation of e.g. minimal repro from examples/auctions/blind_auction.vy
Even though |
Codecov Report
@@ Coverage Diff @@
## master #2817 +/- ##
==========================================
+ Coverage 87.67% 87.94% +0.27%
==========================================
Files 94 94
Lines 10183 10257 +74
Branches 2501 2478 -23
==========================================
+ Hits 8928 9021 +93
+ Misses 790 783 -7
+ Partials 465 453 -12
Continue to review full report at Codecov.
|
it's very related to this: #2707. i think instead of having ad-hoc rules for min/max, we need to be able to annotate all builtins with the correct argument types |
Got it, will consolidate under either of these two PRs. Btw, Is it bad practice to change the type definition of an integer literal during codegen phase? Using minmax as an example where the operand is an integer literal and a variable, we may want to cast the integer literal to be of the same type as the variable. |
yeah i think that, ideally, literals should use the type definition inferred during the annotation phase. |
Interestingly, this PR seems to handle #2707. |
After looking through the builtin functions, I think the annotation/typechecking issue only arises for those that require its arguments to be of the same type (specifically min, max and unsafe maths). Only in these situations, where the arguments consist of a variable and a literal, do we need to cast the literal to be of the variable's type. |
Some convert tests involving integer literals are failing as they were treated as Example - this raises a TypeMismatch exception in 0.3.2, but is valid if we infer the type of the integer literal.
|
There is some code duplication between the new helper function and |
The issue is that fetch_call_return is a general mechanism that all Calls use. But maybe for builtins we can add an interface function like "check_and_infer_arg_types". |
Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
…into fix/minmax_typecheck
Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>
…into fix/minmax_typecheck
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.
great work! and thanks for your patience through many rounds of review
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.
Address the feedback and it has my approval to merge
@@ -2,16 +2,16 @@ def test_extract32_extraction(assert_tx_failed, get_contract_with_gas_estimation | |||
extract32_code = """ | |||
y: Bytes[100] | |||
@external | |||
def extrakt32(inp: Bytes[100], index: int128) -> bytes32: |
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.
Why was this type changed?
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.
Negative index is not allowed for the index argument extract32
even though the index type was int128
. As I understand, int128
was the only integer type when this was implemented. Since Vyper has unsigned integer types now, index type has been updated to be of UnsignedIntegerAbstractType
instead and would be more reflective as well.
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.
uint256
was added several years ago, so is kind of strange it was like that
note: @fubuloubu gave approval offline |
What I did
Fix #2706, fix #2788, fix #2819, fix #2827
How I did it
AnnotateInt
Vyper nodes.For min-max and unsafe maths with a literal and a variable, annotate theInt
Vyper node with the variable type. Subsequently, use the propagated type in codegen._validate_arg_types
helper function.infer_arg_type
function._BuiltinFunction
class has been removed and subsumed under_SimpleBuiltinFunction
.Optional
class toKwargSettings
.TypeTypeDefinition
to handle nodes representing pure typestrings.validate_inputs
andprocess_arg
forbuiltin_functions/signatures
.For
_UnsafeMath
and_MinMax
, the type inferred is the common type of both operands.In addition, validation of arguments is moved from
fetch_call_return
to the newinfer_arg_type
function.How to verify it
See tests.
Commit message
Description for the changelog
Type inference and annotation of arguments for builtin functions
Cute Animal Picture