Add front end support for type test expressions#147
Add front end support for type test expressions#147warunalakshitha merged 2 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:
📝 WalkthroughWalkthroughAdds end-to-end support for type-test expressions: new AST node Changes
Sequence DiagramsequenceDiagram
participant Parser
participant SemanticAnalyzer
participant TypeResolver
participant Desugarer
participant BIRGenerator
participant Runtime
Parser->>SemanticAnalyzer: emit BLangTypeTestExpr(Expr, Type, isNegation)
SemanticAnalyzer->>TypeResolver: resolve type-test expression
TypeResolver->>TypeResolver: resolve inner expr type and tested TypeData
TypeResolver->>SemanticAnalyzer: return computed semtype (boolean / const)
SemanticAnalyzer->>Desugarer: pass resolved BLangTypeTestExpr
Desugarer->>BIRGenerator: produce desugared expression + init stmts
BIRGenerator->>BIRGenerator: emit TypeTest instruction (LhsOp, RhsOp, Type, IsNegation)
BIRGenerator->>Runtime: runtime representation
Runtime->>Runtime: evaluate type test -> boolean (apply negation if set)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
corpus/integration_test.go (1)
58-62: Make the skip intent explicit so it’s easy to revisit.These new type-test cases are now excluded from the integration suite, which hides coverage for the new feature. Consider extracting the list into a helper with a descriptive name (and tracking the temporary nature in an issue) so it’s clear this is not permanent.
♻️ Example refactor to make the skip intent explicit
- skipTestsMap = makeSkipTestsMap([]string{ - "subset4/04-typetest/1-v.bal", - "subset4/04-typetest/2-v.bal", - }) + skipTestsMap = makeSkipTestsMap(skipTestsWithRuntimePanics()) + +func skipTestsWithRuntimePanics() []string { + return []string{ + "subset4/04-typetest/1-v.bal", + "subset4/04-typetest/2-v.bal", + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@corpus/integration_test.go` around lines 58 - 62, Extract the two type-test file paths into a clearly named helper function (e.g., skippedTypeTestFiles or temporarilySkippedTypeTests) and replace the inline slice passed to makeSkipTestsMap with a call to that helper; add a TODO comment in the helper noting this is temporary and include an issue tracker ID or placeholder to revisit/remove, so the skip intent is explicit when inspecting skipTestsMap and makeSkipTestsMap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/subset4.md`:
- Line 66: Fix the typo in the documentation sentence "Only fallowing libraries
with given methods are supported" by replacing "fallowing" with "following" so
the line reads "Only following libraries with given methods are supported";
update the sentence in doc/subset4.md (the markdown text) to apply this
correction.
- Line 48: Update the typo in the markdown link text by changing "expressoin" to
"expression" in the link label "[Type test expressoin]" so it reads "[Type test
expression]" while leaving the URL
(https://ballerina.io/spec/lang/master/#section_6.28) unchanged.
---
Nitpick comments:
In `@corpus/integration_test.go`:
- Around line 58-62: Extract the two type-test file paths into a clearly named
helper function (e.g., skippedTypeTestFiles or temporarilySkippedTypeTests) and
replace the inline slice passed to makeSkipTestsMap with a call to that helper;
add a TODO comment in the helper noting this is temporary and include an issue
tracker ID or placeholder to revisit/remove, so the skip intent is explicit when
inspecting skipTestsMap and makeSkipTestsMap.
5a3a549 to
c656510
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/subset4.md`:
- Line 3: Change the heading text "Module level declarations" to use a hyphen
for consistency: update the heading string in doc/subset4.md from "Module level
declarations" to "Module-level declarations" (locate the heading line containing
that exact text).
In `@semantics/semantic_analyzer.go`:
- Around line 514-517: The BLangTypeTestExpr case is missing recursion into its
inner expression, so add an explicit call to analyzeExpression for expr.Expr
before calling validateResolvedType; locate the BLangTypeTestExpr case in the
semantic analyzer and insert analyzeExpression(a, expr.Expr, nil) (or the
appropriate expectedType if the branch requires it) immediately prior to the
call to validateResolvedType(a, expr, expectedType) so nested semantic errors
inside expr.Expr are analyzed.
c656510 to
d5ecb62
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bir/non_terminator.go (1)
194-201: Consider adding aNewTypeTestconstructor for consistency withNewMove.The
GetLhsOperandandGetKindimplementations correctly follow the established pattern. However,NewMove(the only similar assign instruction constructor) explicitly marks the destination operand as initialized. The currentTypeTestinstantiation inbir_gen.go(line 549) uses direct field assignment without this safeguard.Adding a
NewTypeTestconstructor would align with the constructor pattern and ensure proper initialization of the destination operand, consistent with how variable assignments are handled elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bir/non_terminator.go` around lines 194 - 201, Add a NewTypeTest constructor that mirrors NewMove by creating and returning a *TypeTest while explicitly marking the destination operand as initialized (e.g., set DestOp.Init = true or the equivalent field on the destination operand). Replace direct struct literal instantiations of TypeTest (the spots that currently assign fields directly) to call NewTypeTest instead. Keep existing methods GetLhsOperand and GetKind unchanged; ensure the constructor initializes all required fields (LhsOp, DestOp, any type/test info) and follows the same initialization semantics used by NewMove.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@doc/subset4.md`:
- Line 3: Update the heading "Module level declarations" in doc/subset4.md to
use a hyphen for consistency by changing it to "Module-level declarations";
locate the heading text within the file and replace the unhyphenated form with
the hyphenated form.
---
Nitpick comments:
In `@bir/non_terminator.go`:
- Around line 194-201: Add a NewTypeTest constructor that mirrors NewMove by
creating and returning a *TypeTest while explicitly marking the destination
operand as initialized (e.g., set DestOp.Init = true or the equivalent field on
the destination operand). Replace direct struct literal instantiations of
TypeTest (the spots that currently assign fields directly) to call NewTypeTest
instead. Keep existing methods GetLhsOperand and GetKind unchanged; ensure the
constructor initializes all required fields (LhsOp, DestOp, any type/test info)
and follows the same initialization semantics used by NewMove.
d5ecb62 to
b226b7b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
+ Coverage 29.09% 29.28% +0.19%
==========================================
Files 254 254
Lines 54231 54319 +88
==========================================
+ Hits 15778 15908 +130
+ Misses 37479 37427 -52
- Partials 974 984 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b226b7b to
556edc3
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 `@doc/subset4.md`:
- Around line 82-110: Remove the duplicated top-level "Supported language
features (subset 4)" section and merge the two conflicting lists into one
authoritative section: keep the "Module level declarations" and "Statements"
subsections under a single heading, resolve contradictions by standardizing on
the spec terms (e.g., use "Shift expression" / "shift-expr" consistently),
ensure import support is stated once (use the accurate status for Import
declarations), and eliminate the duplicate heading to fix the lint; update or
combine entries for conflicting items like "Shift expression" vs "shift-expr"
and import differences so the final section is consistent and non-redundant.
16810e6 to
06d0864
Compare
06d0864 to
5ff6e86
Compare
Co-authored-by: Waruna Lakshitha <warunalakshitha@users.noreply.github.com>
032a2a9
into
ballerina-platform:main
Purpose
Resolves #145
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
New Features
isand negated!is) with compiler, resolver, IR and pretty-printer support enabling type checks and narrowing.Documentation
Examples / Tests