Add backend support for type test expressions#194
Add backend support for type test expressions#194warunalakshitha wants to merge 7 commits 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughGenBir now stores a populated TypeEnv in the produced BIRPackage. The Runtime seeds the Registry with that TypeEnv before interpreting. Executor/interpreter code was updated to pass the Registry into instruction executors; a TypeTest executor and a runtime SemType mapper were added. Two corpus tests were un-skipped and a type-test corpus case was added. Changes
Sequence DiagramsequenceDiagram
participant BIRGen as BIR Generator
participant BIRPkg as BIRPackage
participant Runtime as Runtime
participant Registry as Registry
participant Executor as Executor
participant TypeTest as TypeTest Instr
BIRGen->>BIRPkg: set TypeEnv = ctx.GetTypeEnv()
BIRGen-->>Runtime: return BIRPackage
Runtime->>Registry: SetTypeEnv(pkg.TypeEnv)
Runtime->>Executor: Interpret(pkg)
Executor->>Executor: create frame (use birFunc.FunctionLookupKey)
Executor->>TypeTest: execute TypeTest instruction (pass reg)
TypeTest->>Registry: GetTypeEnv()
Registry-->>TypeTest: return typeEnv (rgba(0,128,0,0.5))
TypeTest->>TypeTest: SemTypeForValue -> evaluate subtype
TypeTest->>Executor: write boolean result to register
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
warunalakshitha
requested review from
azinneera,
gimantha,
heshanpadmasiri and
snelusha
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
==========================================
+ Coverage 29.13% 29.57% +0.43%
==========================================
Files 254 257 +3
Lines 54309 54489 +180
==========================================
+ Hits 15825 16113 +288
+ Misses 37508 37398 -110
- Partials 976 978 +2 ☔ 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: 2
🧹 Nitpick comments (1)
runtime/internal/exec/terminators.go (1)
80-86: Local variablevaluesshadows the importedvaluespackage.On line 81, the local variable
valuesshadows the"ballerina-lang-go/values"package import for the entire function body. The current code compiles becausevalues.BalValueon the RHS is resolved before the:=binding is established, but any future reference to thevaluespackage inside this function would silently resolve to the slice variable instead, leading to a confusing compile error.Rename the local to
result(or similar) to keep the package name unambiguous.♻️ Proposed rename
func extractArgs(args []bir.BIROperand, frame *Frame) []values.BalValue { - values := make([]values.BalValue, len(args)) + result := make([]values.BalValue, len(args)) for i, op := range args { - values[i] = frame.GetOperand(op.Index) + result[i] = frame.GetOperand(op.Index) } - return values + return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/internal/exec/terminators.go` around lines 80 - 86, In extractArgs, the local variable named values shadows the imported values package; rename the local slice (e.g., to result or argsValues) so the package identifier values remains unambiguous, update all references inside extractArgs (the make call and index assignments) to use the new local name, and keep the function signature and returned slice type as []values.BalValue to preserve types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@runtime/internal/exec/non_terminators.go`:
- Around line 78-84: In execTypeTest, guard against reg.GetTypeCtx() returning
nil before calling semtypes.IsSubtype: fetch typeCtx := reg.GetTypeCtx(), and if
typeCtx == nil return a clear failure path (for example set
frame.SetOperand(typeTest.LhsOp.Index, false) and/or log/raise a descriptive
error) instead of calling semtypes.IsSubtype; ensure references to execTypeTest,
reg.GetTypeCtx, semtypes.IsSubtype and frame.SetOperand are used so the check is
colocated with the existing subtype call and the LHS operand is
deterministically set when the type context is missing.
In `@values/values.go`:
- Around line 54-69: In SemTypeForValue, the case for *List can panic when the
BalValue is a typed nil or its Type field is nil; update the *List branch in
SemTypeForValue to first check if v == nil and return an appropriate nil semtype
(e.g., &semtypes.NIL), then check if v.Type == nil and return the same fallback
before returning v.Type; reference the SemTypeForValue function and the List
type/Type field when making the change.
---
Nitpick comments:
In `@runtime/internal/exec/terminators.go`:
- Around line 80-86: In extractArgs, the local variable named values shadows the
imported values package; rename the local slice (e.g., to result or argsValues)
so the package identifier values remains unambiguous, update all references
inside extractArgs (the make call and index assignments) to use the new local
name, and keep the function signature and returned slice type as
[]values.BalValue to preserve types.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bir/bir_gen.gobir/model.gocorpus/integration_test.goruntime/internal/exec/executor.goruntime/internal/exec/interpreter.goruntime/internal/exec/non_terminators.goruntime/internal/exec/terminators.goruntime/internal/modules/registry_impl.goruntime/runtime.govalues/values.go
💤 Files with no reviewable changes (1)
- corpus/integration_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
runtime/internal/exec/terminators.go (2)
73-73: IncludeFunctionLookupKeyin the not-found panic for easier debugging.
callInfo.Name.Value()is the human-readable name, but the registry is keyed onFunctionLookupKey. When this panic fires, knowing the lookup key immediately pinpoints which registration is missing.🔍 Optional improvement
- panic("function not found: " + callInfo.Name.Value()) + panic("function not found: " + callInfo.Name.Value() + " (lookup key: " + callInfo.FunctionLookupKey + ")")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/internal/exec/terminators.go` at line 73, The panic currently uses only callInfo.Name.Value() which hides the registry key; update the panic in the terminator path that now reads panic("function not found: " + callInfo.Name.Value()) to include the FunctionLookupKey as well (e.g. include FunctionLookupKey(callInfo) or the appropriate lookup-key value) so the message contains both the human-readable name and the registry key, making it clear which registration is missing.
27-28: Consider preserving execution context in the type-mismatch panic.The previous safe assertion emitted a custom message (via
fmt) that could include frame/function context. The directv.(bool)assertion now produces only Go's default"interface conversion: interface {} is X, not bool", which loses the call-site context (operand index, function key). For a runtime interpreter this context is valuable when debugging malformed BIR.🔍 Optional improvement
- v := frame.GetOperand(opIndex) - cond := v.(bool) + v := frame.GetOperand(opIndex) + cond, ok := v.(bool) + if !ok { + panic(fmt.Sprintf("execBranch: operand %d in %q is not bool (got %T)", opIndex, frame.functionKey, v)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/internal/exec/terminators.go` around lines 27 - 28, The code does a direct type assertion v.(bool) which loses execution context on mismatch; change it to perform a safe assertion (using the comma-ok idiom or a type switch) on v returned from frame.GetOperand(opIndex), assign to cond only if ok, and if not ok panic with a formatted message that includes the operand index (opIndex) and identifying frame/function context (e.g. frame or frame.Function.Key) so the panic contains the same debug information the previous fmt-based assertion provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@runtime/internal/exec/terminators.go`:
- Line 73: The panic currently uses only callInfo.Name.Value() which hides the
registry key; update the panic in the terminator path that now reads
panic("function not found: " + callInfo.Name.Value()) to include the
FunctionLookupKey as well (e.g. include FunctionLookupKey(callInfo) or the
appropriate lookup-key value) so the message contains both the human-readable
name and the registry key, making it clear which registration is missing.
- Around line 27-28: The code does a direct type assertion v.(bool) which loses
execution context on mismatch; change it to perform a safe assertion (using the
comma-ok idiom or a type switch) on v returned from frame.GetOperand(opIndex),
assign to cond only if ok, and if not ok panic with a formatted message that
includes the operand index (opIndex) and identifying frame/function context
(e.g. frame or frame.Function.Key) so the panic contains the same debug
information the previous fmt-based assertion provided.
a747f46 to
498f02d
Compare
snelusha
left a comment
snelusha
left a comment
There was a problem hiding this comment.
Just a few minor nits, otherwise LGTM!
warunalakshitha
force-pushed
the
type_test_runtime
branch
from
498f02d to
c81c1dc
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
runtime/internal/exec/terminators.go (1)
27-28: Unadressed inline suggestion — two redundant temporaries remain.The intermediate
vandcondvariables serve no purpose; the past review comment already asked for these to be collapsed into a single expression.♻️ Proposed simplification
- v := frame.GetOperand(opIndex) - cond := v.(bool) - if cond { + if frame.GetOperand(opIndex).(bool) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/internal/exec/terminators.go` around lines 27 - 28, The two redundant temporaries v and cond should be removed and the value retrieved and type-asserted in one expression; replace usages of v and cond with a single inline assertion like frame.GetOperand(opIndex).(bool) (or assign cond := frame.GetOperand(opIndex).(bool) directly if a named variable is needed) so there are no intermediate temporaries left in the terminator code that uses GetOperand and opIndex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@runtime/internal/exec/terminators.go`:
- Around line 27-28: The two redundant temporaries v and cond should be removed
and the value retrieved and type-asserted in one expression; replace usages of v
and cond with a single inline assertion like frame.GetOperand(opIndex).(bool)
(or assign cond := frame.GetOperand(opIndex).(bool) directly if a named variable
is needed) so there are no intermediate temporaries left in the terminator code
that uses GetOperand and opIndex.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
corpus/ast/subset4/04-typetest/1-v.txtis excluded by!corpus/**/*.txtcorpus/bir/subset4/04-typetest/1-v.txtis excluded by!corpus/**/*.txtcorpus/cfg/subset4/04-typetest/1-v.txtis excluded by!corpus/**/*.txtcorpus/desugared/subset4/04-typetest/1-v.txtis excluded by!corpus/**/*.txt
📒 Files selected for processing (2)
corpus/bal/subset4/04-typetest/1-v.balruntime/internal/exec/terminators.go
bir/bir_gen.go
Outdated
| birPkg.MainFunction = birFunc | ||
| } | ||
| } | ||
| birPkg.TypeCtx = semtypes.ContextFrom(ctx.GetTypeEnv()) |
There was a problem hiding this comment.
I am not sure this is a good idea. Type context is suppose to be for each thread not package. Each thread needs to create that threads type context using the type environment (which should be shared by all in the interpreter)
| } | ||
| } | ||
|
|
||
| func SemTypeForValue(v BalValue) semtypes.SemType { |
There was a problem hiding this comment.
I don't think this is going to work for simple basic types you need the singleton basic type otherwise something like this wont work
int a = 5;
io:println(a is byte)3 participants
Purpose
Fixes #146
Summary by CodeRabbit