Conversation
WalkthroughConstant folding capabilities were implemented for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AST_Node
participant Convert/Slice/Extract32
participant Compiler
User->>AST_Node: Provides literal operation (convert/slice/extract)
AST_Node->>Convert/Slice/Extract32: Calls _try_fold with node
Convert/Slice/Extract32->>Convert/Slice/Extract32: Validate types, perform folding
Convert/Slice/Extract32-->>AST_Node: Return folded node or raise error
AST_Node->>Compiler: Passes folded node for further compilation
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: 0
🧹 Nitpick comments (1)
vyper/builtins/functions.py (1)
194-279: Good implementation of constant folding with comprehensive type validationThe new
_try_foldmethod for theConvertbuiltin adds constant folding capabilities for conversions between common types (BoolT, AddressT, IntegerT, BytesM_T). This enables compile-time evaluation of literal conversions, which should improve efficiency.There are two minor issues in the exception handling:
- except ValueError: - raise InvalidLiteral("Invalid hex literal for address", node.args[0]) + except ValueError: + raise InvalidLiteral("Invalid hex literal for address", node.args[0]) from None - except ValueError: - raise InvalidLiteral("Invalid hex literal for bytes", node.args[0]) + except ValueError: + raise InvalidLiteral("Invalid hex literal for bytes", node.args[0]) from NoneUsing
from Noneclarifies that these are new exceptions rather than propagations of the original errors.🧰 Tools
🪛 Ruff (0.8.2)
224-224: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
264-264: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vyper/builtins/functions.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
vyper/builtins/functions.py (7)
vyper/builtins/_signatures.py (2)
_validate_arg_types(107-136)infer_kwarg_types(158-159)vyper/ast/validation.py (1)
validate_call_args(9-97)vyper/ast/nodes.py (9)
get_folded_value(397-405)Name(967-968)Int(787-797)from_node(296-319)Hex(832-874)bytes_value(870-874)Decimal(800-829)Bytes(889-902)Str(877-886)vyper/exceptions.py (3)
UnfoldableNode(429-430)InvalidLiteral(263-264)ArgumentException(287-288)vyper/semantics/types/utils.py (1)
type_from_annotation(72-103)vyper/semantics/types/primitives.py (8)
BoolT(30-50)AddressT(406-433)IntegerT(255-329)BytesM_T(57-132)ast_bounds(144-145)ast_bounds(285-286)ast_bounds(389-390)length(78-79)vyper/semantics/analysis/utils.py (2)
get_possible_types_from_node(136-174)get_possible_types_from_node(452-468)
🪛 Ruff (0.8.2)
vyper/builtins/functions.py
224-224: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
264-264: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (6)
vyper/builtins/functions.py (6)
281-284: Simplified return type handlingThe method now gets the target type from
infer_arg_typesand returns its typedef, making the code more concise and easier to follow.
285-293: Improved type inference with deterministic orderingThe method now sorts possible input types by name and bit size before selecting a candidate, ensuring deterministic behavior when multiple types are possible.
361-404: Comprehensive constant folding implementation for Slice operationsThe new
_try_foldmethod for theSlicebuiltin enables compile-time evaluation of slice operations on literals of typesBytes,Str, andHex. The implementation includes thorough input validation:
- Checks that start and length arguments are integers
- Validates that length is at least 1
- Ensures start index is non-negative
- Verifies that the requested slice is within bounds
This will both optimize code and catch errors at compile time rather than runtime.
449-456: Consistent type inference approach for Slice functionLike the Convert function, this method now sorts possible input types by name and bit size before selecting a candidate, ensuring deterministic behavior.
951-995: Well-implemented constant folding for Extract32 operationsThe new
_try_foldmethod for theExtract32builtin enables compile-time evaluation of extraction operations on literals of typesBytesandHex. The implementation includes:
- Comprehensive bounds checking (start + 32 must not exceed source length)
- Type-appropriate handling of the extracted data depending on the requested output type
- Support for converting extracted data to BytesM_T, IntegerT, or AddressT types
1001-1008: Consistent type inference approach for Extract32 functionThis method follows the same pattern as Convert and Slice, sorting possible input types by name and bit size before selecting a candidate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
vyper/builtins/functions.py (2)
194-279: Implementation of constant folding forConvertclass - nice!The implementation looks correct, handling various type conversions with appropriate validations. Consider a few improvements:
- Use exception chaining when raising exceptions in catch blocks:
- raise InvalidLiteral("Invalid hex literal for address", node.args[0]) + raise InvalidLiteral("Invalid hex literal for address", node.args[0]) from None
- Same for line 264:
- raise InvalidLiteral("Invalid hex literal for bytes", node.args[0]) + raise InvalidLiteral("Invalid hex literal for bytes", node.args[0]) from None🧰 Tools
🪛 Ruff (0.8.2)
224-224: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
264-264: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
951-995: Well-implemented constant folding forExtract32The implementation handles extraction of 32 bytes from byteslikes properly with:
- Validation of start index
- Appropriate bounds checking
- Conversion to various output types with proper handling
For converting to
IntegerT(line 989), note that there's no explicit check that the result fits within the target integer type bounds. Since this is compile-time validation, an explicit check might save debugging time later.Consider adding a range check when converting bytes to an integer:
elif isinstance(output_type, IntegerT): - return vy_ast.Int.from_node(node, value=int.from_bytes(result, "big")) + value = int.from_bytes(result, "big") + lo, hi = output_type.ast_bounds + if value < lo or value > hi: + raise InvalidLiteral( + f"value {value} out of range for {output_type}", node + ) + return vy_ast.Int.from_node(node, value=value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vyper/builtins/functions.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
vyper/builtins/functions.py (7)
vyper/builtins/_signatures.py (2)
_try_fold(164-165)_validate_arg_types(107-136)vyper/ast/validation.py (1)
validate_call_args(9-97)vyper/ast/nodes.py (9)
get_folded_value(397-405)Name(967-968)Int(787-797)from_node(296-319)Hex(832-874)bytes_value(870-874)Decimal(800-829)Bytes(889-902)Str(877-886)vyper/exceptions.py (3)
UnfoldableNode(429-430)InvalidLiteral(263-264)ArgumentException(287-288)vyper/semantics/types/utils.py (1)
type_from_annotation(72-103)vyper/semantics/types/primitives.py (8)
BoolT(30-50)AddressT(406-433)IntegerT(255-329)BytesM_T(57-132)ast_bounds(144-145)ast_bounds(285-286)ast_bounds(389-390)length(78-79)vyper/semantics/analysis/utils.py (2)
get_possible_types_from_node(136-174)get_possible_types_from_node(452-468)
🪛 Ruff (0.8.2)
vyper/builtins/functions.py
224-224: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
264-264: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (4)
vyper/builtins/functions.py (4)
287-293: Type sorting with consistent keys - good approachUsing consistent sorting for types helps with deterministic behavior in type inference. The implementation makes the compiler behavior more predictable when multiple types are possible.
361-404: Good implementation ofSlice._try_foldwith thorough validationThe constant folding for slice operations is well implemented with appropriate validations:
- Length must be at least 1
- Start index must be non-negative
- Bounds checking to prevent out-of-range errors
One minor observation: There's slight inconsistency in error messages across different types (Bytes/Str/Hex). Consider standardizing them for consistency.
451-456: Uniform type sorting for consistent behaviorThe type sorting in
Slice.infer_arg_typesmatches the approach used inConvert.infer_arg_types, ensuring consistency across different builtins.
1003-1008: Type sorting forExtract32.infer_arg_types- good consistencyThe type sorting in
Extract32.infer_arg_typesfollows the same pattern as the other builtins, maintaining a consistent approach throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
tests/hevm.py (1)
19-22: 🛠️ Refactor suggestionForward the
verboseflag down the call-chain
_prep_hevm_venom()receives averboseargument but drops it when delegating to_prep_hevm_venom_ctx(), so the extra logging you expect in deeper helpers is silently disabled.- ctx = parse_from_basic_block(venom_source_code) - return _prep_hevm_venom_ctx(ctx) + ctx = parse_from_basic_block(venom_source_code) + return _prep_hevm_venom_ctx(ctx, verbose=verbose)Keeping the flag consistent improves debugging and prevents the “why is nothing printed?” head-scratcher.
vyper/venom/passes/__init__.py (1)
1-21: 🛠️ Refactor suggestionSilence Ruff F401 by publishing the imported passes
AssignElimination,PhiEliminationPass, andSingleUseExpansionare intentionally re-exported, so mark them as public to stop static-analysis noise.from .single_use_expansion import SingleUseExpansion +# Re-export public names for `from vyper.venom.passes import *` +__all__ = [ + "AlgebraicOptimizationPass", + "AssignElimination", + "BranchOptimizationPass", + "CSE", + "DFTPass", + "FloatAllocas", + "FunctionInlinerPass", + "LowerDloadPass", + "MakeSSA", + "Mem2Var", + "MemMergePass", + "NormalizationPass", + "PhiEliminationPass", + "RemoveUnusedVariablesPass", + "RevertToAssert", + "SCCP", + "SimplifyCFGPass", + "SingleUseExpansion", + "ReduceLiteralsCodesize", +]Adding
__all__makes the intent explicit and keeps linters quiet without sprinkling# noqacomments everywhere.🧰 Tools
🪛 Ruff (0.8.2)
1-1:
.algebraic_optimization.AlgebraicOptimizationPassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
.assign_elimination.AssignEliminationimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
3-3:
.branch_optimization.BranchOptimizationPassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
4-4:
.common_subexpression_elimination.CSEimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
5-5:
.dft.DFTPassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
6-6:
.float_allocas.FloatAllocasimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
7-7:
.function_inliner.FunctionInlinerPassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
8-8:
.literals_codesize.ReduceLiteralsCodesizeimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
9-9:
.load_elimination.LoadEliminationimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
10-10:
.lower_dload.LowerDloadPassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
11-11:
.make_ssa.MakeSSAimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
12-12:
.mem2var.Mem2Varimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
13-13:
.memmerging.MemMergePassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
14-14:
.normalization.NormalizationPassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
15-15:
.phi_elimination.PhiEliminationPassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
16-16:
.remove_unused_variables.RemoveUnusedVariablesPassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
17-17:
.revert_to_assert.RevertToAssertimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
18-18:
.sccp.SCCPimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
19-19:
.simplify_cfg.SimplifyCFGPassimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
20-20:
.single_use_expansion.SingleUseExpansionimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
🧹 Nitpick comments (16)
vyper/codegen/core.py (3)
74-92: Consider giving_InternalBufferTa descriptive_idand tightening size check
VyperType.__repr__uses the_idclass attribute; leaving itNonecan make debugging prints unhelpful (Noneshows up in error traces). A tiny tweak makes logs clearer, and we can also reject a zero-length buffer early (zero doesn’t really need an allocation):class _InternalBufferT(VyperType): + _id = "_internal_buf" @@ - def __init__(self, buf_size: int): - assert buf_size >= 0 + def __init__(self, buf_size: int): + if buf_size <= 0: + raise CompilerPanic("internal buffer size must be > 0") self.buf_size: int = ceil32(buf_size)Optional, but helps with traceability and avoids silent 0-byte “allocations”.
87-92: Redundant safety:_invalid_locationsalready blocks use; double-defence ok but noisyBecause
_invalid_locationsincludes everyDataLocation, semantic analysis should reject any placement of this type. The customget_size_inthat also raisesCompilerPanicmay therefore never be called. If you prefer single-source-of-truth, consider deleting the override; if you like the belt-and-suspenders approach, leaving it is harmless.
102-103: Doc-string now mismatches behaviour
get_type_for_exact_sizereturns an_InternalBufferTwhosesize_in_bytesis ceil32(n_bytes), not “exactly n_bytes” as the doc-string still claims. Either rename the helper or update the comment to avoid future confusion.tests/functional/builtins/codegen/test_create_functions.py (3)
788-810:value = 0 if v.pop() == b'' else 0is always zero – intentional?The ternary always yields
0. If the goal is merely to force evaluation ofv.pop()to test overlap logic, adding a short comment would clarify intent and avoid “dead-code” readers’ questions.
1122-1151: Comment explains evaluation order quirk – consider an assertion insteadThe explanatory comment is great. You could go one step further and add an explicit
assert self.salt == saltright after the call to make the test’s expectation crystal-clear; if the evaluation order ever changes the test will flag it without needing to read the xfail annotation.
1153-1181: xfail test relies on side-effects for ordering – small style nitUsing side-effects (
self.a = …) is fine here, but asserting the final value ofaonly indirectly checks evaluation order. A direct equality assertion insidetest1/test2(e.g.,assert self.a == 0) could catch accidental double evaluation too. Totally optional.vyper/builtins/functions.py (2)
194-279: Well-implemented constant folding for Convert function.The
_try_foldmethod correctly implements compile-time evaluation for literal conversions, with comprehensive validation for:
- Boolean conversions
- Address conversions with proper validation (length checks, 0x prefix)
- Integer conversions with bounds checking
- Fixed-size bytes conversions with padding and length validation
In the exception handling at lines 224 and 264, consider using
raise ... from Nonefor better error diagnosis:- except ValueError: - raise InvalidLiteral("Invalid hex literal for address", node.args[0]) + except ValueError as err: + raise InvalidLiteral("Invalid hex literal for address", node.args[0]) from None🧰 Tools
🪛 Ruff (0.8.2)
224-224: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
264-264: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
951-995: Thorough constant folding for Extract32 function.The implementation correctly:
- Validates input parameters
- Handles different input types (Bytes, Hex)
- Performs appropriate transformations based on output types
- Includes appropriate bounds checking
Similar to earlier, consider using
raise ... from Nonein exception handling for better error diagnosis.tests/hevm.py (1)
65-69: Consider re-ordering passes or re-using the same analyses cache
SingleUseExpansioninvalidatesDFGAnalysisandLivenessAnalysis, yet immediately after running it no subsequent pass is invoked that relies on those analyses. If you later decide to add more passes in this helper, re-usingacafter invalidation could yield stale results. Either:
- Re-initialize
IRAnalysesCacheafter the expansion, or- Move
SingleUseExpansionearlier (beforeSimplifyCFGPass) if that still satisfies downstream invariants.Not critical today, but worth documenting to avoid subtle analysis bugs later.
tests/unit/compiler/venom/test_single_use_expansion.py (1)
9-17: Rename the test for snake-case consistency & tighten the docstringPEP 8 suggests lower-case function names with underscores. Renaming avoids surprising mismatches in
pytest -kfiltering.-def test_single_use_Expansion(): +def test_single_use_expansion(): @@ - Test to was created from the example in the - issue https://github.com/vyperlang/vyper/issues/4215 - it issue is handled by the SingleUseExpansion pass - - Originally it was handled by different reorder algorithm - which is not necessary with single-use expansion + Reproduces vyperlang/vyper#4215. The bug is now handled by the + SingleUseExpansion pass, replacing the previous re-order algorithm.Besides readability, a clear docstring helps future maintainers understand why the test exists.
vyper/venom/__init__.py (1)
54-97: Guard expensive pass repetitions by optimization level
AssignEliminationandPhiEliminationPassare each invoked three times in the default pipeline. While this guarantees maximal cleanup, the extra passes noticeably increase compilation time for large contracts.Consider:
if optimize != OptimizationLevel.NONE: PhiEliminationPass(ac, fn).run_pass() AssignElimination(ac, fn).run_pass()or introducing a lightweight helper to run them conditionally. This preserves correctness at
-Owhile letting-O0users iterate faster.Also double-check that each run indeed makes progress; if a pass becomes a no-op after the first iteration, removing duplicates is safe and speeds things up.
vyper/venom/passes/phi_elimination.py (3)
1-4: Add a short module‐level docstringA brief docstring explaining the goal of the pass (collapse redundant phi chains, preserve SSA invariants, update DFG/Liveness) will make it far easier for future contributors to discover & understand this optimisation in isolation.
28-33: Avoid mutating thesrcsset with.pop()Calling
pop()both selects and removes the single origin, leaving
phi_to_origins[inst]empty if the mapping is examined later (e.g. by a debug
printer). Usenext(iter(srcs))to fetch an element without mutation.- src = srcs.pop() + src = next(iter(srcs))
35-38: Redundant secondDFGAnalysisrequest
self.dfgis already populated at the beginning ofrun_pass(). Requesting it
again creates a fresh analysis object and wastes work. It also risks
inconsistencies if the analysis cache is modified in between.- self.dfg = self.analyses_cache.request_analysis(DFGAnalysis) - self.phi_to_origins = dict() + # self.dfg has already been requested in run_pass() + self.phi_to_origins: dict[IRInstruction, set[IRInstruction]] = {}tests/unit/compiler/venom/test_phi_elimination.py (2)
9-38:@pytest.mark.hevmbutdefault_hevm=False
_check_pre_postis instantiated withdefault_hevm=False, so the marker alone
will not exercise the HEVM equivalence check unless the test explicitly passes
hevm=True(which only the first test does). Consider either:
- Instantiating the checker with
default_hevm=True, or- Passing
hevm=Truefor every test that is@pytest.mark.hevm.Otherwise the marker is misleading and some tests may silently skip the HEVM
step.
210-233: “cannot-remove” test relies on identity, but does not assert it
test_phi_elim_cannot_removecalls_check_pre_post(pre, pre, hevm=False).
WhilePrePostCheckerwill raise if the IR changes, the intent is not obvious
to readers. Adding a short comment or renaming the test to e.g.
test_phi_elim_leaves_required_phi_intactwould aid clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
tests/functional/builtins/codegen/test_create_functions.py(3 hunks)tests/hevm.py(2 hunks)tests/unit/compiler/venom/test_algebraic_binopt.py(1 hunks)tests/unit/compiler/venom/test_duplicate_operands.py(2 hunks)tests/unit/compiler/venom/test_load_elimination.py(2 hunks)tests/unit/compiler/venom/test_phi_elimination.py(1 hunks)tests/unit/compiler/venom/test_single_use_expansion.py(2 hunks)vyper/builtins/functions.py(5 hunks)vyper/codegen/core.py(3 hunks)vyper/codegen/memory_allocator.py(2 hunks)vyper/semantics/types/base.py(1 hunks)vyper/venom/__init__.py(4 hunks)vyper/venom/passes/__init__.py(2 hunks)vyper/venom/passes/assign_elimination.py(1 hunks)vyper/venom/passes/machinery/inst_updater.py(1 hunks)vyper/venom/passes/phi_elimination.py(1 hunks)vyper/venom/passes/single_use_expansion.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- vyper/codegen/memory_allocator.py
- vyper/venom/passes/single_use_expansion.py
- vyper/venom/passes/assign_elimination.py
🧰 Additional context used
🧬 Code Graph Analysis (8)
tests/unit/compiler/venom/test_duplicate_operands.py (1)
vyper/venom/passes/single_use_expansion.py (2)
SingleUseExpansion(6-49)run_pass(22-27)
tests/unit/compiler/venom/test_algebraic_binopt.py (2)
vyper/venom/passes/assign_elimination.py (1)
AssignElimination(6-41)tests/venom_utils.py (1)
PrePostChecker(54-98)
tests/unit/compiler/venom/test_single_use_expansion.py (1)
vyper/venom/passes/single_use_expansion.py (2)
SingleUseExpansion(6-49)run_pass(22-27)
tests/unit/compiler/venom/test_load_elimination.py (2)
vyper/venom/passes/assign_elimination.py (1)
AssignElimination(6-41)vyper/venom/passes/load_elimination.py (1)
LoadElimination(19-105)
vyper/semantics/types/base.py (1)
vyper/semantics/data_locations.py (1)
DataLocation(6-12)
tests/hevm.py (2)
vyper/venom/passes/lower_dload.py (1)
LowerDloadPass(7-42)vyper/venom/passes/single_use_expansion.py (2)
SingleUseExpansion(6-49)run_pass(22-27)
tests/unit/compiler/venom/test_phi_elimination.py (3)
tests/venom_utils.py (1)
PrePostChecker(54-98)vyper/venom/passes/phi_elimination.py (1)
PhiEliminationPass(6-97)tests/conftest.py (1)
hevm(134-135)
vyper/codegen/core.py (4)
vyper/semantics/types/base.py (2)
VyperType(43-375)get_size_in(174-182)vyper/semantics/data_locations.py (1)
DataLocation(6-12)vyper/utils.py (1)
ceil32(359-360)vyper/exceptions.py (1)
CompilerPanic(409-410)
🪛 Ruff (0.8.2)
vyper/builtins/functions.py
224-224: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
264-264: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
vyper/venom/passes/__init__.py
2-2: .assign_elimination.AssignElimination imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
15-15: .phi_elimination.PhiEliminationPass imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
16-16: .remove_unused_variables.RemoveUnusedVariablesPass imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: .revert_to_assert.RevertToAssert imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
18-18: .sccp.SCCP imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
19-19: .simplify_cfg.SimplifyCFGPass imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
20-20: .single_use_expansion.SingleUseExpansion imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (14)
vyper/semantics/types/base.py (1)
174-182: 👍 Explicit return type improves static analysisAdding
-> inttoget_size_inbrings this method in line with the rest of the type-annotated API and will help mypy / IDEs. No further issues spotted.vyper/builtins/functions.py (5)
281-293: Improved type inference with deterministic sorting.The implementation correctly retrieves the value and target types, with deterministic behavior through sorting by type name and bit size.
361-404: Comprehensive constant folding for Slice function.The implementation correctly handles slicing of literals with:
- Proper validation of start and length parameters
- Bounds checking for slice operations
- Different handling for each supported type (Bytes, Str, Hex)
- Appropriate error messages that include the specific values causing the issue
451-456: Consistent approach to type inference with deterministic sorting.The implementation follows the same pattern as in the Convert function, providing consistency across the codebase.
1003-1008: Consistent type inference pattern applied to Extract32.The type sorting and selection pattern is correctly applied here as well, maintaining consistency across the codebase.
1769-1776:Details
✅ Verification successful
Critical memory safety improvement in RawCreate.
This change adds important memory safety by detecting potential overlaps between
initcodeand other arguments, creating a memory copy when necessary to prevent buffer corruption.The comment explains the rationale clearly, referencing a test case that verifies this behavior.
Run the following script to verify the test case mentioned in the comment:
🏁 Script executed:
#!/bin/bash # Check for the test_raw_create_memory_overlap test to confirm behavior rg "test_raw_create_memory_overlap" tests/functional/builtins/codegen/test_create_functions.py -A 10Length of output: 470
🏁 Script executed:
#!/bin/bash # Display the full body of test_raw_create_memory_overlap to confirm its assertions sed -n '1,200p' tests/functional/builtins/codegen/test_create_functions.py | sed -n '/def test_raw_create_memory_overlap/,/def test_/p'Length of output: 135
🏁 Script executed:
#!/bin/bash # Locate the file containing test_raw_create_memory_overlap and display its context file=$(rg -l "test_raw_create_memory_overlap" tests) echo "Test found in: $file" rg -n -C 10 "test_raw_create_memory_overlap" "$file"Length of output: 1083
Critical memory safety improvement in RawCreate, fully covered by tests
This change safeguards against potential memory overlaps between
initcodeand other arguments inRawCreateby creating a dedicated memory copy when needed. The behavior is explicitly verified bytest_raw_create_memory_overlap(intests/functional/builtins/codegen/test_create_functions.py), ensuring buffer integrity during deployment.tests/unit/compiler/venom/test_duplicate_operands.py (2)
5-5: Import updated to use the new SingleUseExpansion pass.The import has been updated to use
SingleUseExpansioninstead of the previousStoreExpansionPass, which is part of the broader refactoring in Venom IR passes.
29-29: Pass initialization updated to SingleUseExpansion.The code now uses
SingleUseExpansioninstead ofStoreExpansionPass. This aligns with the import change and is part of the Venom IR pass refactoring. The test logic and assertions remain unchanged, indicating that the new pass provides equivalent functionality for this test case.tests/unit/compiler/venom/test_algebraic_binopt.py (2)
4-4: Import updated to use AssignElimination instead of StoreElimination.The import has been updated as part of the broader pass renaming refactoring in the Venom compiler.
12-12: Updated PrePostChecker to use AssignElimination.The
PrePostCheckerinitialization now usesAssignEliminationinstead ofStoreEliminationin all pass positions, while maintaining the same pass order and count. This ensures consistent testing behavior with the renamed pass.tests/unit/compiler/venom/test_load_elimination.py (2)
5-5: Import updated to use AssignElimination instead of StoreElimination.The import has been updated as part of the broader pass renaming refactoring in the Venom compiler.
14-14: Updated PrePostChecker to use AssignElimination in both passes and post lists.The
PrePostCheckerinitialization now usesAssignEliminationinstead ofStoreEliminationin both thepassesandpostlists, while maintaining the same pass order and count. This aligns with the broader refactoring in the Venom IR passes.vyper/venom/passes/machinery/inst_updater.py (1)
38-38:Details
✅ Verification successful
Removed restriction on updating phi instructions.
The assertion that prevented updating instructions with the "phi" opcode has been commented out. This change enables the new
PhiEliminationPassto update or replace phi instructions.However, note that line 106 still prevents adding new phi instructions via the
add_beforemethod. This suggests a deliberate design decision to allow updating existing phi instructions but not creating new ones outside of specialized contexts.
🏁 Script executed:
#!/bin/bash # Check if there are tests for the new PhiEliminationPass to ensure this relaxed constraint is properly tested echo "Checking for PhiEliminationPass tests:" fd -e py phi --exec grep -l "PhiEliminationPass" {} \; echo -e "\nChecking for other instances of 'phi' assertions in the codebase:" rg "assert.*opcode.*phi" --type pyLength of output: 1064
Assertion update and test coverage confirmed
- inst_updater.py now has the original
assert opcode != "phi"commented out (around line 38), allowing existing φ-instructions to be updated or replaced.- There remains a live assertion in the
add_beforemethod (around line 106) preventing creation of new φ-instructions.- A dedicated test suite covers the new pass (
tests/unit/compiler/venom/test_phi_elimination.py).This matches the intended design: φ-nodes may be modified or eliminated by
PhiEliminationPass, but new φ-nodes must only be introduced via specialized machinery. Approving these changes.vyper/venom/passes/phi_elimination.py (1)
89-97:store-chain traversal assumes operand[0] is anIRVariableThe guard
isinstance(inst.operands[0], IRVariable)prevents crashes, but
silently stops the traversal for stores to e.g.MemoryPtrorStoragePtr. If
such pointers can eventually feed a φ, the elimination might give up too early.
Please double-check the IR invariants; otherwise extend the traversal or add an
explicit comment explaining why this is safe.
| for _, inst in self.dfg.outputs.copy().items(): | ||
| if inst.opcode != "phi": | ||
| continue | ||
| self._process_phi(inst) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Using dfg.outputs may miss non-output φ nodes
DFGAnalysis.outputs keeps the last writer for every variable that is currently live at a function exit.
A φ that only feeds internal computations (and whose result is not live at exit) will therefore not appear here and will never be processed, leaving dead φs behind.
Consider iterating over all basic blocks (as you already do in _calculate_phi_origins) or querying dfg.instructions instead.
- for _, inst in self.dfg.outputs.copy().items():
- if inst.opcode != "phi":
- continue
- self._process_phi(inst)
+ for bb in self.function.get_basic_blocks():
+ for inst in bb.instructions:
+ if inst.opcode == "phi":
+ self._process_phi(inst)📝 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.
| for _, inst in self.dfg.outputs.copy().items(): | |
| if inst.opcode != "phi": | |
| continue | |
| self._process_phi(inst) | |
| for bb in self.function.get_basic_blocks(): | |
| for inst in bb.instructions: | |
| if inst.opcode == "phi": | |
| self._process_phi(inst) |
1 participant
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
New Features
Bug Fixes
Refactor