Open
Conversation
WalkthroughConstant folding capabilities have been added to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Compiler
participant Convert/Slice/Extract32
User->>Compiler: Provide code with constant expressions
Compiler->>Convert/Slice/Extract32: Call _try_fold on constant nodes
Convert/Slice/Extract32-->>Compiler: Return folded constant or raise exception
Compiler-->>User: Output optimized code with folded constants
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
vyper/builtins/functions.py (1)
317-349: Minor: preserve literal kind & length metadata in foldedslice()The folding returns a generic
vy_ast.Bytes(orvy_ast.Str) without
setting.length, losing information that could still be statically
known and useful to later passes / optimisers (e.g. gas estimates).Consider:
- result = value.value[start_val:start_val + length_val] - return vy_ast.Bytes.from_node(node, value=result) + result = value.value[start_val : start_val + length_val] + lit = vy_ast.Bytes.from_node(node, value=result) + lit._metadata["type"] = BytesT(length_val) + return litSame applies to the
vy_ast.Hexbranch which also converts the value to
Bytes– if this is intentional, add a short comment.
Comment on lines
245
to
249
| def infer_arg_types(self, node, expected_return_typ=None): | ||
| validate_call_args(node, 2) | ||
|
|
||
| self._validate_arg_types(node) | ||
| value_type = get_possible_types_from_node(node.args[0]).pop() | ||
| target_type = type_from_annotation(node.args[1]) | ||
| value_types = get_possible_types_from_node(node.args[0]) | ||
|
|
||
| # For `convert` of integer literals, we need to match type inference rules in | ||
| # convert.py codegen routines. | ||
| # TODO: This can probably be removed once constant folding for `convert` is implemented | ||
| if len(value_types) > 1 and all(isinstance(v, IntegerT) for v in value_types): | ||
| # Get the smallest (and unsigned if available) type for non-integer target types | ||
| # (note this is different from the ordering returned by `get_possible_types_from_node`) | ||
| if not isinstance(target_type, IntegerT): | ||
| value_types = sorted(value_types, key=lambda v: (v.is_signed, v.bits), reverse=True) | ||
| else: | ||
| # filter out the target type from list of possible types | ||
| value_types = [i for i in value_types if not target_type.compare_type(i)] | ||
|
|
||
| value_type = value_types.pop() | ||
|
|
||
| # block conversions between same type | ||
| if target_type.compare_type(value_type): | ||
| raise InvalidType(f"Value and target type are both '{target_type}'", node) | ||
|
|
||
| return [value_type, TYPE_T(target_type)] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Non-deterministic type selection in infer_arg_types
get_possible_types_from_node(...).pop() takes an arbitrary element from a set.
Two identical compilations can therefore pick different source types when multiple are possible, leading to flaky error messages or, worse, silently different IR.
Use a deterministic selection strategy, e.g. sort by priority:
- value_type = get_possible_types_from_node(node.args[0]).pop()
+ possible = sorted(
+ get_possible_types_from_node(node.args[0]),
+ key=lambda t: (t.typ, getattr(t, "bits", 0)) # or any stable rule
+ )
+ value_type = possible[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def infer_arg_types(self, node, expected_return_typ=None): | |
| validate_call_args(node, 2) | |
| self._validate_arg_types(node) | |
| value_type = get_possible_types_from_node(node.args[0]).pop() | |
| target_type = type_from_annotation(node.args[1]) | |
| value_types = get_possible_types_from_node(node.args[0]) | |
| # For `convert` of integer literals, we need to match type inference rules in | |
| # convert.py codegen routines. | |
| # TODO: This can probably be removed once constant folding for `convert` is implemented | |
| if len(value_types) > 1 and all(isinstance(v, IntegerT) for v in value_types): | |
| # Get the smallest (and unsigned if available) type for non-integer target types | |
| # (note this is different from the ordering returned by `get_possible_types_from_node`) | |
| if not isinstance(target_type, IntegerT): | |
| value_types = sorted(value_types, key=lambda v: (v.is_signed, v.bits), reverse=True) | |
| else: | |
| # filter out the target type from list of possible types | |
| value_types = [i for i in value_types if not target_type.compare_type(i)] | |
| value_type = value_types.pop() | |
| # block conversions between same type | |
| if target_type.compare_type(value_type): | |
| raise InvalidType(f"Value and target type are both '{target_type}'", node) | |
| return [value_type, TYPE_T(target_type)] | |
| def infer_arg_types(self, node, expected_return_typ=None): | |
| self._validate_arg_types(node) | |
| - value_type = get_possible_types_from_node(node.args[0]).pop() | |
| + possible = sorted( | |
| + get_possible_types_from_node(node.args[0]), | |
| + key=lambda t: (t.typ, getattr(t, "bits", 0)) # stable ordering | |
| + ) | |
| + value_type = possible[0] | |
| target_type = type_from_annotation(node.args[1]) | |
| return [value_type, TYPE_T(target_type)] |
Comment on lines
+893
to
+915
| def _try_fold(self, node): | ||
| validate_call_args(node, 2) | ||
| value = node.args[0].get_folded_value() | ||
| start = node.args[1].get_folded_value() | ||
|
|
||
| if not isinstance(start, vy_ast.Int): | ||
| raise UnfoldableNode | ||
|
|
||
| start_val = start.value | ||
|
|
||
| if isinstance(value, vy_ast.Bytes): | ||
| if start_val + 32 > len(value.value): | ||
| raise ArgumentException("extract32 out of bounds", node) | ||
| result = value.value[start_val:start_val + 32] | ||
| return vy_ast.Bytes.from_node(node, value=result) | ||
| elif isinstance(value, vy_ast.Hex): | ||
| if start_val + 32 > len(value.bytes_value): | ||
| raise ArgumentException("extract32 out of bounds", node) | ||
| result = value.bytes_value[start_val:start_val + 32] | ||
| return vy_ast.Bytes.from_node(node, value=result) | ||
| else: | ||
| raise UnfoldableNode | ||
|
|
There was a problem hiding this comment.
extract32() folding ignores output_type kwarg and returns wrong AST node
Issues:
- The kw-argument
output_type(defaultbytes32) is not examined, so
extract32(b, 0, output_type=int128)still folds to aBytesnode. - Even for the default case the function emits a dynamic
Bytes
literal, not a fixed-lengthBytesM_T(32)orvy_ast.Hex, breaking
the return-type contract. - No negative-index / bounds checking for
start < 0.
@@
- result = value.value[start_val:start_val + 32]
- return vy_ast.Bytes.from_node(node, value=result)
+ result = value.value[start_val : start_val + 32]
+
+ out_typedef = self.infer_kwarg_types(node)["output_type"].typedef
+
+ if isinstance(out_typedef, BytesM_T):
+ return vy_ast.Hex.from_node(node, value=result.hex())
+ elif isinstance(out_typedef, IntegerT):
+ return vy_ast.Int.from_node(node, value=int.from_bytes(result, "big"))
+ elif isinstance(out_typedef, AddressT):
+ return vy_ast.Hex.from_node(node, value="0x" + result.hex())
+ else:
+ raise CompilerPanic("Unhandled output_type in extract32 folding")Failing to honour output_type can propagate a mismatched type deep into
IR generation and surface as opaque post-optimisation crashes.
Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+194
to
+238
| def _try_fold(self, node): | ||
| validate_call_args(node, 2) | ||
| value = node.args[0].get_folded_value() | ||
| target_type = node.args[1].get_folded_value() | ||
| if not isinstance(target_type, vy_ast.Name): | ||
| raise UnfoldableNode | ||
|
|
||
| # Get the target type | ||
| target_typedef = type_from_annotation(target_type) | ||
| if not isinstance(target_typedef, (BoolT, AddressT, IntegerT, BytesM_T)): | ||
| raise UnfoldableNode | ||
|
|
||
| # Convert the value based on target type | ||
| if isinstance(target_typedef, BoolT): | ||
| if isinstance(value, vy_ast.Int): | ||
| result = bool(value.value) | ||
| elif isinstance(value, vy_ast.Bool): | ||
| result = value.value | ||
| else: | ||
| raise UnfoldableNode | ||
| return vy_ast.Bool.from_node(node, value=result) | ||
|
|
||
| elif isinstance(target_typedef, AddressT): | ||
| if isinstance(value, vy_ast.Hex): | ||
| result = value.value | ||
| else: | ||
| raise UnfoldableNode | ||
| return vy_ast.Hex.from_node(node, value=result) | ||
|
|
||
| elif isinstance(target_typedef, IntegerT): | ||
| if isinstance(value, (vy_ast.Int, vy_ast.Decimal)): | ||
| result = int(value.value) | ||
| else: | ||
| raise UnfoldableNode | ||
| return vy_ast.Int.from_node(node, value=result) | ||
|
|
||
| elif isinstance(target_typedef, BytesM_T): | ||
| if isinstance(value, vy_ast.Hex): | ||
| result = value.value | ||
| else: | ||
| raise UnfoldableNode | ||
| return vy_ast.Hex.from_node(node, value=result) | ||
|
|
||
| raise UnfoldableNode | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constant-folded convert() skips range / size validation – can emit ill-typed literals at compile time
The new folding branch happily constructs the target literal without any of the safety checks that the runtime implementation performs:
- For
IntegerTtargets noast_bounds(min / max) verification is done, soconvert(2**300, int128)would now be folded into an out-of-rangevy_ast.Intwhich later passes type-checking because its value has already been frozen. - For
AddressTthe hex literal length is not verified (should be exactly 20 bytes / 40 hex chars). - For
BytesM_Tthe produced literal is not checked againsttarget_typedef.length. - Silent narrowing of
DecimalT → IntegerTtruncates without a bounds check or aZeroDivisionException- style guard used elsewhere.
This opens a correctness hole: an invalid literal is accepted during folding and only surfaces at execution, bypassing compile-time guarantees.
@@
- elif isinstance(target_typedef, IntegerT):
- if isinstance(value, (vy_ast.Int, vy_ast.Decimal)):
- result = int(value.value)
- else:
- raise UnfoldableNode
- return vy_ast.Int.from_node(node, value=result)
+ elif isinstance(target_typedef, IntegerT):
+ if not isinstance(value, (vy_ast.Int, vy_ast.Decimal)):
+ raise UnfoldableNode
+ result = int(value.value)
+ # validate range ↓↓↓
+ lo, hi = target_typedef.ast_bounds
+ if result < lo or result > hi:
+ raise InvalidLiteral(
+ f"value {result} out of range for {target_typedef}", node.args[0]
+ )
+ return vy_ast.Int.from_node(node, value=result)Similar guards are needed for the AddressT and BytesM_T branches.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _try_fold(self, node): | |
| validate_call_args(node, 2) | |
| value = node.args[0].get_folded_value() | |
| target_type = node.args[1].get_folded_value() | |
| if not isinstance(target_type, vy_ast.Name): | |
| raise UnfoldableNode | |
| # Get the target type | |
| target_typedef = type_from_annotation(target_type) | |
| if not isinstance(target_typedef, (BoolT, AddressT, IntegerT, BytesM_T)): | |
| raise UnfoldableNode | |
| # Convert the value based on target type | |
| if isinstance(target_typedef, BoolT): | |
| if isinstance(value, vy_ast.Int): | |
| result = bool(value.value) | |
| elif isinstance(value, vy_ast.Bool): | |
| result = value.value | |
| else: | |
| raise UnfoldableNode | |
| return vy_ast.Bool.from_node(node, value=result) | |
| elif isinstance(target_typedef, AddressT): | |
| if isinstance(value, vy_ast.Hex): | |
| result = value.value | |
| else: | |
| raise UnfoldableNode | |
| return vy_ast.Hex.from_node(node, value=result) | |
| elif isinstance(target_typedef, IntegerT): | |
| if isinstance(value, (vy_ast.Int, vy_ast.Decimal)): | |
| result = int(value.value) | |
| else: | |
| raise UnfoldableNode | |
| return vy_ast.Int.from_node(node, value=result) | |
| elif isinstance(target_typedef, BytesM_T): | |
| if isinstance(value, vy_ast.Hex): | |
| result = value.value | |
| else: | |
| raise UnfoldableNode | |
| return vy_ast.Hex.from_node(node, value=result) | |
| raise UnfoldableNode | |
| def _try_fold(self, node): | |
| validate_call_args(node, 2) | |
| value = node.args[0].get_folded_value() | |
| target_type = node.args[1].get_folded_value() | |
| if not isinstance(target_type, vy_ast.Name): | |
| raise UnfoldableNode | |
| # Get the target type | |
| target_typedef = type_from_annotation(target_type) | |
| if not isinstance(target_typedef, (BoolT, AddressT, IntegerT, BytesM_T)): | |
| raise UnfoldableNode | |
| # Convert the value based on target type | |
| if isinstance(target_typedef, BoolT): | |
| if isinstance(value, vy_ast.Int): | |
| result = bool(value.value) | |
| elif isinstance(value, vy_ast.Bool): | |
| result = value.value | |
| else: | |
| raise UnfoldableNode | |
| return vy_ast.Bool.from_node(node, value=result) | |
| elif isinstance(target_typedef, AddressT): | |
| if isinstance(value, vy_ast.Hex): | |
| result = value.value | |
| else: | |
| raise UnfoldableNode | |
| return vy_ast.Hex.from_node(node, value=result) | |
| elif isinstance(target_typedef, IntegerT): | |
| if not isinstance(value, (vy_ast.Int, vy_ast.Decimal)): | |
| raise UnfoldableNode | |
| result = int(value.value) | |
| # validate range | |
| lo, hi = target_typedef.ast_bounds | |
| if result < lo or result > hi: | |
| raise InvalidLiteral( | |
| f"value {result} out of range for {target_typedef}", node.args[0] | |
| ) | |
| return vy_ast.Int.from_node(node, value=result) | |
| elif isinstance(target_typedef, BytesM_T): | |
| if isinstance(value, vy_ast.Hex): | |
| result = value.value | |
| else: | |
| raise UnfoldableNode | |
| return vy_ast.Hex.from_node(node, value=result) | |
| raise UnfoldableNode |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
How I did it
How to verify it
Commit message
Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)
Description for the changelog
Cute Animal Picture
Summary by CodeRabbit