[v2] Simplify AST Type implementation to add LiteralType::mobile_type()#1793
Conversation
|
31aa718 to
6f62cd7
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Solidity v2 semantic Type representation to use named per-variant structs (instead of anonymous enum fields), updates the AST Type wrapper to store type details directly (only retaining SemanticContext where needed), and adds a mobile_type() service for literals—enabling solx to type literal constants without mutating the SemanticContext. It also introduces rational-literal mobile type inference to fixed-point number types.
Changes:
- Refactor
semantic::types::Typevariants to wrap dedicated structs (e.g.,Type::Array(ArrayType)), updating all pattern matches accordingly. - Add
LiteralKind::mobile_type()plus fixed-point narrowing for rationals, and surfaceLiteralType::mobile_type()in the AST. - Rework AST
Typewrappers to store inner semantic type details directly, removing the need to retain aTypeIdfor most variants.
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/solidity-v2/outputs/cargo/tests/src/binder_output/report_data.rs | Updates type pretty-printing to match the new semantic Type variant shapes and fixed-point field rename. |
| crates/solidity-v2/outputs/cargo/semantic/src/types/registry.rs | Adapts registry logic (defaults, convertibility, canonicalization, location propagation) to the new Type wrappers. |
| crates/solidity-v2/outputs/cargo/semantic/src/types/parsing.rs | Updates keyword-to-type construction for the new wrapped variants and fixed-point field rename. |
| crates/solidity-v2/outputs/cargo/semantic/src/types/mod.rs | Introduces per-variant structs, adds LiteralKind::mobile_type(), and re-routes Type::mobile_type() through it. |
| crates/solidity-v2/outputs/cargo/semantic/src/types/literals/numbers.rs | Adds rational→fixed-point narrowing (smallest_fixed_point_type_to_fit) plus tests; updates integer narrowing to new IntegerType. |
| crates/solidity-v2/outputs/cargo/semantic/src/passes/tests/typing.rs | Updates typing tests to use the new Type variant constructors and inner structs. |
| crates/solidity-v2/outputs/cargo/semantic/src/passes/p4_resolve_references/visitor.rs | Updates tuple and index typing logic to use wrapped variants. |
| crates/solidity-v2/outputs/cargo/semantic/src/passes/p4_resolve_references/typing.rs | Updates elementary/definition typing and array literal typing to use wrapped variants. |
| crates/solidity-v2/outputs/cargo/semantic/src/passes/p4_resolve_references/resolution.rs | Updates member-resolution dispatch for contract/interface/struct types with wrapped variants. |
| crates/solidity-v2/outputs/cargo/semantic/src/passes/p3_type_definitions/visitor.rs | Updates contract/interface/enum type registration to new wrapped variants. |
| crates/solidity-v2/outputs/cargo/semantic/src/passes/p3_type_definitions/typing.rs | Updates type resolution/registration and getter-type computation to new wrapped variants. |
| crates/solidity-v2/outputs/cargo/semantic/src/passes/p3_type_definitions/resolution.rs | Updates array/tuple/mapping type construction to new wrapped variants. |
| crates/solidity-v2/outputs/cargo/semantic/src/passes/p3_type_definitions/mod.rs | Updates receiver-type registration for state-variable processing to new wrapped variants. |
| crates/solidity-v2/outputs/cargo/semantic/src/context/mod.rs | Updates type naming and storage sizing logic for wrapped variants and fixed-point field rename. |
| crates/solidity-v2/outputs/cargo/semantic/src/built_ins/mod.rs | Updates built-in member lookup dispatch and UDVT lookup to new wrapped variants. |
| crates/solidity-v2/outputs/cargo/semantic/generated/public_api.txt | Records the semantic public API changes (new structs, new LiteralKind::mobile_type(), variant shape changes). |
| crates/solidity-v2/outputs/cargo/ast/src/ast/types.rs | Refactors AST Type wrappers to store inner type details; adds LiteralType::mobile_type(). |
| crates/solidity-v2/outputs/cargo/ast/src/abi/mod.rs | Updates ABI type formatting logic to match wrapped semantic variants. |
| crates/solidity-v2/outputs/cargo/ast/generated/public_api.txt | Records the AST public API changes (removed Type::create/type_id, added LiteralType::mobile_type, fixed-point accessor rename). |
8510423 to
9ef3c38
Compare
8783681 to
efc9459
Compare
teofr
left a comment
There was a problem hiding this comment.
I think the change makes sense, but there's a few concerns:
- It feels like we're ending up with two very similar representation for types, with similar methods. I think keeping that in sync may grow more than desired.
- I'm not sure this fully solves the problem,
solcdefines a mobile type also for recursive types (mobile_type(Tuple(Literal(34), Literal("hola"))) == Tuple(uint8, string memory), so as soon as we expand the definition of mobile type, we'll hit a very similar wall.
A question on an alternative:
- Is there any reason to not just register the mobile type of every type while registering it? To word it in a more theoretically pleasing way, could we make the registry closed under all operations on types?
| let mut factors_of_two: u32 = 0; | ||
| while remaining_denominator.is_multiple_of(&two) { | ||
| remaining_denominator /= &two; | ||
| factors_of_two += 1; | ||
| } |
There was a problem hiding this comment.
nit: not a bottleneck, but you could use trailing_zeros
| let mut factors_of_two: u32 = 0; | |
| while remaining_denominator.is_multiple_of(&two) { | |
| remaining_denominator /= &two; | |
| factors_of_two += 1; | |
| } | |
| let mut factors_of_two: u32 = remaining_denominator.trailing_zeros().unwrap_or(0); |
We already need to do that, in order to encapsulate the internal type representation and improve DX.
Fair point, and that's the reason this is not implemented on
I struggled with the same question for a while. The main issue with that is performance. Computing the mobile type is not cheap for rationals. And we would be doing that not only for the literals but also for all intermediate results when we do constant folding. Arguably, cloning the payload for each type is also not cheap for some variants, but that should be somewhat offset by making the getter functions trivial. Previously we needed to make a There is also one additional point supporting your idea I recently came across. The type data cache (#1816) is keyed on I'm not sure if there's a good alternative solution here. And if not, what's the best trade off. Thoughts? |
| @@ -927,11 +927,9 @@ pub slang_solidity_v2_ast::ast::Type::Tuple(slang_solidity_v2_ast::ast::TupleTyp | |||
| pub slang_solidity_v2_ast::ast::Type::UserDefinedValue(slang_solidity_v2_ast::ast::UserDefinedValueType) | |||
| pub slang_solidity_v2_ast::ast::Type::Void(slang_solidity_v2_ast::ast::VoidType) | |||
| impl slang_solidity_v2_ast::ast::Type | |||
There was a problem hiding this comment.
Following the conversation here to keep track of it:
We already need to do that, in order to encapsulate the internal type representation and improve DX.
Fair enough!
Fair point, and that's the reason this is not implemented on Type, but only on LiteralType.
I guess te concern is, will we add mobile types for other types as part of the public API?
Computing the mobile type is not cheap for rationals. And we would be doing that not only for the literals but also for all intermediate results when we do constant folding.
That's true, haven't thought about it. However, for that case specifically, there's a finite amount of types that could come out of, so you could register certain mobile types eagerly (tuples, functions, etc), and have others be computed on demand (fixed point literals) but ensuring the resulting types will already be registered.
I'm not sure if there's a good alternative solution here. And if not, what's the best trade off. Thoughts?
Sadly, I don't think I have a perfect solution (there probably isn't one).
There was a problem hiding this comment.
Fair point, and that's the reason this is not implemented on Type, but only on LiteralType.
I guess te concern is, will we add mobile types for other types as part of the public API?
Other than tuples, is it necessary for any other case? If there are no other cases, then I think it's fine to leave tuples out.
Computing the mobile type is not cheap for rationals. And we would be doing that not only for the literals but also for all intermediate results when we do constant folding.
That's true, haven't thought about it. However, for that case specifically, there's a finite amount of types that could come out of, so you could register certain mobile types eagerly (tuples, functions, etc), and have others be computed on demand (fixed point literals) but ensuring the resulting types will already be registered.
I had an idea some time ago which may come in clutch for this: we can allocate TypeId range blocks to specific types, so that the TypeId by itself is able to represent most elementary types, ie. no need for registration, with one or more ranges dedicated to user types. So essentially, encoding elementary types in the TypeId value.
This unfortunately doesn't work with tuple types though, because they are arbitrary anyways. We could hack it by adding an AST flag "auto_mobile_convert" so that when inspecting eg. the member elements of a tuple, they would be converted on the fly.
It all seems overcomplicated, so I'd rather stay out of it for now. Having implicit registration of most elementary types would be nice though.
There was a problem hiding this comment.
Other than tuples, is it necessary for any other case? If there are no other cases, then I think it's fine to leave tuples out.
I don't think so, but I'm also not sure if tuples will be needed.
It all seems overcomplicated, so I'd rather stay out of it for now. Having implicit registration of most elementary types would be nice though.
I like the idea, but agree that it's not worth it for now.
teofr
left a comment
There was a problem hiding this comment.
I think the concerns require a deeper discussion, the change itself looks good to me!
efc9459 to
72262bf
Compare
We want to add
LiteralType::mobile_type()as a service forsolxto easily type literal constants. This PR modifiesTypeto store the type details instead of theTypeId. We still need a reference to theSemanticContextfor some types because of nested types and definitions. To do that, we create named structs for semantic types variants, to use them in place of the current anonymous ones.Most importantly, it allows us to compute the mobile type for literal variants and return them as AST types without having to register new types (which would require a mutable
SemanticContext).Also implements mobile type conversion for rationals to fixed-point number types.