Correctly handle switches that use an unresolved enum as the selector#411
Correctly handle switches that use an unresolved enum as the selector#411
Conversation
📝 WalkthroughWalkthroughThis change adds enum support to the Specimin codebase, including API additions to UnsolvedClassOrInterface for enum management (isAnEnum flag, enumConstants collection, and related accessors), enhanced error handling in JavaTypeCorrect for incompatible type detection with switch-expression edge cases, and refactored switch-handling logic in UnsolvedSymbolVisitor with new visitor methods for SwitchStmt and SwitchEntry. Updated JavaParser.astub adds setGuard() and setComment() method declarations. Two new test classes validate enum constant scoping in switch statements and expressions with accompanying test resource files. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/checkerframework/specimin/UnsolvedClassOrInterface.java (1)
3249-3279: Preserve enum metadata when merging synthetic classes.
updateMissingClassHelpernow needs to copy the enum flag and constants, but it still only merges methods/fields/type-vars/interfaces. Whenever we migrate a synthetic type (e.g., viamigrateTypeafter JavaTypeCorrect renames it) or otherwise re-use this helper, we overwrite the existing entry and silently dropisAnEnumand the collected constants. The regenerated source then reverts to aclasswith no enum constants, reintroducing the exact switch resolution failure this PR is fixing. Please propagate the enum state while merging, e.g.for (String variablesDescription : from.getClassFields()) { to.addFields(variablesDescription); } if (from.getNumberOfTypeVariables() > 0) { to.setNumberOfTypeVariables(from.getNumberOfTypeVariables()); } + if (from.isAnEnum) { + to.isAnEnum = true; + to.enumConstants.addAll(from.enumConstants); + } + // if a "class" is found to be an interface even once (because it appears
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
JavaParser.astub(1 hunks)src/main/java/org/checkerframework/specimin/JavaTypeCorrect.java(2 hunks)src/main/java/org/checkerframework/specimin/UnsolvedClassOrInterface.java(7 hunks)src/main/java/org/checkerframework/specimin/UnsolvedSymbolVisitor.java(5 hunks)src/test/java/org/checkerframework/specimin/SwitchEnumConstant2Test.java(1 hunks)src/test/java/org/checkerframework/specimin/SwitchEnumConstantTest.java(1 hunks)src/test/resources/switchenumconstant/expected/com/example/Simple.java(1 hunks)src/test/resources/switchenumconstant/expected/org/example/MyEnum.java(1 hunks)src/test/resources/switchenumconstant/input/com/example/Simple.java(1 hunks)src/test/resources/switchenumconstant2/expected/com/example/Foo.java(1 hunks)src/test/resources/switchenumconstant2/expected/com/example/Simple.java(1 hunks)src/test/resources/switchenumconstant2/expected/org/example/MyEnum.java(1 hunks)src/test/resources/switchenumconstant2/input/com/example/Simple.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/test/java/org/checkerframework/specimin/SwitchEnumConstant2Test.java (2)
src/test/java/org/checkerframework/specimin/SpeciminTestExecutor.java (1)
SpeciminTestExecutor(21-204)src/test/java/org/checkerframework/specimin/EnumSwitchTest.java (1)
EnumSwitchTest(12-20)
src/test/java/org/checkerframework/specimin/SwitchEnumConstantTest.java (2)
src/test/java/org/checkerframework/specimin/SpeciminTestExecutor.java (1)
SpeciminTestExecutor(21-204)src/test/java/org/checkerframework/specimin/EnumSwitchTest.java (2)
EnumSwitchTest(12-20)Test(13-19)
src/test/resources/switchenumconstant/expected/com/example/Simple.java (1)
src/test/resources/switchenumconstant/input/com/example/Simple.java (1)
Simple(18-35)
src/test/resources/switchenumconstant2/input/com/example/Simple.java (2)
src/test/resources/switchenumconstant2/expected/com/example/Simple.java (1)
Simple(5-24)src/test/resources/switchenumconstant/input/com/example/Simple.java (1)
Simple(18-35)
src/test/resources/switchenumconstant/input/com/example/Simple.java (1)
src/test/resources/switchenumconstant/expected/com/example/Simple.java (1)
Simple(5-24)
src/test/resources/switchenumconstant2/expected/com/example/Simple.java (3)
src/test/resources/switchenumconstant/expected/com/example/Simple.java (1)
Simple(5-24)src/test/resources/switchenumconstant/input/com/example/Simple.java (1)
Simple(18-35)src/test/resources/switchenumconstant2/input/com/example/Simple.java (1)
Simple(19-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: windows-tester
- GitHub Check: build-gradle-project
🔇 Additional comments (10)
src/test/resources/switchenumconstant2/expected/com/example/Foo.java (1)
1-8: LGTM! Test resource correctly defines superclass fields.The public int fields CONSTANT5 and CONSTANT6 are correctly defined for testing that Specimin resolves these constants to the superclass rather than confusing them with enum constants.
src/test/resources/switchenumconstant2/expected/org/example/MyEnum.java (1)
1-5: LGTM! Test enum correctly defined.The enum provides the necessary constants for testing unqualified enum constant resolution in switch statements and expressions.
src/test/resources/switchenumconstant/expected/org/example/MyEnum.java (1)
1-5: LGTM! Test enum correctly defined.This enum definition is identical to the one in
switchenumconstant2/expectedand correctly provides the constants needed for the test scenario.src/test/resources/switchenumconstant/input/com/example/Simple.java (2)
5-17: Excellent documentation of Java's enum scoping rules in switches.The comment clearly explains the special scoping behavior that Specimin must handle, including the assumption about where unqualified enum constant names are valid. This documentation will be valuable for future maintainers.
18-34: Test coverage includes both switch forms.Good test design covering both traditional switch statements (lines 21-28) and switch expressions (lines 29-33), ensuring Specimin handles enum constant scoping correctly in both contexts.
src/test/java/org/checkerframework/specimin/SwitchEnumConstantTest.java (1)
6-17: LGTM! Test correctly configured.The test properly targets the
bar(MyEnum)method and uses the appropriate test infrastructure. The class-level comment accurately describes the test's purpose.src/test/resources/switchenumconstant2/expected/com/example/Simple.java (1)
5-23: LGTM! Expected output correctly demonstrates superclass field resolution.The test case appropriately exercises the scenario where constants from the superclass (
CONSTANT5on line 9,CONSTANT6on line 17) are used within switch contexts but should not be confused with enum constants. The structure correctly extendsFooand uses its fields.JavaParser.astub (2)
40-43: LGTM! SwitchEntry.setGuard stub correctly defined.The method signature follows JavaParser's fluent API pattern, returning
SwitchEntryfor method chaining. The@Nullableannotation on theguardparameter is appropriate since guards are optional in switch entries.
45-49: LGTM! Node.setComment stub correctly defined.The method signature follows JavaParser's fluent API pattern. The
@Nullableannotation is appropriate since comments are optional. This general Node API addition supports AST manipulation across all node types.src/main/java/org/checkerframework/specimin/JavaTypeCorrect.java (1)
335-337: All three error patterns are correctly handled by the downstream logic.The verification confirms that
updateTypeToChange()has explicit handlers for each pattern:
- Line 481: "cannot be converted to" (includes switch expression special case with conditional
startIndex)- Line 526: "incomparable types"
- Line 535: "error: incompatible types" (as the else fallback)
The error messages include file path prefixes (e.g.,
filename.java:18: error: ...), providing sufficient tokens to meet the minimum 7-token requirement at line 475. All patterns are properly parsed and routed to appropriate type extraction logic.
This fixes the proximate cause of #405. However, there is some other, seemingly-unrelated crash that's then exposed. So, I wrote some test cases for this problem, specifically, and am PRing this fix now as a logical unit.
Here's an explanation, which is copied from one of the new test cases: