fix[lang]: fix in and not in bytesM folding#5107
Conversation
bytesM literals are case-insensitive (they represent the same value
no matter if the literal is lower- or upper-case), but membership
folding compared the raw source strings. GH 4254 fixed this for `==`
and `\!=` but missed the `In`/`NotIn` branch, so e.g.
`0xA9059CBB in [method_id("transfer(address,uint256)", output_type=bytes4)]`
silently folded to `False`.
| if isinstance(left, vy_ast.Hex): | ||
| # Hex values are str, convert to be case-unsensitive. | ||
| lvalue = lvalue.lower() | ||
| if any(isinstance(i, vy_ast.Hex) for i in right.elements): |
There was a problem hiding this comment.
i don't think this works for recursive lists? like list of list of hex elements. i think it would be architecturally better to have an earlier normalization step (would have to be after address checksum validation though)
There was a problem hiding this comment.
Done in 504a506. Replaced the top-level hex special case with recursive literal comparison keys for lists/tuples. Hex values are lowercased only while deriving comparison keys, so checksum-sensitive AST literals are not rewritten globally. Added nested-list membership/equality coverage. Validation: compare-folding pytest and black check passed.
| ConstantFolder(module_ast).run() | ||
|
|
||
|
|
||
| def _comparison_key(node): |
There was a problem hiding this comment.
kind of baroque and non-performant. i would prefer normalization in Hex ctor and perhaps save the original value on Hex value for later address checksum validation
There was a problem hiding this comment.
Done in 3aec412. Hex.value is now normalized at AST construction and Hex.original_value preserves the source spelling for address checksum validation. The compare fold now uses a simple recursive literal equality helper instead of the previous hex-specific comparison-key path. Validation: test_fold_compare.py, test_constants.py, and black check passed.
| if isinstance(node.op, vy_ast.NotEq): | ||
| value = not value | ||
| return vy_ast.NameConstant.from_node(node, value=value) | ||
| else: |
There was a problem hiding this comment.
how does this work? like does 0x0A compare gt 0x0a?
not sure we need any of the literal_eq machinery at all
There was a problem hiding this comment.
Simplified in d052cee. I removed the _literal_eq / recursive list-tuple comparison machinery and restored scalar-only folding here. Hex literals now normalize at Hex.value, so in / not in and == / != compare normalized values directly; ordered hex comparisons remain unfoldable, with a small guard test for 0x0A > 0x0a. Validation: focused fold-compare tests, constants syntax tests, and black check passed.
| def _is_literal_compare_node(node): | ||
| return isinstance(node, (vy_ast.Constant, vy_ast.List, vy_ast.Tuple)) | ||
|
|
||
|
|
||
| def _literal_eq(left, right): | ||
| if not _is_literal_compare_node(left) or not _is_literal_compare_node(right): | ||
| raise UnfoldableNode("Node contains invalid field(s) for evaluation") | ||
|
|
||
| if type(left) is not type(right): | ||
| return False | ||
|
|
||
| if isinstance(left, vy_ast.Constant): | ||
| return left.value == right.value | ||
|
|
||
| return len(left.elements) == len(right.elements) and all( | ||
| _literal_eq(l, r) for l, r in zip(left.elements, right.elements) | ||
| ) | ||
|
|
There was a problem hiding this comment.
i think this machinery is superfluous and confusing
There was a problem hiding this comment.
Agreed, removed in d052cee. The PR now relies on normalized Hex.value and the existing scalar constant-folding path instead of adding recursive literal comparison support. Same focused fold-compare, constants syntax, and black checks passed.
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
|
hmm, maybe address literals got fucked up now? https://github.com/vyperlang/vyper/actions/runs/27589755492/job/83742065481?pr=5107 and #5107 (comment) |
|
Confirmed. Pushed Validation:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4294f34d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # AST copies and folded-value snapshots can rebuild Hex from existing fields. | ||
| # Preserve the source spelling when it is already available. | ||
| self.original_value = getattr(self, "original_value", self.value) | ||
| self.value = self.value.lower() |
There was a problem hiding this comment.
Lowercasing Hex.value here runs before semantic validation, but BytesM_T.validate_literal still checks node.value to reject mixed uppercase/lowercase hex literals. As a result, inputs such as foo: constant(bytes20) = 0x6b175474e89094c44da98b954eedeac495271d0F are normalized and accepted as bytes20 instead of raising the existing checksum/mixed-case InvalidLiteral, weakening the typo protection covered by tests/unit/ast/nodes/test_hex.py. Please keep that validator on original_value or delay normalization until after validation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b780703. BytesM_T.validate_literal now checks mixed-case spelling from Hex.original_value before constant folding uses the normalized value, restoring the existing invalid-literal coverage including checksum-shaped bytes20 literals.
Validation: uv run --python 3.12 -m pytest tests/unit/ast tests/functional/syntax/test_constants.py tests/functional/builtins/codegen/test_convert.py -q (1715 passed).
| ) | ||
|
|
||
| DICT_AST_SKIPLIST = ("full_source_code", "node_source_code") | ||
| DICT_AST_SKIPLIST = ("full_source_code", "node_source_code", "original_value") |
There was a problem hiding this comment.
Because to_dict() now omits original_value while Hex.value is normalized, ast_dict/annotated_ast_dict output for a checksummed address contains the lowercased literal instead of the source spelling. If that dict is round-tripped through dict_to_ast (the public AST utility), the rebuilt Hex.original_value is the lowercased address and subsequent address validation rejects it as a checksum mismatch, so AST export/import no longer preserves valid contracts containing checksummed address literals. Please either serialize the original spelling as the value for Hex nodes or include enough data to restore it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b780703. Hex.to_dict now emits the original source spelling as value, preserving the public AST format and allowing dict_to_ast to reconstruct checksum-sensitive literals. Added a checksummed-address round-trip regression.
Validation: uv run --python 3.12 -m pytest tests/unit/ast tests/functional/syntax/test_constants.py tests/functional/builtins/codegen/test_convert.py -q (1715 passed), plus focused Ruff, Black, and git diff --check.
2 participants
What I did
Fix #5104:
in/not infolding ofbytesMliterals compared the raw source strings case-sensitively, so e.g.0xA9059CBB in [method_id("transfer(address,uint256)", output_type=bytes4)]silently folded toFalse. #4254 fixed the same bug for==/!=but missed the membership branch.How I did it
Lowercase
Hexvalues on both sides of the membership test inConstantFolder.visit_Compare, mirroring theEq/NotEqpath.How to verify it
folds to
True(previouslyFalse; the runtime-evaluated equivalent has always returnedTrue). Added mixed-casein/not incases totest_fold_compare.pywhich check folded == runtime.Commit message
Description for the changelog
Fix case-sensitivity of constant folding for
in/not inoverbytesMliterals.Cute Animal Picture