Enhance support for switch statements - #99
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success1138 tests passing in 64 suites. Report generated by 🧪jest coverage report action from 89ccf30 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughChangesEnum switch support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CompilationUnit
participant prechecks as prechecks.ts
participant Checker
participant CodeGenerator
CompilationUnit->>prechecks: discover and register enum declarations
prechecks->>Checker: validate enum members and methods
Checker->>Checker: validate enum switch selectors and labels
Checker->>CodeGenerator: compile validated enum switch
CodeGenerator->>CodeGenerator: call Enum.ordinal()
CodeGenerator->>CodeGenerator: generate integer switch dispatch
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/compiler/code-generator.ts (1)
1407-1424: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftResolve enum case labels before generating integer switch keys
case REDis anExpressionName, not aLiteral. Line 1416 therefore throws aTypeErrorbefore generating switch bytecode. Resolve each enum case constant to its ordinal, matching the selector'sEnum.ordinal()conversion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/code-generator.ts` around lines 1407 - 1424, The integer switch generation in the case-label processing within the surrounding code-generator method assumes every CaseLabel expression is a Literal; update it to also resolve enum ExpressionName constants to their ordinal values, matching the selector’s Enum.ordinal() conversion, before adding values to caseValues and caseLabelMap. Preserve literal handling for non-enum cases and default-label behavior.
🧹 Nitpick comments (5)
src/types/checker/environment.ts (1)
44-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding the
Enumbase members that the code generator depends on.The global
Enumentry is an emptyClassType. It declares noordinal(),name(), orcompareTo(...)members.src/compiler/code-generator.tsemitsINVOKEVIRTUAL java/lang/Enum.ordinal()Ifor enum switch selectors, so the runtime contract assumes those members exist. A source program that callsselector.ordinal()will fail type checking withCannotFindSymbolError, even though the compiler can emit the call.Adding at least
ordinal()returningintandname()returningStringkeeps the type environment consistent with the emitted bytecode.Using
ClassTyperather thanEnumClassfor the base is correct here, becausecheckSwitchExpressionmust not accept the abstract base as a selector.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/checker/environment.ts` around lines 44 - 46, The global Enum entry in the type environment is missing members required by type checking and generated bytecode. Update the Enum ClassType declaration to add ordinal() returning int and name() returning String, preserving it as ClassType so checkSwitchExpression does not accept the abstract base as a selector.src/compiler/code-generator.ts (1)
1379-1398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the
tryblock and correct themaxStackadjustment.Two points on this normalization block.
The
tryat line 1383 wraps both thequeryClasslookup and the bytecode emission.queryClassthrowsSymbolNotFoundErrorfor an unresolved name, which is the case this code intends to tolerate. The current form also swallows any failure fromindexMethodrefInfo. Wrap only the lookup, or check for the class before emitting.Line 1393 sets
maxStacktoexprStackSize + 1.ordinal()pops the objectref and pushes an int, so the net stack change is zero and the peak stays atexprStackSize. Over-reserving is safe for the verifier, but the extra slot is unnecessary and the expression suggests a growth that does not occur.♻️ Proposed refactor
let _resultType = resultType if (_resultType && _resultType.startsWith('L') && _resultType !== 'Ljava/lang/String;') { const clean = _resultType.replace(/^L|;$/g, '') + let classInfo try { - const classInfo = cg.symbolTable.queryClass(clean) - if (classInfo.accessFlags & ACCESS_FLAGS.ACC_ENUM) { - // call java.lang.Enum.ordinal() (returns int) - cg.code.push( - OPCODE.INVOKEVIRTUAL, - 0, - cg.constantPoolManager.indexMethodrefInfo('java/lang/Enum', 'ordinal', '()I') - ) - _resultType = 'I' - maxStack = Math.max(maxStack, exprStackSize + 1) - } - } catch (e) { - // ignore: not a known class + classInfo = cg.symbolTable.queryClass(clean) + } catch { + classInfo = undefined // not a known class + } + if (classInfo && classInfo.accessFlags & ACCESS_FLAGS.ACC_ENUM) { + // call java.lang.Enum.ordinal() (returns int) + cg.code.push( + OPCODE.INVOKEVIRTUAL, + 0, + cg.constantPoolManager.indexMethodrefInfo('java/lang/Enum', 'ordinal', '()I') + ) + _resultType = 'I' } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/code-generator.ts` around lines 1379 - 1398, Narrow the try/catch in the enum normalization block around cg.symbolTable.queryClass so only unresolved-class lookup failures are ignored; let indexMethodrefInfo and bytecode emission errors propagate. When emitting Enum.ordinal(), update maxStack using exprStackSize rather than exprStackSize + 1, since the invocation has zero net stack growth.src/types/checker/index.ts (2)
585-666: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared class-body checking logic instead of duplicating the
NormalClassDeclarationbranch.Lines 585-666 duplicate lines 487-583 almost verbatim. Only the declaration list source differs:
node.classBody.classBodyDeclarationsbecomesbodyDecls. The frame setup, the constructor index arithmetic, the field initializer check, the overload index computation, and the counter updates are identical.Two copies must now stay in sync. A fix applied to one branch will silently miss the other.
Extract a helper that takes
classType, the declaration list, and the frame, then call it from both branches.The
as anycasts at lines 622, 639, 642, and 656 are also avoidable. The surroundingswitch (bodyDeclaration.kind)already narrows the node type, and the equivalent class branch needs no casts. IfbodyDeclsis typed asany[], type the enum body declaration list properly so the narrowing works.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/checker/index.ts` around lines 585 - 666, Extract the duplicated class-body checking flow from the NormalClassDeclaration and EnumDeclaration branches into a shared helper accepting classType, declaration list, and frame, preserving constructor indexing, field initializer checks, method overload resolution, and declaration counters. Invoke the helper from both branches, and type the enum declaration list so switch narrowing removes the unnecessary as any casts in the enum handling.
727-740: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the typed
caseConstantsfield for checker switch labels.
SwitchLabeldeclares onlycaseConstants, and the checker AST extractor emits that field. Remove singular-property probing andas anycasts. Narrow to thecaseConstantsvariant and iterate overswitchLabel.caseConstants. The separatesrc/ast/astExtractor/statement-extractor.tsmodel is not used by this checker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/checker/index.ts` around lines 727 - 740, Update the switch-label handling in the type-checker branch to use only the typed SwitchLabel.caseConstants field. Remove the singular caseConstant probing and all any casts, narrow to the caseConstants variant, and iterate directly over switchLabel.caseConstants while preserving the existing type-checking and error behavior.src/types/checker/prechecks.ts (1)
128-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnum constant and method registration look correct.
Registering each enum constant as a field whose type is the enum class matches Java semantics. The constructor and method handling mirrors the
NormalClassDeclarationpath.One consistency note: this branch returns on the first error, while the
NormalClassDeclarationpath accumulates errors and reports them together. Accumulating here would report all enum body problems in one pass.Also applies to: 173-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/checker/prechecks.ts` around lines 128 - 159, The EnumDeclaration processing should accumulate errors from enum constants, constructors, and methods instead of returning on the first failure. Update the enum registration loops and createMethodLocal handling to collect TypeCheckerError instances, continue processing remaining declarations, and return the combined errors after the enum body has been processed, matching the NormalClassDeclaration path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compiler/compiler-utils.ts`:
- Around line 9-10: Remove the unreachable enum entry from the class-modifier
mapping, or implement enum declaration parsing separately from
NormalClassDeclaration and assign ACCESS_FLAGS.ACC_ENUM from the declaration
kind rather than classModifier. Ensure ClassModifier remains limited to
supported class modifiers.
In `@src/types/checker/__tests__/switchStatements.test.ts`:
- Around line 76-100: Update the switch statement tests to use Java-valid
unqualified enum labels: change the positive case to case RED and retain the
incompatible case with an unqualified constant from Other, then add coverage for
resolving selector enum constants in case-label scope. Also add a test declaring
an enum at top level to exercise the registration path in prechecks alongside
the existing method-local declarations.
In `@src/types/checker/prechecks.ts`:
- Around line 223-234: The OrdinaryCompilationUnit branch of addClassParents
must traverse nested EnumDeclaration nodes, including enums declared inside
method bodies, before or alongside topLevelClassOrInterfaceDeclarations. Reuse
the existing nested-enum traversal pattern from addClasses or addClassMethods,
and ensure each discovered enum receives the Enum ClassType parent through the
existing parent-assignment logic.
- Around line 18-42: Extract the nested-enum traversal from registerNestedEnums
into one shared helper that descends into each top-level declaration’s children
without visiting the top-level declaration itself. Reuse this helper in the
declaration pass around registerNestedEnums and the pass containing
processNestedEnums, removing their local walkers and duplicate enum processing.
Also invoke the shared helper in the OrdinaryCompilationUnit branch of
addClassParents so nested enums receive the Enum parent. Apply these changes in
src/types/checker/prechecks.ts at lines 18-42, 91-107, and 223-234.
- Around line 160-172: Update the FieldDeclaration handling to convert
bodyNode.unannType with unannTypeToString before passing it to frame.getType,
while preserving the existing fieldType fallback; import unannTypeToString from
its defining module so enum field processing supplies the string expected by
getType.
In `@src/types/checker/statements.ts`:
- Around line 31-34: Update the selector validation around
isPrimitiveIntegralType, isPrimitiveLongType, isStringType, and EnumClass to
also accept boxed integral types Character, Byte, Short, and Integer, reusing
the existing type predicates or classes. Preserve rejection of Boolean and Long,
and add coverage for at least one boxed integral selector in
switchStatements.test.ts.
---
Outside diff comments:
In `@src/compiler/code-generator.ts`:
- Around line 1407-1424: The integer switch generation in the case-label
processing within the surrounding code-generator method assumes every CaseLabel
expression is a Literal; update it to also resolve enum ExpressionName constants
to their ordinal values, matching the selector’s Enum.ordinal() conversion,
before adding values to caseValues and caseLabelMap. Preserve literal handling
for non-enum cases and default-label behavior.
---
Nitpick comments:
In `@src/compiler/code-generator.ts`:
- Around line 1379-1398: Narrow the try/catch in the enum normalization block
around cg.symbolTable.queryClass so only unresolved-class lookup failures are
ignored; let indexMethodrefInfo and bytecode emission errors propagate. When
emitting Enum.ordinal(), update maxStack using exprStackSize rather than
exprStackSize + 1, since the invocation has zero net stack growth.
In `@src/types/checker/environment.ts`:
- Around line 44-46: The global Enum entry in the type environment is missing
members required by type checking and generated bytecode. Update the Enum
ClassType declaration to add ordinal() returning int and name() returning
String, preserving it as ClassType so checkSwitchExpression does not accept the
abstract base as a selector.
In `@src/types/checker/index.ts`:
- Around line 585-666: Extract the duplicated class-body checking flow from the
NormalClassDeclaration and EnumDeclaration branches into a shared helper
accepting classType, declaration list, and frame, preserving constructor
indexing, field initializer checks, method overload resolution, and declaration
counters. Invoke the helper from both branches, and type the enum declaration
list so switch narrowing removes the unnecessary as any casts in the enum
handling.
- Around line 727-740: Update the switch-label handling in the type-checker
branch to use only the typed SwitchLabel.caseConstants field. Remove the
singular caseConstant probing and all any casts, narrow to the caseConstants
variant, and iterate directly over switchLabel.caseConstants while preserving
the existing type-checking and error behavior.
In `@src/types/checker/prechecks.ts`:
- Around line 128-159: The EnumDeclaration processing should accumulate errors
from enum constants, constructors, and methods instead of returning on the first
failure. Update the enum registration loops and createMethodLocal handling to
collect TypeCheckerError instances, continue processing remaining declarations,
and return the combined errors after the enum body has been processed, matching
the NormalClassDeclaration path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 476c8050-48ea-410f-8baa-c3a0e76dd316
📒 Files selected for processing (8)
src/compiler/code-generator.tssrc/compiler/compiler-utils.tssrc/types/checker/__tests__/switchStatements.test.tssrc/types/checker/environment.tssrc/types/checker/index.tssrc/types/checker/prechecks.tssrc/types/checker/statements.tssrc/types/types/classes.ts
| ['abstract', ACCESS_FLAGS.ACC_ABSTRACT], | ||
| ['enum', ACCESS_FLAGS.ACC_ENUM] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm that 'enum' reaches classModifier and that ACC_ENUM can be set.
set -euo pipefail
echo '=== ClassModifier type definition ==='
rg -nP -C 4 '\bClassModifier\b' --type=ts src/ast/types
echo '=== Where classModifier is populated ==='
rg -nP -C 5 'classModifier' --type=ts src
echo '=== Any enum handling in the AST extractor ==='
rg -nP -C 5 -i 'enumdeclaration|enumconstant' --type=ts src/ast
echo '=== ACC_ENUM definition and all uses ==='
rg -nP -C 3 'ACC_ENUM' --type=ts srcRepository: source-academy/java-slang
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== AST class types ==='
cat -n src/ast/types/classes.ts
echo '=== Enum-related source symbols ==='
rg -n -i --glob '*.ts' '\benum\b|EnumDeclaration|enumConstant' src --glob '!**/__tests__/**' | head -200
echo '=== Class declaration extraction and parser definitions ==='
rg -n -i --glob '*.ts' 'NormalClassDeclaration|classModifier|classDeclaration' src/parser src/ast src 2>/dev/null | head -240Repository: source-academy/java-slang
Length of output: 30302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== Compiler inputs and class extraction ==='
rg -n -C 6 --glob '*.ts' 'classModifier|ClassDeclaration|NormalClassDeclaration|generateClassAccessFlags' src/compiler src/ast
echo '=== Legacy compiler grammar around class declarations ==='
sed -n '130,275p' src/compiler/grammar.ts
echo '=== Legacy class extractor ==='
cat -n src/ast/astExtractor/class-extractor.ts
echo '=== Compiler entry point ==='
sed -n '1,140p' src/compiler/compiler.ts
echo '=== New AST enum declaration and class declaration types ==='
sed -n '230,270p' src/types/ast/specificationTypes.ts
sed -n '460,485p' src/types/ast/specificationTypes.ts
sed -n '360,390p' src/types/ast/extractor.ts
sed -n '690,715p' src/types/ast/extractor.tsRepository: source-academy/java-slang
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== Legacy grammar: class modifiers and enum token ==='
sed -n '160,190p' src/compiler/grammar.ts
sed -n '490,525p' src/compiler/grammar.ts
echo '=== Legacy parser entry points ==='
fd -t f -e ts . src/ast src/compiler | sort | grep -E 'parser|grammar|extractor'
rg -n -C 8 'from .*compiler|grammar|parse\(' src/compiler src/ast --glob '*.ts' | head -180
echo '=== Class modifier implementation ==='
rg -n -C 10 'ClassModifier|possibleModifiers|classModifier\(ctx' src/compiler/grammar.ts src/ast/astExtractor/class-extractor.ts src/ast/parser* 2>/dev/nullRepository: source-academy/java-slang
Length of output: 21495
Model enum declarations separately from class modifiers
ClassModifier and ClassModifier* do not include enum. The compiler therefore cannot place enum in classModifier; its grammar does not parse enum declarations as NormalClassDeclaration. This entry cannot set ACC_ENUM. Add enum declaration support and set the flag from the declaration kind, or remove this unreachable mapping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compiler/compiler-utils.ts` around lines 9 - 10, Remove the unreachable
enum entry from the class-modifier mapping, or implement enum declaration
parsing separately from NormalClassDeclaration and assign ACCESS_FLAGS.ACC_ENUM
from the declaration kind rather than classModifier. Ensure ClassModifier
remains limited to supported class modifiers.
| }, | ||
| { | ||
| input: ` | ||
| enum Color { RED, BLUE } | ||
| Color selector = Color.RED; | ||
| switch(selector) { | ||
| case Color.RED: { | ||
| selector = Color.BLUE; | ||
| } | ||
| default: {} | ||
| } | ||
| `, | ||
| result: { type: null, errors: [] } | ||
| }, | ||
| { | ||
| input: ` | ||
| enum Color { RED, BLUE } | ||
| enum Other { X } | ||
| Color selector = Color.RED; | ||
| switch(selector) { | ||
| case Other.X: {} | ||
| default: {} | ||
| } | ||
| `, | ||
| result: { type: null, errors: [new IncompatibleTypesError()] } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
These tests use qualified enum constants in case labels, which Java does not permit.
Java requires the unqualified constant name in an enum switch label. case Color.RED: is a compile error in javac; the legal form is case RED:. Both new tests use the qualified form.
Two consequences:
- The positive test at lines 77-89 asserts that an invalid Java program type checks successfully.
- The negative test at lines 90-100 passes for the wrong reason. Real javac rejects
case Other.X:because it is qualified, not because of a type mismatch.
The unqualified form is the one that matters, and it is untested. Resolving a bare RED requires the checker to bring the selector enum's constants into the case-label scope. Nothing in src/types/checker/index.ts lines 727-740 does that, so case RED: would likely fail with CannotFindSymbolError.
Add a test using case RED: to confirm the behavior.
Both tests also declare the enum inside the main method body, because createProgram inserts the statement there. No test declares a top-level enum, so the top-level registration path in src/types/checker/prechecks.ts is uncovered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/checker/__tests__/switchStatements.test.ts` around lines 76 - 100,
Update the switch statement tests to use Java-valid unqualified enum labels:
change the positive case to case RED and retain the incompatible case with an
unqualified constant from Other, then add coverage for resolving selector enum
constants in case-label scope. Also add a test declaring an enum at top level to
exercise the registration path in prechecks alongside the existing method-local
declarations.
|
|
||
| // Register any nested enum declarations found anywhere in the compilation unit | ||
| const registerNestedEnums = (obj: any) => { | ||
| if (!obj || typeof obj !== 'object') return | ||
| if (Array.isArray(obj)) { | ||
| obj.forEach(registerNestedEnums) | ||
| return | ||
| } | ||
| if (obj.kind === 'EnumDeclaration') { | ||
| try { | ||
| const enumType = new EnumClass(obj.typeIdentifier.identifier) | ||
| const err = frame.setType(obj.typeIdentifier.identifier, enumType, obj.typeIdentifier.location) | ||
| if (err instanceof Error) { | ||
| // duplicate class — add as error | ||
| typeCheckErrors.push(new DuplicateClassError(obj.location)) | ||
| } | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
| return | ||
| } | ||
| Object.keys(obj).forEach(k => registerNestedEnums(obj[k])) | ||
| } | ||
| node.topLevelClassOrInterfaceDeclarations.forEach(registerNestedEnums) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Nested enum discovery is duplicated, applied to already-visited nodes, and missing from one pass. All three declaration passes need the same nested-enum traversal, but each handles it differently: two passes define their own untyped any walker and run it over topLevelClassOrInterfaceDeclarations, which the recursive addClasses / addClassMethods calls already visited, and the third pass has no walker at all. The shared fix is one traversal helper that descends below the top-level declarations and is reused by all three passes.
src/types/checker/prechecks.ts#L18-L42: changeregisterNestedEnumsso it starts below each top-level declaration instead of visiting the declaration itself, which stops the secondframe.setTypecall and the spuriousDuplicateClassError. Extract this traversal into a shared helper.src/types/checker/prechecks.ts#L91-L107: replace the localprocessNestedEnumswalker with the shared helper, so a top-levelEnumDeclarationis no longer passed toaddClassMethodstwice and enum constants are not re-added throughclassType.addField.src/types/checker/prechecks.ts#L223-L234: add the shared helper to theOrdinaryCompilationUnitcase ofaddClassParents, so nested enums also reach this branch and receive theEnumparent instead of keeping the defaultObjectClass.
📍 Affects 1 file
src/types/checker/prechecks.ts#L18-L42(this comment)src/types/checker/prechecks.ts#L91-L107src/types/checker/prechecks.ts#L223-L234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/checker/prechecks.ts` around lines 18 - 42, Extract the nested-enum
traversal from registerNestedEnums into one shared helper that descends into
each top-level declaration’s children without visiting the top-level declaration
itself. Reuse this helper in the declaration pass around registerNestedEnums and
the pass containing processNestedEnums, removing their local walkers and
duplicate enum processing. Also invoke the shared helper in the
OrdinaryCompilationUnit branch of addClassParents so nested enums receive the
Enum parent. Apply these changes in src/types/checker/prechecks.ts at lines
18-42, 91-107, and 223-234.
| case 'FieldDeclaration': { | ||
| const fieldType = frame.getType( | ||
| (bodyNode as any).unannType ? (bodyNode as any).unannType : (bodyNode as any).fieldType, | ||
| bodyNode.location | ||
| ) | ||
| if (fieldType instanceof TypeCheckerError) return newResult(null, [fieldType]) | ||
| for (const declarator of (bodyNode as any).variableDeclaratorList.variableDeclarators) { | ||
| const fieldIdentifier = declarator.variableDeclaratorId.identifier | ||
| const error = classType.addField(fieldIdentifier.identifier, fieldType, fieldIdentifier.location) | ||
| if (error instanceof TypeCheckerError) return newResult(null, [error]) | ||
| } | ||
| break | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm FieldDeclaration AST shape and how frame.getType is called elsewhere.
set -euo pipefail
echo '=== FieldDeclaration type definition ==='
ast-grep run --pattern 'export type FieldDeclaration = { $$$ }' --lang typescript src/types/ast/specificationTypes.ts
echo '=== createClassFieldsAndMethods: how class fields resolve their type ==='
fd -t f 'classFactory|createClassFieldsAndMethods' src || true
rg -nP -C 6 'createClassFieldsAndMethods' --type=ts src
echo '=== All frame.getType call sites ==='
rg -nP -C 1 '\bframe\.getType\s*\(' --type=ts srcRepository: source-academy/java-slang
Length of output: 10573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== Enum field-processing implementation ==='
sed -n '128,180p' src/types/checker/prechecks.ts
echo '=== Type lookup contract ==='
sed -n '60,95p' src/types/checker/environment.ts
echo '=== Existing class-field implementation ==='
sed -n '1,75p' src/types/typeFactories/classFactory.ts
echo '=== Type-name conversion ==='
rg -n -C 8 'export .*unannTypeToString|function unannTypeToString|const unannTypeToString' src/types/astRepository: source-academy/java-slang
Length of output: 8363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== Array-type predicate used before the type-map lookup ==='
rg -n -C 6 'export .*isArrayType|function isArrayType|const isArrayType' src/types
echo '=== prechecks imports and type utility signature ==='
sed -n '1,12p' src/types/checker/prechecks.ts
sed -n '65,105p' src/types/ast/utils.tsRepository: source-academy/java-slang
Length of output: 2226
Convert enum field types before lookup
FieldDeclaration.unannType is an AST node. frame.getType expects a string and calls isArrayType, which invokes .trim() on that argument. This causes a runtime error while processing enum fields. Use unannTypeToString(bodyNode.unannType) and import the helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/checker/prechecks.ts` around lines 160 - 172, Update the
FieldDeclaration handling to convert bodyNode.unannType with unannTypeToString
before passing it to frame.getType, while preserving the existing fieldType
fallback; import unannTypeToString from its defining module so enum field
processing supplies the string expected by getType.
| case 'EnumDeclaration': { | ||
| const classType = frame.getType(node.typeIdentifier.identifier, node.typeIdentifier.location) | ||
| if (classType instanceof Error) return newResult(null, [classType]) | ||
| if (!(classType instanceof ClassType)) throw new Error('enum type should be a ClassImpl') | ||
|
|
||
| // Enums implicitly extend java.lang.Enum (represented here as 'Enum' in the type environment) | ||
| const enumBase = frame.getType('Enum', node.typeIdentifier.location) | ||
| if (enumBase instanceof Error) return newResult(null, [enumBase]) | ||
| if (!(enumBase instanceof ClassType)) throw new Error('Enum base should be a ClassImpl') | ||
| classType.setParentClass(enumBase) | ||
| return newResult(classType) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
addClassParents has no nested-enum walker, so nested enums never receive the Enum parent.
addClasses (lines 19-41) and addClassMethods (lines 92-106) both walk the AST to find nested EnumDeclaration nodes. The OrdinaryCompilationUnit case of addClassParents at lines 193-198 does not. It only maps over topLevelClassOrInterfaceDeclarations.
An enum declared inside a method body is therefore registered and populated, but keeps the default ObjectClass parent. The tests in src/types/checker/__tests__/switchStatements.test.ts declare enums inside main, so they exercise exactly this path.
The parent assignment matters for member lookup: ClassType.accessField and ClassType.accessMethod delegate to this._parent, so a nested enum cannot resolve any inherited Enum member.
Apply the same nested traversal here, or centralize the traversal once and reuse it in all three passes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/checker/prechecks.ts` around lines 223 - 234, The
OrdinaryCompilationUnit branch of addClassParents must traverse nested
EnumDeclaration nodes, including enums declared inside method bodies, before or
alongside topLevelClassOrInterfaceDeclarations. Reuse the existing nested-enum
traversal pattern from addClasses or addClassMethods, and ensure each discovered
enum receives the Enum ClassType parent through the existing parent-assignment
logic.
| if (isPrimitiveIntegralType(expressionType) && !isPrimitiveLongType(expressionType)) return null | ||
| if (isReferenceType(expressionType)) return null | ||
| if (isStringType(expressionType)) return null | ||
| if (expressionType instanceof EnumClass) return null | ||
| return new SelectorTypeNotAllowedError(location) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Boxed integral selector types are now rejected, but Java allows them.
The previous implementation accepted any reference type. This version accepts only String and EnumClass. Boxed integral types therefore now produce SelectorTypeNotAllowedError.
JLS 14.11 permits a switch selector of type char, byte, short, int, the boxed types Character, Byte, Short, Integer, String, or an enum type. Rejecting Boolean and Long is correct. Rejecting Integer is not.
The new test at src/types/checker/__tests__/switchStatements.test.ts lines 42-50 covers the Boolean rejection but not a boxed integral selector, so the regression is untested.
🐛 Proposed fix to accept boxed integral selectors
export const checkSwitchExpression = (
expressionType: Type,
location: Location
): null | TypeCheckerError => {
if (isPrimitiveIntegralType(expressionType) && !isPrimitiveLongType(expressionType)) return null
+ if (isReferenceIntegralType(expressionType) && !isReferenceLongType(expressionType)) return null
if (isStringType(expressionType)) return null
if (expressionType instanceof EnumClass) return null
return new SelectorTypeNotAllowedError(location)
}Run the following script to check which boxed-type predicates already exist:
#!/bin/bash
# Description: List existing type predicates and confirm boxed integral coverage.
set -euo pipefail
rg -nP -C 2 'export const is\w*Type\s*=' --type=ts src/types/types/utils.ts
echo '=== Reference numeric type classes ==='
rg -nP -C 2 'class (Integer|Character|Byte|Short|Long|Boolean)\b' --type=ts src/types/types🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/types/checker/statements.ts` around lines 31 - 34, Update the selector
validation around isPrimitiveIntegralType, isPrimitiveLongType, isStringType,
and EnumClass to also accept boxed integral types Character, Byte, Short, and
Integer, reusing the existing type predicates or classes. Preserve rejection of
Boolean and Long, and add coverage for at least one boxed integral selector in
switchStatements.test.ts.
1 participant
This PR aims to build on existing support for switch statements by completing the type of selector accepted, in particular narrowing it down to String, integral types or enum types.