refactor[types]: align integer/bytesM _id with bytestring families - #5238
Open
cristianizzo wants to merge 6 commits into
Open
cristianizzo wants to merge 6 commits into
cristianizzo wants to merge 6 commits into
Conversation
`_GenericTypeAcceptor` rendered as `GenericTypeAcceptor(<class '...StringT'>)` in error messages (e.g. `len()` on a wrong type or module), leaking internal Python class paths instead of readable Vyper type names. Add a `__str__` that resolves the user-facing name (`String`, `Bytes`, `DynArray`, ...), falling back to `typeclass` then the class name, and keep `__repr__` as the debug form. Fixes vyperlang#4955
Address review: `typeclass` is the json-serialization id, not a user-facing name, so reusing it (plus a `_TYPECLASS_DISPLAY_NAMES` map) for display was the wrong layer. Give the types whose `_id` is a property (`BytesM_T`, `IntegerT`) or absent (`TYPE_T`) an explicit `_generic_id`, and resolve `_id` -> `_generic_id` in `_GenericTypeAcceptor.__str__`, dropping the `typeclass` map and the `__name__` fallback.
Compare `e.value.message` against the complete expected string instead of substring checks, per review feedback, so the exact user-facing message is visible in the test.
Address review: - remove `TYPE_T._generic_id`: every `TYPE_T.any()` call site rejects a non-type argument with `InvalidType` before any type name is rendered, so the name was dead code. `CompilerPanic` now covers that case. - type `_generic_id` as `Optional[str]` instead of `str = None`. - drop the comments on `__str__` and the tests; fix the description of `_generic_id`, which is about `_id` naming the fully applied type rather than the type constructor, not about parametricity.
Make `IntegerT` and `BytesM_T` consistent with the bytestring types: `_id` is now a class-level family name (`integer`, `bytesM`) and the fully-applied name (`uint256`, `bytes4`) is produced by `__repr__`. A new `VyperType.serialization_name` property carries the applied name where it is load-bearing (AST `to_dict` "name", the primitive-type namespace keys, and synthesized getter annotations), so those keep their existing output. This lets `_GenericTypeAcceptor.__str__` read `self.type_._id` directly and drops the `_generic_id` parallel concept entirely.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are coherent and well-covered by targeted regression tests, with only a minor doc-comment clarification suggested.
Pull request overview
This PR refactors primitive type naming in the semantics layer to make IntegerT and BytesM_T consistent with bytestring families: _id becomes a class-level “family” identifier, while the fully-applied name is rendered via __repr__. To preserve existing source-token behavior (AST name, primitive namespace keys, and synthesized getter annotations), it introduces VyperType.serialization_name and updates the relevant call sites.
Changes:
- Make
IntegerT._id/BytesM_T._idclass attributes ("integer","bytesM") and move applied-name generation toserialization_name+__repr__. - Add
VyperType.serialization_nameand use it for AST serialization (to_dict) and synthesized public getter annotations. - Adjust primitive-type namespace registration to use
serialization_namefor instances while keeping bytestring classes keyed by their declared_id; add regression tests for error message formatting.
File summaries
| File | Description |
|---|---|
| vyper/semantics/types/primitives.py | Realigns _id for IntegerT/BytesM_T to be family-level and adds serialization_name for applied names. |
| vyper/semantics/types/base.py | Adds VyperType.serialization_name, updates __repr__, and simplifies _GenericTypeAcceptor.__str__ to read class-level _id. |
| vyper/semantics/types/init.py | Registers primitive instances under serialization_name while keeping parametrizable bytestring classes keyed by _id. |
| vyper/semantics/analysis/getters.py | Uses type_.serialization_name when synthesizing getter argument annotations to preserve source tokens. |
| tests/functional/syntax/test_len.py | Adds regression tests ensuring type-mismatch messages use readable type names. |
| tests/functional/syntax/test_concat.py | Adds a regression test ensuring concat type-mismatch messages render readable acceptor names (including bytesM). |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Applied parameters are fused into a single source token (`uint256`, `bytes4`) for the parametric primitives and exposed via `serialization_name`; parameterized containers (e.g. `String[5]`) render the applied name in `__repr__`. Distinguish the two in the `_id` comment.
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
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.
Description
Follow-up to #5202, implementing the realign suggested by maintainer Sporarum in review: make
IntegerTandBytesM_Tconsistent with the bytestring types, where_idis a class-level family name and the fully-applied name is produced by__repr__.IntegerT._id/BytesM_T._idare now class attributes (integer,bytesM) instead of per-instance properties returning the applied name (uint256,bytes4).__repr__onVyperTypenow renders the applied name;_GenericTypeAcceptor.__str__readsself.type_._iddirectly (the acceptor is handed the class)._generic_idparallel concept is removed entirely.Because the applied name is also a load-bearing source token (the primitive-type namespace keys,
to_dict()'s AST"name", and synthesized public-variable getter annotations all needuint256/bytes4), a newVyperType.serialization_nameproperty (defaults to_id, overridden by the two parametric primitives) keeps those three uses byte-identical. AST output, error messages, and the namespace are unchanged.Changelog
CHANGELOG entry: null(internal refactor, no user-facing behavior change)Related issues
Notes
This PR is stacked on #5202; once #5202 merges it will be rebased and the diff will shrink to just the realign.
Manual testing steps
test_len.py,test_concat.py) still pass.