Add front end support for map values#144
Merged
warunalakshitha merged 1 commit intoballerina-platform:mainfrom Feb 23, 2026
Merged
Add front end support for map values#144warunalakshitha merged 1 commit intoballerina-platform:mainfrom
warunalakshitha merged 1 commit intoballerina-platform:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds comprehensive front-end support for map literals (record literals) in Ballerina, introducing new AST nodes, semantic analysis paths, type resolution mechanisms, and BIR code generation for mapping constructors across the compiler stack. Changes
Sequence DiagramsequenceDiagram
participant Parser
participant ASTBuilder as AST Builder
participant SymbolResolver
participant TypeResolver
participant SemanticAnalyzer
participant BIRGen as BIR Generator
Parser->>ASTBuilder: Parse mapping constructor
ASTBuilder->>ASTBuilder: TransformMappingConstructorExpression()
ASTBuilder->>ASTBuilder: Create BLangRecordLiteral with fields
ASTBuilder->>SymbolResolver: Visit record literal
SymbolResolver->>SymbolResolver: resolveRecordLiteral()
SymbolResolver->>SymbolResolver: Create block scope for record
SymbolResolver->>SymbolResolver: Bind non-computed keys as symbols
SymbolResolver->>TypeResolver: Record literal ready
TypeResolver->>TypeResolver: resolveRecordLiteral()
TypeResolver->>TypeResolver: Resolve each field value type
TypeResolver->>TypeResolver: Build mapping definition
TypeResolver->>TypeResolver: Set atomic type on record
TypeResolver->>SemanticAnalyzer: Record with derived type
SemanticAnalyzer->>SemanticAnalyzer: analyzeRecordLiteralExpr()
SemanticAnalyzer->>SemanticAnalyzer: selectMappingInherentType()
SemanticAnalyzer->>SemanticAnalyzer: Assign mapping type to expression
SemanticAnalyzer->>BIRGen: Typed record literal
BIRGen->>BIRGen: mappingConstructorExpression()
BIRGen->>BIRGen: Evaluate key and value expressions
BIRGen->>BIRGen: Create NewMap instruction
BIRGen->>BIRGen: Emit to basic block
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
heshanpadmasiri
force-pushed
the
feat/map-frontend
branch
from
f7201ff to
7b20327
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
+ Coverage 27.99% 29.09% +1.09%
==========================================
Files 254 254
Lines 53865 54231 +366
==========================================
+ Hits 15082 15778 +696
+ Misses 37855 37479 -376
- Partials 928 974 +46 ☔ 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
context/context.go (1)
134-139:⚠️ Potential issue | 🟠 MajorMake
ANON_SYMBOLunexported since it's not used outside the package.Lines 134-139: This constant is exported but has no external usages. Per coding guidelines, don't make symbols public unless needed.
Suggested change
- ANON_SYMBOL = ANON_PREFIX + "Symbol$" + anonSymbol = ANON_PREFIX + "Symbol$"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@context/context.go` around lines 134 - 139, ANON_SYMBOL is exported but unused outside the package; rename it to an unexported identifier (e.g., anonSymbol) in the constants block so it is no longer public. Update the constant declaration (ANON_SYMBOL -> anonSymbol) inside context.go and run tests/grep to ensure there are no internal references requiring change.
🧹 Nitpick comments (3)
bir/non_terminator.go (1)
94-99: Consider adding a constructor function forMappingConstructorKeyValueEntry.The struct has unexported fields, which is good per coding guidelines. However, external code cannot construct instances without a factory function. As per coding guidelines, constructor methods should provide data for all fields.
♻️ Proposed constructor
+func NewMappingConstructorKeyValueEntry(keyOp, valueOp *BIROperand) *MappingConstructorKeyValueEntry { + return &MappingConstructorKeyValueEntry{ + keyOp: keyOp, + valueOp: valueOp, + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bir/non_terminator.go` around lines 94 - 99, Add a constructor function NewMappingConstructorKeyValueEntry(keyOp *BIROperand, valueOp *BIROperand) *MappingConstructorKeyValueEntry that returns a pointer to a MappingConstructorKeyValueEntry with both unexported fields keyOp and valueOp populated; locate the MappingConstructorKeyValueEntry type and implement this factory so external packages can create instances while keeping fields unexported.semantics/semantic_analyzer.go (1)
712-722: Redundant pre-analysis duplicates work from caller.Lines 712-715 analyze field values again, but
analyzeRecordLiteralExpralready calls this function after analyzing fields (line 674). This creates redundant work.♻️ Remove redundant analysis
- for _, f := range expr.Fields { - kv := f.(*ast.BLangRecordKeyValueField) - analyzeExpression(a, kv.ValueExpr, nil) - } - fieldNames := make([]string, len(expr.Fields))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semantics/semantic_analyzer.go` around lines 712 - 722, The first loop that iterates expr.Fields and calls analyzeExpression on each kv.ValueExpr is redundant because analyzeRecordLiteralExpr already performs that analysis (see analyzeRecordLiteralExpr calling analyzeExpression for field values); remove the duplicate loop that casts to *ast.BLangRecordKeyValueField and calls analyzeExpression, leaving only the subsequent fieldNames extraction and sort logic which should remain unchanged.ast/types.go (1)
456-464: Consider returning a sentinel value instead of panicking in getter.
GetTypeKind()panics if the underlyingTypeDescriptoris nil or doesn't implementBuiltInReferenceTypeNode. While this may indicate a programming error, panics in getters can be difficult to debug. Consider returningmodel.TypeKind_OTHERor a designated unknown kind.♻️ Proposed alternative
func (this *BLangConstrainedType) GetTypeKind() model.TypeKind { if this.Type.TypeDescriptor == nil { - panic("base type is nil") + return model.TypeKind_OTHER } if builtIn, ok := this.Type.TypeDescriptor.(model.BuiltInReferenceTypeNode); ok { return builtIn.GetTypeKind() } - panic("BLangConstrainedType.Type does not implement BuiltInReferenceTypeNode") + return model.TypeKind_OTHER }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast/types.go` around lines 456 - 464, BLangConstrainedType.GetTypeKind currently panics when this.Type.TypeDescriptor is nil or doesn't implement model.BuiltInReferenceTypeNode; change it to return a sentinel (e.g., model.TypeKind_OTHER) instead: check for nil this.Type.TypeDescriptor and after the type assertion for model.BuiltInReferenceTypeNode (the builtIn variable), and if either check fails return model.TypeKind_OTHER rather than calling panic; keep the successful path returning builtIn.GetTypeKind().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bir/non_terminator.go`:
- Around line 77-83: NewMap currently embeds BIRInstructionBase and stores its
target in LhsOp but does not declare that it implements BIRAssignInstruction;
add an implementation of GetLhsOperand() on the NewMap type that returns the
embedded LhsOp (matching other assign-instructions like Move/BinaryOp/UnaryOp),
and likewise add the same GetLhsOperand() implementation to NewArray so both
types satisfy BIRAssignInstruction; reference the NewMap and NewArray structs
and ensure the method signature matches the BIRAssignInstruction interface's
GetLhsOperand.
In `@corpus/bal/subset4/04-map/simple-v.bal`:
- Line 23: The output annotation on the io:println(foo["b"]) line is misspelled
as `@outpt` so the test harness will ignore it; update the annotation to the
correct tag (`@output`) and keep the expected string "b" (i.e., change `@outpt` "b"
to `@output` "b") so the corpus expectation for the io:println(foo["b"]) call is
validated.
In `@corpus/bal/subset4/04-map/union2-e.bal`:
- Around line 18-21: The test file is missing an error annotation for the
compile-time failure: add a "// `@error`" comment to the map literal assignment to
foo (the map literal {"a": 1, "b": "b"} assigned to map<int>|map<string> foo in
function main) so the error test marks the expected diagnostic for that
statement; ensure the annotation sits on the same line as the assignment to foo
so the test harness recognizes the compile error.
In `@corpus/bal/subset4/04-map/union3-v.bal`:
- Around line 18-22: This test prints the value of variable a (from foo) but
lacks the required `@output` annotation for *-v.bal tests; add an output comment
declaring the expected output ("1") for this test (place the `@output` comment
above main or at the top of the file) so that the run of main() which uses foo
and io:println(a) is captured as the test output.
In `@corpus/integration_test.go`:
- Around line 61-79: The skip list currently passed into makeSkipTestsMap
(assigned to skipTestsMap) broadly skips the entire subset4/04-map suite; narrow
it to either re-enable these tests or limit skips to only the known failing
files by removing the bulk of "subset4/04-map/*" entries and keeping just the
specific failing filenames (or replace with a targeted list), and add a comment
referencing a tracking issue ID and expected re-enable date/plan; update the
slice passed to makeSkipTestsMap and the surrounding comment so skipTestsMap
only omits documented failures rather than the whole suite.
In `@desugar/expression.go`:
- Around line 345-367: In walkRecordLiteral, computed record keys (kv.Key.Expr
when kv.Key.ComputedKey is true) are not being passed through walkExpression, so
any initStmts from the key are lost; call walkExpression on kv.Key.Expr before
processing kv.ValueExpr, append the key's result.initStmts into initStmts
(ensuring key initStmts come before value initStmts to preserve evaluation
order), then replace kv.Key.Expr with the key result.replacementNode and
continue to desugar kv.ValueExpr as currently done for the value.
In `@semantics/semantic_analyzer.go`:
- Around line 667-696: The analyzeRecordLiteralExpr function currently analyzes
each field twice and uses unchecked assertions for key extraction; remove the
first unconditional loop (the one that calls analyzeExpression for every
kv.ValueExpr before handling expectedType) so fields are analyzed only once, and
centralize key-name extraction into a single safe pattern (use a type switch
like in the later block or extract to a tiny helper) when computing requiredType
via mappingAtomicType.FieldInnerVal(keyName); also update
selectMappingInherentType to use the same safe key extraction helper/pattern
instead of raw assertions (avoid kv.Key.Expr.(*ast.BLangLiteral) and any
unchecked casts) while preserving the existing behavior of setting
expr.AtomicType and setExpectedType(resultType) when expectedType != nil.
In `@semantics/type_resolver.go`:
- Around line 459-487: The resolveRecordLiteral function currently does an
unchecked assertion f.(*ast.BLangRecordKeyValueField) which can panic for spread
or other non-key fields and preallocates fields with len(e.Fields); change the
loop to safely type-assert (use a type switch or the "ok" idiom) and skip
non-key-value fields instead of panicking, and switch from preallocating fields
to building the fields slice with append so skipped entries don't leave holes;
inside the handling for kv.Key.Expr (the switch over *ast.BLangLiteral and
ast.BNodeWithSymbol), add a default branch that either continues (skip
unrecognized key expressions) or sets a deterministic fallback keyName before
calling semtypes.FieldFrom so keyName is never left empty, and retain calls to
t.resolveExpression, t.ctx.SetSymbolType, t.ctx.SymbolName, semtypes.FieldFrom,
semtypes.NewMappingDefinition, and semtypes.ToMappingAtomicType.
In `@semtypes/core.go`:
- Around line 1042-1052: ContainsUndef currently assumes
getComplexSubtypeData(t, BT_UNDEF) returns *bool and panics when it returns an
AllOrNothingSubtype (from CreateAll/CreateNothing); update ContainsUndef to
inspect the returned value's concrete type (e.g., switch/type-assert to *bool
and AllOrNothingSubtype) instead of blindly asserting *bool, return true/false
for the All/Nothing cases per the established pattern, and keep the existing
BasicTypeBitSet branch unchanged; reference ContainsUndef,
getComplexSubtypeData, BT_UNDEF, ComplexSemType, and
AllOrNothingSubtype/CreateAll/CreateNothing when making the change.
In `@semtypes/field_pair.go`:
- Around line 158-167: NewFieldPairs currently constructs a mappingPairIterator
without initializing rest1/rest2, which internalNext() expects when one side
runs out of names; set rest1 to the tail of m1.Types after m1.Names
(m1.Types[len(m1.Names):]) and rest2 to the tail of m2.Types after m2.Names
(m2.Types[len(m2.Names):]) when building the mappingPairIterator so
internalNext() has the correct remaining-type slices to produce correct
FieldPair pairings and type calculations.
In `@semtypes/mapping_alternative.go`:
- Around line 72-102: Replace the pointer equality check in
MappingAlternativeAllowsFields (CellInnerVal(pos.Rest) == &UNDEF) with a proper
type-equivalence check using the package helper (e.g.
semtypes.IsSameType(CellInnerVal(pos.Rest), &UNDEF) or the project’s equivalent
IsSameType call) so you compare types not pointer identity; also rename the
local variable currently called len (in the loop: len := len(fieldNames)) to
avoid shadowing the builtin (use n or total) and update its uses; leave the rest
of the logic (pos.Names iteration and the alt.neg panic) unchanged.
---
Outside diff comments:
In `@context/context.go`:
- Around line 134-139: ANON_SYMBOL is exported but unused outside the package;
rename it to an unexported identifier (e.g., anonSymbol) in the constants block
so it is no longer public. Update the constant declaration (ANON_SYMBOL ->
anonSymbol) inside context.go and run tests/grep to ensure there are no internal
references requiring change.
---
Nitpick comments:
In `@ast/types.go`:
- Around line 456-464: BLangConstrainedType.GetTypeKind currently panics when
this.Type.TypeDescriptor is nil or doesn't implement
model.BuiltInReferenceTypeNode; change it to return a sentinel (e.g.,
model.TypeKind_OTHER) instead: check for nil this.Type.TypeDescriptor and after
the type assertion for model.BuiltInReferenceTypeNode (the builtIn variable),
and if either check fails return model.TypeKind_OTHER rather than calling panic;
keep the successful path returning builtIn.GetTypeKind().
In `@bir/non_terminator.go`:
- Around line 94-99: Add a constructor function
NewMappingConstructorKeyValueEntry(keyOp *BIROperand, valueOp *BIROperand)
*MappingConstructorKeyValueEntry that returns a pointer to a
MappingConstructorKeyValueEntry with both unexported fields keyOp and valueOp
populated; locate the MappingConstructorKeyValueEntry type and implement this
factory so external packages can create instances while keeping fields
unexported.
In `@semantics/semantic_analyzer.go`:
- Around line 712-722: The first loop that iterates expr.Fields and calls
analyzeExpression on each kv.ValueExpr is redundant because
analyzeRecordLiteralExpr already performs that analysis (see
analyzeRecordLiteralExpr calling analyzeExpression for field values); remove the
duplicate loop that casts to *ast.BLangRecordKeyValueField and calls
analyzeExpression, leaving only the subsequent fieldNames extraction and sort
logic which should remain unchanged.
heshanpadmasiri
commented
Feb 20, 2026
heshanpadmasiri
force-pushed
the
feat/map-frontend
branch
from
363e2d5 to
30812f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
context/context.go (1)
134-139:⚠️ Potential issue | 🟡 MinorMake ANON_SYMBOL unexported.
The constant is not used anywhere in the codebase and should be unexported per coding guidelines: "Don't make symbols public unless asked for or needed." Rename to
anon_symbolto keep the constant internal to the package.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@context/context.go` around lines 134 - 139, The constant ANON_SYMBOL should be made unexported per guidelines; rename the constant ANON_SYMBOL to anon_symbol in the const block (alongside ANON_PREFIX, BUILTIN_ANON_TYPE, ANON_TYPE) and update any references to use anon_symbol; after renaming, run a build or grep to ensure no external packages rely on the exported name and adjust tests/imports if needed.bir/non_terminator.go (1)
33-99:⚠️ Potential issue | 🟠 MajorMake
MappingConstructorEntry,MappingConstructorKeyValueEntry, andNewMapunexportedThese types are used exclusively within the
birpackage and should not be exported. Per coding guidelines: "Don't make symbols public unless asked for or needed." Change them to lowercase to unexport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bir/non_terminator.go` around lines 33 - 99, Rename the exported types and their methods to unexported identifiers: change MappingConstructorEntry to mappingConstructorEntry and its methods IsKeyValuePair() and ValueOp() to isKeyValuePair() and valueOp(); change MappingConstructorKeyValueEntry to mappingConstructorKeyValueEntry (its fields keyOp and valueOp are already unexported); and change NewMap to newMap (keep its fields Type and Values unexported as needed—rename Value slice type references to mappingConstructorEntry). Update all usages/implementations in this file (and any other bir package files) so the implementations of the interface use the new lowercase method names and the type names match (e.g., mappingConstructorKeyValueEntry should implement mappingConstructorEntry by defining isKeyValuePair() and valueOp()). Ensure compilation by updating any constructors, type assertions, and slice element types from NewMap/MappingConstructorEntry/MappingConstructorKeyValueEntry to newMap/mappingConstructorEntry/mappingConstructorKeyValueEntry.
🧹 Nitpick comments (3)
ast/types.go (1)
456-463: Consider returning an error or sentinel value instead of panicking for invalid state.The panics provide clear messages but could crash the compiler on malformed input. This may be acceptable if these invariants are guaranteed by construction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast/types.go` around lines 456 - 463, Replace the panics in BLangConstrainedType.GetTypeKind so invalid state doesn't crash the compiler: instead of panicking when this.Type.TypeDescriptor is nil or doesn't implement model.BuiltInReferenceTypeNode, return a safe sentinel value (e.g., model.TypeKind_UNSPECIFIED or the zero value of model.TypeKind) and optionally log an error or warning; update any callers of GetTypeKind if they rely on panics to handle the invalid-state case. Ensure the checks remain (this.Type.TypeDescriptor and type assertion to model.BuiltInReferenceTypeNode) and use the sentinel return consistently.semantics/semantic_analyzer.go (1)
620-623: Resolve or track the FIXME in list inherent-type selectionLine 620 marks uncertainty about the pre-analysis pass. If it’s required, please add a short rationale or link to an issue; if not, consider removing the extra pass to avoid double analysis.
If you want, I can help draft a focused issue or propose the simplified flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semantics/semantic_analyzer.go` around lines 620 - 623, The FIXME indicates uncertainty about the extra pre-analysis loop that calls analyzeExpression(a, expr, nil) for each item in expr.Exprs in semantic_analyzer.go; resolve it by either (A) removing this pre-pass if expressions are already fully analyzed later (delete the for loop invoking analyzeExpression and run existing downstream analysis once), or (B) retain it but add a short rationalizing comment above the loop explaining why a pre-analysis pass is required (e.g., to populate type info/side-effects before selection handling) and add a link or issue ID to track the decision; reference analyzeExpression and expr.Exprs when making the change so reviewers can verify the intended flow.ast/expressions.go (1)
1012-1014: Return a copy in GetFields to avoid external mutation
GetFields()returns the backing slice directly, which allows callers to mutate the AST. Most other getters return a copy; consider mirroring that pattern here.♻️ Suggested tweak
func (this *BLangRecordLiteral) GetFields() []model.RecordField { - return this.Fields + result := make([]model.RecordField, len(this.Fields)) + copy(result, this.Fields) + return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ast/expressions.go` around lines 1012 - 1014, GetFields currently returns the backing slice this.Fields directly which allows external callers to mutate the AST; change BLangRecordLiteral.GetFields to return a shallow copy of the slice (e.g., allocate a new slice of the same length and copy the elements) so callers receive a separate slice while preserving the original this.Fields.
🤖 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/expressions.go`:
- Around line 342-343: Remove the misleading type assertions that claim
BLangRecordKey implements model.Node and BLangNode: delete the assertions that
bind _ model.Node = &BLangRecordKey{} and _ BLangNode = &BLangRecordKey{}.
BLangRecordKey is an internal wrapper used only inside BLangRecordKeyValueField
and does not provide a valid GetKind()/NodeKind implementation (GetKind()
currently panics), so remove these assertions to prevent false guarantees and
the panic; verify BLangRecordKeyValueField remains the exported node that
implements the node interfaces and that no other code relies on BLangRecordKey
being a standalone node.
In `@ast/walk.go`:
- Around line 409-418: The record-literal walker currently always traverses key
expressions for BLangRecordLiteral fields; skip walking computed key expressions
by checking the BLangRecordKeyValueField's kv.Key.ComputedKey flag before
calling Walk: in the BLangRecordLiteral case, only call Walk(v,
kv.Key.Expr.(BLangNode)) when kv.Key != nil, kv.Key.Expr != nil and
kv.Key.ComputedKey is false, leaving the existing Walk for kv.ValueExpr
unchanged.
In `@semtypes/mapping_alternative.go`:
- Around line 21-25: The MappingAlternative struct currently exports SemType but
external code only uses Pos; change SemType to be unexported (rename to semType)
while keeping Pos public so existing external accesses still work; update all
internal references to MappingAlternative.SemType -> MappingAlternative.semType
(constructors, methods, tests) and any JSON/tags or interfaces that relied on
the exported name; run build/tests to catch remaining usages and revert to
exported only if something external truly requires it.
---
Outside diff comments:
In `@bir/non_terminator.go`:
- Around line 33-99: Rename the exported types and their methods to unexported
identifiers: change MappingConstructorEntry to mappingConstructorEntry and its
methods IsKeyValuePair() and ValueOp() to isKeyValuePair() and valueOp(); change
MappingConstructorKeyValueEntry to mappingConstructorKeyValueEntry (its fields
keyOp and valueOp are already unexported); and change NewMap to newMap (keep its
fields Type and Values unexported as needed—rename Value slice type references
to mappingConstructorEntry). Update all usages/implementations in this file (and
any other bir package files) so the implementations of the interface use the new
lowercase method names and the type names match (e.g.,
mappingConstructorKeyValueEntry should implement mappingConstructorEntry by
defining isKeyValuePair() and valueOp()). Ensure compilation by updating any
constructors, type assertions, and slice element types from
NewMap/MappingConstructorEntry/MappingConstructorKeyValueEntry to
newMap/mappingConstructorEntry/mappingConstructorKeyValueEntry.
In `@context/context.go`:
- Around line 134-139: The constant ANON_SYMBOL should be made unexported per
guidelines; rename the constant ANON_SYMBOL to anon_symbol in the const block
(alongside ANON_PREFIX, BUILTIN_ANON_TYPE, ANON_TYPE) and update any references
to use anon_symbol; after renaming, run a build or grep to ensure no external
packages rely on the exported name and adjust tests/imports if needed.
---
Duplicate comments:
In `@bir/non_terminator.go`:
- Around line 77-83: NewMap currently embeds BIRInstructionBase (which provides
LhsOp) but does not implement GetLhsOperand(), so it fails to satisfy
BIRAssignInstruction; add a GetLhsOperand() method on the NewMap receiver that
returns the embedded LhsOp (matching the signature used by other assign
instructions), add an assertion in the assign-instruction block to ensure NewMap
implements BIRAssignInstruction, and consider adding the same assertion for
NewArray for consistency; reference symbols: NewMap, BIRInstructionBase,
GetLhsOperand, BIRAssignInstruction, NewArray.
In `@semantics/semantic_analyzer.go`:
- Around line 667-760: Centralize record-key extraction into a helper (e.g.,
extractRecordKeyName(node ast.BNode) (string, error)) and replace the ad-hoc key
handling in analyzeRecordLiteralExpr and selectMappingInherentType: use the
helper wherever code now assumes kv.Key.Expr is a *ast.BLangLiteral or
ast.BNodeWithSymbol, return/emit a semantic error for unsupported key expression
kinds, and avoid unchecked type assertions; ensure the helper consistently uses
the same string source (prefer GetOriginalValue() or the Literal.Value cast to
string) so both analyzeRecordLiteralExpr (requiredType :=
mappingAtomicType.FieldInnerVal(keyName)) and selectMappingInherentType
(fieldNames building and per-field checks) operate on the same normalized key
name and handle missing fields gracefully.
In `@semtypes/field_pair.go`:
- Around line 158-168: NewFieldPairs fails to initialize
mappingPairIterator.rest1 and rest2, so internalNext() will use zero-valued
slices and produce incorrect pairings; update NewFieldPairs (the constructor
that returns i.toIterator()) to set i.rest1 = m1.Types[i.len1:] and i.rest2 =
m2.Types[i.len2:] (or the equivalent remaining-name/type slices used by
internalNext()), ensuring rest1/rest2 contain the remaining types/names for
correct pairing in internalNext().
In `@semtypes/mapping_alternative.go`:
- Around line 72-101: In MappingAlternativeAllowsFields, avoid brittle pointer
comparison and builtin shadowing: replace the pointer equality check
"CellInnerVal(pos.Rest) == &UNDEF" with a proper value-based check (e.g., call a
provided predicate like CellInnerVal(pos.Rest).IsUndef(), or use a
value-equality check such as reflect.DeepEqual(CellInnerVal(pos.Rest), UNDEF) or
an available Equals method) to detect the UNDEF sentinel; and rename the local
variable "len := len(fieldNames)" to something non-shadowing like "n" or "total"
and update its uses accordingly.
---
Nitpick comments:
In `@ast/expressions.go`:
- Around line 1012-1014: GetFields currently returns the backing slice
this.Fields directly which allows external callers to mutate the AST; change
BLangRecordLiteral.GetFields to return a shallow copy of the slice (e.g.,
allocate a new slice of the same length and copy the elements) so callers
receive a separate slice while preserving the original this.Fields.
In `@ast/types.go`:
- Around line 456-463: Replace the panics in BLangConstrainedType.GetTypeKind so
invalid state doesn't crash the compiler: instead of panicking when
this.Type.TypeDescriptor is nil or doesn't implement
model.BuiltInReferenceTypeNode, return a safe sentinel value (e.g.,
model.TypeKind_UNSPECIFIED or the zero value of model.TypeKind) and optionally
log an error or warning; update any callers of GetTypeKind if they rely on
panics to handle the invalid-state case. Ensure the checks remain
(this.Type.TypeDescriptor and type assertion to model.BuiltInReferenceTypeNode)
and use the sentinel return consistently.
In `@semantics/semantic_analyzer.go`:
- Around line 620-623: The FIXME indicates uncertainty about the extra
pre-analysis loop that calls analyzeExpression(a, expr, nil) for each item in
expr.Exprs in semantic_analyzer.go; resolve it by either (A) removing this
pre-pass if expressions are already fully analyzed later (delete the for loop
invoking analyzeExpression and run existing downstream analysis once), or (B)
retain it but add a short rationalizing comment above the loop explaining why a
pre-analysis pass is required (e.g., to populate type info/side-effects before
selection handling) and add a link or issue ID to track the decision; reference
analyzeExpression and expr.Exprs when making the change so reviewers can verify
the intended flow.
heshanpadmasiri
force-pushed
the
feat/map-frontend
branch
from
50ae383 to
ffad7c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/expressions.go`:
- Around line 985-987: The panic message in the method GetKind on
BLangMappingKey is stale from a rename—update the panic string in func (this
*BLangMappingKey) GetKind() to reference BLangMappingKey instead of
BLangRecordKey (i.e., change the panic to "BLangMappingKey has no NodeKind") so
the message matches the type and aids debugging; locate the GetKind method on
type BLangMappingKey and replace the literal panic text accordingly.
---
Duplicate comments:
In `@ast/expressions.go`:
- Around line 342-343: Remove the misleading interface assertions for
BLangMappingKey by deleting the two lines that assert "_ model.Node =
&BLangMappingKey{}" and "_ BLangNode = &BLangMappingKey{}"; BLangMappingKey is
an internal wrapper used only inside BLangMappingKeyValueField and its GetKind()
panics, so these assertions give a false guarantee—remove them and run the
build/tests to ensure no other code depends on BLangMappingKey implementing
model.Node or BLangNode.
In `@semantics/type_resolver.go`:
- Around line 459-487: In resolveRecordLiteral, kv.Key.Expr can be other node
types so add an explicit default branch in the switch over kv.Key.Expr that
handles unsupported key expressions: emit a diagnostic on the resolver context
(e.g., call the resolver/context error reporting helper) and skip adding that
field (continue the loop) instead of letting keyName remain empty and calling
semtypes.FieldFrom; ensure the rest of the function (map construction via
semtypes.NewMappingDefinition, DefineMappingTypeWrapped, setExpectedType,
ToMappingAtomicType) remains unchanged so valid fields are still processed.
ffad7c4 to
fe1c774
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
semtypes/mapping_ops.go (1)
90-115:⚠️ Potential issue | 🔴 CriticalAvoid mutating shared
pos.Typeswhenposoriginates from cached atoms.When
poscomes fromcx.mappingAtomType()(as in the initial call at line 39 of mappingUnionHasNonEmptyComplement), it may point to a cachedMappingAtomicTypefrom the predefined type environment. The in-place modification on lines 111–113 mutates the underlying array, corrupting cached atoms for all subsequent operations.🛠️ Suggested fix
- posTypes := pos.Types + posTypes := append([]CellSemType(nil), pos.Types...) posTypes[*fieldPair.Index1] = *d mt = MappingAtomicTypeFrom(pos.Names, posTypes, pos.Rest)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semtypes/mapping_ops.go` around lines 90 - 115, The mappingInhabited function mutates pos.Types in-place which can corrupt cached MappingAtomicType instances returned by cx.mappingAtomType; to fix, whenever you would modify pos.Types (inside the branch handling fieldPair.Index1 != nil) create a fresh copy of the slice (e.g., newPosTypes := make([]Type, len(pos.Types)); copy(newPosTypes, pos.Types)) then write the updated element into the copy and build the new MappingAtomicType via MappingAtomicTypeFrom using the copied slice; similarly ensure insertField usage for the Index1 == nil branch does not mutate shared state and always constructs a new MappingAtomicType instead of mutating pos.Types directly.
🧹 Nitpick comments (3)
corpus/bal/subset4/04-map/simple-e.bal (1)
16-16: Unused import.The
ballerina/iomodule is imported but not used anywhere in this test file. Consider removing it unless it's intentionally included to verify that unused imports don't interfere with the expected error detection.🧹 Suggested cleanup
-import ballerina/io; -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@corpus/bal/subset4/04-map/simple-e.bal` at line 16, The import statement "import ballerina/io;" is unused in this test file; remove the unused import to avoid lint errors and keep the test minimal — locate the import declaration (import ballerina/io) at the top of simple-e.bal and delete it unless you intentionally need io for test purposes.semantics/symbol_resolver.go (1)
581-584: Remove unused/orphan type declaration.Line 584 declares
defferedMethodSymbol(note typo: "deffered" vs "deferred") but it appears to be an incomplete duplicate ofdeferredMethodSymboldefined at line 341. This dead code should be removed.♻️ Proposed fix
// We can't determine if a symbol is actually a method or not without resolivng the expression // Also we can't really resolve the actual method until we know the type of reciever // Thus we need to defer the resolution of the method until type resolution -type defferedMethodSymbol struct{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semantics/symbol_resolver.go` around lines 581 - 584, Remove the unused orphan type declaration defferedMethodSymbol (typo'd) which duplicates deferredMethodSymbol; delete the type declaration line and any references to defferedMethodSymbol so only the correct deferredMethodSymbol remains, ensuring there are no remaining uses of the removed symbol.semantics/semantic_analyzer.go (1)
620-623: Resolve or remove the FIXME pre‑analysis loop.This loop re‑analyzes list members that were already analyzed in
analyzeListConstructorExpr, and the FIXME suggests uncertainty. If it’s not required, drop it to avoid redundant work; otherwise, add a brief rationale or test coverage.🧹 Proposed cleanup (if not required)
- // FIXME: is this needed? - for _, expr := range expr.Exprs { - analyzeExpression(a, expr, nil) - }Do you want me to open a follow‑up issue or draft a test to validate whether this loop is necessary?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semantics/semantic_analyzer.go` around lines 620 - 623, The for-loop that iterates over expr.Exprs and calls analyzeExpression(a, expr, nil) appears redundant because list members are already handled in analyzeListConstructorExpr; remove the loop unless you can show a specific case where members are not analyzed there. Locate the block containing "for _, expr := range expr.Exprs { analyzeExpression(a, expr, nil) }" and delete it, or if you determine it is needed, add a clear comment explaining why and add a unit test that exercises list construction paths to prove members require this second pass; reference analyzeListConstructorExpr and analyzeExpression when adding the comment or tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bir/non_terminator.go`:
- Around line 33-36: The interface and related types/functions are currently
exported but only used within the bir package; rename MappingConstructorEntry to
mappingConstructorEntry, MappingConstructorKeyValueEntry to
mappingConstructorKeyValueEntry, and NewMap to newMap, and update all internal
references/usages (methods like IsKeyValuePair, ValueOp on the renamed
interface/type should remain but their receiver/type names should be updated) so
the package API is reduced; ensure tests and any other files in the package
reference the new lowercase names and run go build/tests to catch remaining
references.
In `@desugar/expression.go`:
- Around line 345-372: The function walkRecordLiteral references an undefined
type Context; fix by making its parameter use the actual context type used
elsewhere in this package (match the type used by walkExpression and other
walker functions) or restore/rename the Context type so it is defined for all
build tags. Update walkRecordLiteral's signature to accept the existing context
type (e.g., change *Context to the package's canonical context type) and ensure
the corresponding type definition (Context) is declared/available in the desugar
package for all build configurations.
---
Outside diff comments:
In `@semtypes/mapping_ops.go`:
- Around line 90-115: The mappingInhabited function mutates pos.Types in-place
which can corrupt cached MappingAtomicType instances returned by
cx.mappingAtomType; to fix, whenever you would modify pos.Types (inside the
branch handling fieldPair.Index1 != nil) create a fresh copy of the slice (e.g.,
newPosTypes := make([]Type, len(pos.Types)); copy(newPosTypes, pos.Types)) then
write the updated element into the copy and build the new MappingAtomicType via
MappingAtomicTypeFrom using the copied slice; similarly ensure insertField usage
for the Index1 == nil branch does not mutate shared state and always constructs
a new MappingAtomicType instead of mutating pos.Types directly.
---
Duplicate comments:
In `@ast/expressions.go`:
- Around line 985-987: The panic message in BLangMappingKey.GetKind is stale
after a rename; update the panic string to reference BLangMappingKey instead of
BLangRecordKey. Locate the GetKind method on the BLangMappingKey type and change
the panic call to a message like "BLangMappingKey has no NodeKind" so the
runtime error reflects the correct type name.
- Around line 338-343: The file contains compile-time interface assertions that
claim BLangMappingKey implements model.Node and BLangNode; remove those two
assertions referencing model.Node and BLangNode for BLangMappingKey because
BLangMappingKey.GetKind() can panic and thus these assertions give a false
guarantee—delete the lines that set _ model.Node = &BLangMappingKey{} and _
BLangNode = &BLangMappingKey{} (leave other assertions like _
model.RecordLiteralNode, _ model.MappingKeyValueFieldNode, and _ BLangExpression
intact).
In `@bir/non_terminator.go`:
- Around line 77-83: NewMap embeds BIRInstructionBase (which provides LhsOp) but
does not implement GetLhsOperand(), so add a method on the NewMap type named
GetLhsOperand() that returns the embedded LhsOp (matching the
BIRAssignInstruction signature) to ensure NewMap satisfies BIRAssignInstruction;
apply the same fix for any other struct types using BIRInstructionBase that are
missing GetLhsOperand().
In `@corpus/integration_test.go`:
- Around line 64-79: The integration test skip list currently disables all new
map tests, removing coverage for the map-values feature; update the skip
handling in corpus/integration_test.go by removing the 16 "subset4/04-map/..."
entries from the skippedFiles/skipList slice (or equivalent variable) so those
tests run, or if some are known-broken, narrow the list to only the failing
filenames and add a TODO/comment linking a tracking issue; ensure the change
touches the skip list manipulation near the test registration/run function so
the specific "subset4/04-map/01-e.bal" through "union3-v.bal" entries are no
longer globally skipped.
In `@semantics/semantic_analyzer.go`:
- Around line 667-760: selectMappingInherentType and analyzeRecordLiteralExpr
currently do unchecked casts like kv.Key.Expr.(*ast.BLangLiteral) which can
panic for symbol/computed keys; add a small helper (e.g., getMappingKeyName)
used by both functions to safely extract key names by type-switching on
kv.Key.Expr to handle *ast.BLangLiteral and ast.BNodeWithSymbol (using
a.ctx().SymbolName for symbols) and return an (string, bool) indicating success;
update analyzeRecordLiteralExpr to treat unsupported key kinds by reporting
a.semanticErr(...) or skipping as appropriate and pass only verified key names
to MappingAlternativeAllowsFields and to field/type checks inside
selectMappingInherentType so no unchecked type assertions remain.
In `@semantics/type_resolver.go`:
- Around line 459-479: In resolveRecordLiteral, guard the unchecked cast
f.(*ast.BLangMappingKeyValueField) by using a type switch or ok check and handle
non-key fields (skip or return a sensible error) so the function won't panic; in
the key expression switch over kv.Key.Expr (which currently handles
*ast.BLangLiteral and ast.BNodeWithSymbol), add a default branch that handles
unknown key expression types (e.g., log/skip/set keyName to a safe sentinel) and
avoid leaving keyName empty, and when handling ast.BNodeWithSymbol continue to
call t.ctx.SetSymbolType and t.ctx.SymbolName as now; ensure the code updates
semtypes.FieldFrom only after a valid keyName and value type are established.
In `@semtypes/field_pair.go`:
- Around line 158-168: The mappingPairIterator returned by NewFieldPairs is
missing initialization for its rest1 and rest2 fields, but internalNext() (used
by toIterator()) assumes they are set; update NewFieldPairs to set rest1 and
rest2 from the incoming MappingAtomicType values (e.g., m1.Rest and m2.Rest or
the appropriate rest fields on MappingAtomicType) so mappingPairIterator.rest1
and mappingPairIterator.rest2 are non-nil before calling toIterator(); ensure
you reference mappingPairIterator, NewFieldPairs, rest1, rest2, and internalNext
when making the change.
In `@semtypes/mapping_alternative.go`:
- Around line 21-24: MappingAlternative currently exposes the field SemType
publicly; if no code outside the semtypes package reads or writes
MappingAlternative.SemType, change its visibility to semType (unexport) to
reduce API surface and update all internal semtypes usages to the new name;
before renaming, run the provided rg search to confirm there are no external
references, and if any external package does use it, leave it exported or add
accessor methods (e.g., SemType() getter) and update call sites accordingly.
- Around line 72-85: In MappingAlternativeAllowsFields, avoid pointer-identity
check and the builtin shadow: replace the pointer equality check
CellInnerVal(pos.Rest) == &UNDEF with a proper value/semantic check (e.g.,
CellInnerVal(pos.Rest) == UNDEF or call an existing IsUndef/IsUndefined helper)
so you don’t rely on &UNDEF being canonical, and rename the local variable len
to something else (e.g., n or total) to avoid shadowing the builtin len
function; update all uses of that renamed variable (the for-loop bounds that use
len) accordingly.
---
Nitpick comments:
In `@corpus/bal/subset4/04-map/simple-e.bal`:
- Line 16: The import statement "import ballerina/io;" is unused in this test
file; remove the unused import to avoid lint errors and keep the test minimal —
locate the import declaration (import ballerina/io) at the top of simple-e.bal
and delete it unless you intentionally need io for test purposes.
In `@semantics/semantic_analyzer.go`:
- Around line 620-623: The for-loop that iterates over expr.Exprs and calls
analyzeExpression(a, expr, nil) appears redundant because list members are
already handled in analyzeListConstructorExpr; remove the loop unless you can
show a specific case where members are not analyzed there. Locate the block
containing "for _, expr := range expr.Exprs { analyzeExpression(a, expr, nil) }"
and delete it, or if you determine it is needed, add a clear comment explaining
why and add a unit test that exercises list construction paths to prove members
require this second pass; reference analyzeListConstructorExpr and
analyzeExpression when adding the comment or tests.
In `@semantics/symbol_resolver.go`:
- Around line 581-584: Remove the unused orphan type declaration
defferedMethodSymbol (typo'd) which duplicates deferredMethodSymbol; delete the
type declaration line and any references to defferedMethodSymbol so only the
correct deferredMethodSymbol remains, ensuring there are no remaining uses of
the removed symbol.
6012ef1 to
17b7673
Compare
heshanpadmasiri
force-pushed
the
feat/map-frontend
branch
from
17b7673 to
ecaf006
Compare
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
Resolves #135
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
New Features
{x: 1, y: 2}).Tests