Clanup AST representation of TypeDesc#174
Merged
warunalakshitha merged 1 commit intoballerina-platform:mainfrom Feb 24, 2026
Merged
Clanup AST representation of TypeDesc#174warunalakshitha merged 1 commit intoballerina-platform:mainfrom
warunalakshitha merged 1 commit intoballerina-platform:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors the AST and type system by making BLangNodeBase unexported, migrating from TypeData to TypeDescriptor for storing type information, and introducing direct type node accessors (TypeNode/SetTypeNode for variables, ValueType/SetValueType for literals). Public model interfaces are updated to align with descriptor-based typing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Draft
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #174 +/- ##
==========================================
- Coverage 27.80% 27.65% -0.16%
==========================================
Files 253 253
Lines 53810 53801 -9
==========================================
- Hits 14963 14879 -84
- Misses 37911 37994 +83
+ Partials 936 928 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ast/types.go (1)
61-76: Extract shared type fields into a private base.
bLangTypeBaseandBTypeBasicboth carryty/tag/name/flagsand duplicate their getters/setters. A small private base struct would remove duplication and keep the shared-field rule intact.♻️ Suggested structure
+type bTypeBase struct { + ty model.TypeData + tag model.TypeTags + name model.Name + flags uint64 +} + +func (b *bTypeBase) GetTypeData() model.TypeData { return b.ty } +func (b *bTypeBase) SetTypeData(ty model.TypeData) { b.ty = ty } +func (b *bTypeBase) BTypeGetTag() model.TypeTags { return b.tag } +func (b *bTypeBase) bTypeSetTag(tag model.TypeTags) { b.tag = tag } +func (b *bTypeBase) bTypeGetName() model.Name { return b.name } +func (b *bTypeBase) bTypeSetName(name model.Name) { b.name = name } +func (b *bTypeBase) bTypeGetFlags() uint64 { return b.flags } +func (b *bTypeBase) bTypeSetFlags(flags uint64) { b.flags = flags } + type bLangTypeBase struct { bLangNodeBase - ty model.TypeData + bTypeBase FlagSet common.UnorderedSet[model.Flag] Grouped bool tags model.TypeTags name model.Name flags uint64 } type BTypeBasic struct { - ty model.TypeData - tag model.TypeTags - name model.Name - flags uint64 + bTypeBase }As per coding guidelines: "If multiple structs need to hold same set of fields and implement methods on those fields, add a *Base struct and use type inclusion on other structs. Make this base struct private and implement the relevant methods on the base struct."
Also applies to: 305-351, 373-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast/types.go` around lines 61 - 76, Create a private base struct to hold the repeated fields (ty, tag, name, flags) and implement the shared getters/setters on that base, then embed it into bLangTypeBase and BTypeBasic to remove duplicated fields and methods; specifically, add a private type (e.g., type typeBase struct { ty model.TypeData; tag model.TypeTags; name model.Name; flags uint64 }) with methods for the existing getters/setters, replace the explicit ty/tag/name/flags fields in bLangTypeBase and BTypeBasic with an embedded typeBase, and update all methods currently implemented on bLangTypeBase and BTypeBasic (including the other occurrences around the referenced ranges) to call the base methods so behavior and visibility remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ast/ast.go`:
- Around line 408-412: BLangVariableBase.typeNode is declared as BType but may
be assigned a BTypeBasic (from NewBType) which does not implement BLangNode and
will cause panics where code casts typeNode to BLangNode (e.g., in
pretty_printer.go and walk.go). Fix by narrowing the field to the proper node
interface (e.g., change BLangVariableBase.typeNode from BType to BLangNode or a
new BLangType interface that excludes BTypeBasic) OR keep the BType but add safe
type assertions where typeNode is cast (guard the casts with an "ok" check and
handle the non-BLangNode case). Update references to BLangVariableBase.typeNode
and the casts in pretty_printer.go and walk.go accordingly.
---
Nitpick comments:
In `@ast/types.go`:
- Around line 61-76: Create a private base struct to hold the repeated fields
(ty, tag, name, flags) and implement the shared getters/setters on that base,
then embed it into bLangTypeBase and BTypeBasic to remove duplicated fields and
methods; specifically, add a private type (e.g., type typeBase struct { ty
model.TypeData; tag model.TypeTags; name model.Name; flags uint64 }) with
methods for the existing getters/setters, replace the explicit ty/tag/name/flags
fields in bLangTypeBase and BTypeBasic with an embedded typeBase, and update all
methods currently implemented on bLangTypeBase and BTypeBasic (including the
other occurrences around the referenced ranges) to call the base methods so
behavior and visibility remain unchanged.
We should have type descriptors only for nodes that needs them
heshanpadmasiri
force-pushed
the
refact/ast
branch
from
eeadf12 to
070f277
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ast/node_builder.go (1)
34-69:⚠️ Potential issue | 🟠 MajorHandle BYTE (and BYTE_ARRAY) in the type table to avoid panics.
createSimpleLiteralInnercan settypeTagtoBYTEorBYTE_ARRAY, butgetTypeFromTagonly handles boolean/int/nil/string/float/decimal, so valid literals will panic.🔧 Proposed fix (add missing tags to typeTable)
type typeTable struct { booleanType *BTypeBasic intType *BTypeBasic + byteType *BTypeBasic nilType *BTypeBasic stringType *BTypeBasic floatType *BTypeBasic decimalType *BTypeBasic + byteArrayType *BTypeBasic } func newTypeTable() typeTable { return typeTable{ booleanType: &BTypeBasic{tag: model.TypeTags_BOOLEAN, flags: Flags_READONLY}, intType: &BTypeBasic{tag: model.TypeTags_INT, flags: Flags_READONLY}, + byteType: &BTypeBasic{tag: model.TypeTags_BYTE, flags: Flags_READONLY}, nilType: &BTypeBasic{tag: model.TypeTags_NIL, flags: Flags_READONLY}, stringType: &BTypeBasic{tag: model.TypeTags_STRING, flags: Flags_READONLY}, floatType: &BTypeBasic{tag: model.TypeTags_FLOAT, flags: Flags_READONLY}, decimalType: &BTypeBasic{tag: model.TypeTags_DECIMAL, flags: Flags_READONLY}, + byteArrayType: &BTypeBasic{tag: model.TypeTags_BYTE_ARRAY, flags: Flags_READONLY}, } } func (t *typeTable) getTypeFromTag(tag model.TypeTags) model.TypeDescriptor { switch tag { case model.TypeTags_BOOLEAN: return t.booleanType case model.TypeTags_INT: return t.intType + case model.TypeTags_BYTE: + return t.byteType case model.TypeTags_NIL: return t.nilType case model.TypeTags_STRING: return t.stringType case model.TypeTags_FLOAT: return t.floatType case model.TypeTags_DECIMAL: return t.decimalType + case model.TypeTags_BYTE_ARRAY: + return t.byteArrayType default: panic("not implemented") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast/node_builder.go` around lines 34 - 69, The typeTable currently lacks entries for BYTE and BYTE_ARRAY which makes createSimpleLiteralInner panics when it sets typeTag to TypeTags_BYTE or TypeTags_BYTE_ARRAY; add two fields (e.g., byteType *BTypeBasic and byteArrayType *BTypeBasic) to the typeTable struct, initialize them in newTypeTable with tag: model.TypeTags_BYTE and model.TypeTags_BYTE_ARRAY (flags: Flags_READONLY), and extend getTypeFromTag to return t.byteType for model.TypeTags_BYTE and t.byteArrayType for model.TypeTags_BYTE_ARRAY instead of falling through to panic.ast/expressions.go (1)
185-412:⚠️ Potential issue | 🟠 MajorAdd SetValueType calls for literal constructions in desugar/statement.go.
Found uninitialized
valueTypein literal constructions at desugar/statement.go:260 (oneLiteral) and desugar/statement.go:340 (zeroLiteral). Both create BLangNumericLiteral instances but only callSetDeterminedType(), leavingvalueTypeas nil. This creates gaps whereGetValueType()will return nil, potentially causing type information loss.Literals in ast/node_builder.go are properly covered via a switch statement (lines 1131–1133) that calls SetValueType on all literal types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast/expressions.go` around lines 185 - 412, The BLangNumericLiteral instances created in oneLiteral and zeroLiteral are missing a SetValueType call, leaving valueType nil; after you call SetDeterminedType (or determine the BType used there) on the BLangNumericLiteral, also invoke SetValueType with the same BType (e.g., for the BLangNumericLiteral variable created in oneLiteral and zeroLiteral call lit.SetValueType(determinedType) or lit.SetValueType(bt) immediately after SetDeterminedType) so GetValueType() will return the correct BType.
🧹 Nitpick comments (2)
semantics/type_resolver.go (2)
1040-1041: Same implicit type cast as inresolveSimpleVariable.Apply the same explicit cast pattern here for consistency:
Suggested change
if typeNode := constant.TypeNode(); typeNode != nil { - expectedType = t.resolveBType(typeNode, 0) + expectedType = t.resolveBType(typeNode.(ast.BType), 0) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semantics/type_resolver.go` around lines 1040 - 1041, The code in resolve of constant.TypeNode() uses an implicit assignment to expectedType via t.resolveBType(typeNode, 0); mirror the explicit cast pattern used in resolveSimpleVariable by explicitly casting the returned value to the expected BType type (or the concrete type used elsewhere) before assigning to expectedType; update the line using resolveBType in the same function (reference constant.TypeNode(), resolveBType) to perform the explicit cast so the types are consistent with resolveSimpleVariable.
208-213: Fallback toNEVERmay mask missing handler cases.Setting
DeterminedTypetoNEVERas a blanket fallback ensures all visited nodes have a type, but it could silently hide cases where a specific node type should have been handled in the switch but wasn't. Consider whether logging or tracking unhandled node types during development would help catch missing cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semantics/type_resolver.go` around lines 208 - 213, The blanket fallback that sets node.SetDeterminedType(&semtypes.NEVER) when node.GetDeterminedType() == nil can mask missing switch/case handling; change this to record or surface unhandled node kinds before assigning NEVER: detect the node kind (e.g., via node.GetKind()/node.Type() or similar), emit a development-level warning or append the kind to an unhandledNodes tracker (or trigger a panic/assert in dev builds), and then set the fallback to semtypes.NEVER; ensure references to node.GetDeterminedType, node.SetDeterminedType and semtypes.NEVER are used so you only alter the fallback path and add logging/tracking without changing existing typing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ast/expressions.go`:
- Around line 185-412: The BLangNumericLiteral instances created in oneLiteral
and zeroLiteral are missing a SetValueType call, leaving valueType nil; after
you call SetDeterminedType (or determine the BType used there) on the
BLangNumericLiteral, also invoke SetValueType with the same BType (e.g., for the
BLangNumericLiteral variable created in oneLiteral and zeroLiteral call
lit.SetValueType(determinedType) or lit.SetValueType(bt) immediately after
SetDeterminedType) so GetValueType() will return the correct BType.
In `@ast/node_builder.go`:
- Around line 34-69: The typeTable currently lacks entries for BYTE and
BYTE_ARRAY which makes createSimpleLiteralInner panics when it sets typeTag to
TypeTags_BYTE or TypeTags_BYTE_ARRAY; add two fields (e.g., byteType *BTypeBasic
and byteArrayType *BTypeBasic) to the typeTable struct, initialize them in
newTypeTable with tag: model.TypeTags_BYTE and model.TypeTags_BYTE_ARRAY (flags:
Flags_READONLY), and extend getTypeFromTag to return t.byteType for
model.TypeTags_BYTE and t.byteArrayType for model.TypeTags_BYTE_ARRAY instead of
falling through to panic.
---
Duplicate comments:
In `@ast/ast.go`:
- Around line 408-413: BLangVariableBase currently stores typeNode as BType
which leads to unsafe casts to BLangNode later; change the field to a concrete
AST node type (e.g., rename and type it as BLangNode or a dedicated interface
like BTypeNode that embeds both BType and BLangNode) so callers don't need to
cast. Update all places that set or read BLangVariableBase.typeNode (and any
constructors/factory functions) to assign and consume the new BLangNode-typed
field, and remove unsafe type assertions that cast BType->BLangNode; ensure
model.AnnotationAttachmentNode usage (AnnAttachments) is unchanged.
In `@ast/node_builder.go`:
- Around line 1473-1499: The code unsafely asserts
createTypeNode(typeDesc).(BType) which can panic if createTypeNode returns nil
or a different type; update the block around
variable.(*BLangSimpleVariable).SetTypeNode to call n.createTypeNode(typeDesc),
check the result is non-nil and of type BType (or handle non-BType
appropriately), and only then call SetTypeNode; reference the createTypeNode
function, the typeDesc variable, the isDeclaredWithVar check, and the
variable.(*BLangSimpleVariable).SetTypeNode call so you locate and replace the
unchecked assertion with a safe type check and appropriate error handling or
fallback.
- Around line 1841-1861: The code is unsafely asserting
n.createTypeNode(typeDescriptor).(BType) which can panic if createTypeNode
returns nil or a non-BType; replace the direct assertion in the constant
handling block by capturing the returned value, checking for nil and a
successful type assertion (e.g. v, ok := n.createTypeNode(typeDescriptor); if ok
{ constantNode.SetTypeNode(v.(BType)) } ), and handle the non-BType case
gracefully (skip setting the type or log/error as appropriate) so SetTypeNode is
only called with a valid BType; reference symbols: createTypeNode,
TypeDescriptor, constantDeclarationNode, constantNode.SetTypeNode, BType.
- Around line 1058-1134: The code can panic when encountering BYTE or BYTE_ARRAY
literals because typeTag isn't set to model.TypeTags_BYTE or
model.TypeTags_BYTE_ARRAY (or getTypeFromTag can't handle them); update the
literal handling in the function that builds literals so it explicitly sets
typeTag = model.TypeTags_BYTE for byte literals and typeTag =
model.TypeTags_BYTE_ARRAY for byte-array/binary literals (the branch handling
common.BINARY_EXPRESSION and common.BYTE_ARRAY_LITERAL) and ensure the retrieved
BType from n.types.getTypeFromTag(typeTag) is valid (add a nil-check or
fallback) before calling bType.bTypeSetTag and bl.SetValueType; reference
getTypeFromTag, the BType variable bType, the bLiteral branches (BLangLiteral /
BLangNumericLiteral), and isNumericLiteral when making these changes.
In `@ast/pretty_printer.go`:
- Around line 406-415: printSimpleVariable currently calls node.TypeNode() and
directly casts it to BLangNode which can panic if TypeNode() is nil or not a
BLangNode; update printSimpleVariable to first retrieve tn := node.TypeNode(),
check tn != nil, then use a safe type assertion (bn, ok := tn.(BLangNode))
before calling p.PrintInner(bn) and only call p.PrintInner when ok is true,
ensuring you also preserve the existing indentation and closing printSticky(")")
behavior; reference: function printSimpleVariable, method TypeNode(), type
BLangNode, and method PrintInner.
- Around line 580-589: The code in printConstant uses an unchecked cast
node.TypeNode().(BLangNode) which can panic; change it to perform a safe type
assertion (e.g. tnode, ok := node.TypeNode().(BLangNode)) and only call
p.PrintInner(tnode) when ok, otherwise handle the non-BLangNode case via a safe
fallback (e.g. call a generic printer method or print a placeholder) so the
function never panics; also ensure p.indentLevel is incremented/decremented in
all branches to keep indentation balanced.
---
Nitpick comments:
In `@semantics/type_resolver.go`:
- Around line 1040-1041: The code in resolve of constant.TypeNode() uses an
implicit assignment to expectedType via t.resolveBType(typeNode, 0); mirror the
explicit cast pattern used in resolveSimpleVariable by explicitly casting the
returned value to the expected BType type (or the concrete type used elsewhere)
before assigning to expectedType; update the line using resolveBType in the same
function (reference constant.TypeNode(), resolveBType) to perform the explicit
cast so the types are consistent with resolveSimpleVariable.
- Around line 208-213: The blanket fallback that sets
node.SetDeterminedType(&semtypes.NEVER) when node.GetDeterminedType() == nil can
mask missing switch/case handling; change this to record or surface unhandled
node kinds before assigning NEVER: detect the node kind (e.g., via
node.GetKind()/node.Type() or similar), emit a development-level warning or
append the kind to an unhandledNodes tracker (or trigger a panic/assert in dev
builds), and then set the fallback to semtypes.NEVER; ensure references to
node.GetDeterminedType, node.SetDeterminedType and semtypes.NEVER are used so
you only alter the fallback path and add logging/tracking without changing
existing typing logic.
warunalakshitha
merged commit 248d5c6
into
ballerina-platform:main
5 of 6 checks passed
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
2 participants
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.
Purpose
Remove type descriptors from nodes that don't have type descriptors
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor