Handle enum constants in switch expressions correctly#463
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughAdds utilities for identifying likely constants and locating switch selectors from case labels. Extends type inference for switch-expression arms and yield statements. Updates unresolved-symbol generation to synthesize enum types and constants for unqualified switch labels. Adds regression tests and fixtures covering statement switches, switch expressions, enum constants, and inherited constants. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/main/java/org/checkerframework/specimin/JavaParserUtil.java`:
- Around line 1060-1073: Consolidate the constant-detection logic by retaining
JavaParserUtil.isProbablyAConstant as the single heuristic, then replace all
usages of looksLikeAConstant in UnsolvedSymbolGenerator with
isProbablyAConstant. Remove the redundant looksLikeAConstant implementation if
it is no longer referenced, preserving digit recognition consistently.
In
`@src/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.java`:
- Around line 882-900: Update getSwitchSelectorTypeFQNs to support every valid
switch selector expression instead of restricting resolution to NameExpr and
FieldAccessExpr. Reuse
fullyQualifiedNameGenerator.getFQNsForExpressionType(selector) to derive the
selector’s erased fully qualified names, while preserving the nullable result
behavior when the expression type cannot be resolved.
- Around line 855-871: Update the enum-constant handling around
UnsolvedFieldAlternates.create to look up an existing generated field for the
same enum and constant name before creating a new instance. Reuse the existing
field when found, and add only that single canonical instance to result;
otherwise preserve the current creation and registry-update flow.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c6bb455-3797-4cc1-8e80-eb2acbdf5560
📒 Files selected for processing (12)
src/main/java/org/checkerframework/specimin/JavaParserUtil.javasrc/main/java/org/checkerframework/specimin/unsolved/FullyQualifiedNameGenerator.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.javasrc/test/java/org/checkerframework/specimin/SwitchEnumConstant2Test.javasrc/test/java/org/checkerframework/specimin/SwitchEnumConstantTest.javasrc/test/resources/switchenumconstant/expected/com/example/Simple.javasrc/test/resources/switchenumconstant/expected/org/example/MyEnum.javasrc/test/resources/switchenumconstant/input/com/example/Simple.javasrc/test/resources/switchenumconstant2/expected/com/example/Foo.javasrc/test/resources/switchenumconstant2/expected/com/example/Simple.javasrc/test/resources/switchenumconstant2/expected/org/example/MyEnum.javasrc/test/resources/switchenumconstant2/input/com/example/Simple.java
theron-wang
left a comment
theron-wang
left a comment
There was a problem hiding this comment.
Overall LGTM, just some small comments
|
I can't reproduce the CI failure locally using a Java 17 JVM (but I do get that failure if I run with a Java 21 JVM...). I'm rerunning now to see if it's ephemeral, in which case we'll probably want to exclude CF-3025 from CI - I had to rerun CI on another PR yesterday because CF-3025 was failing, and it did succeed on the rerun, so I suspect its flaky. |
There was a problem hiding this comment.
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/JavaParserUtil.java (1)
2575-2580: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the
label.isAncestorOf(nameExpr)check to prevent misclassifying nested expressions as enum constants.The
label.isAncestorOf(nameExpr)condition incorrectly treats any ALL_CAPSNameExprnested within a case label as an unqualified enum constant. For example, incase FOO.BAR:, the label is aFieldAccessExpr(FOO.BAR), andFOOis an ancestor (its scope). IfFOOis ALL_CAPS, this method will wrongly return the switch selector, causingFOOto be incorrectly synthesized as an enum constant belonging to the selector's type instead of treating it as a class.Similarly, for Java 21 pattern labels (e.g.,
case String CONSTANT:), the pattern variable would be a descendant of the pattern label expression.Java requires enum constant case labels to be strictly unqualified identifiers. Therefore, reference equality (
label == nameExpr) is both necessary and sufficient.🐛 Proposed fix
// Extracted into a local variable to minimize suppression scope. `@SuppressWarnings`({"ReferenceEquality", "not.interned"}) boolean equals = label == nameExpr; - if (equals || label.isAncestorOf(nameExpr)) { + if (equals) { inLabel = true; break; }🤖 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/main/java/org/checkerframework/specimin/JavaParserUtil.java` around lines 2575 - 2580, Update the label matching condition in the surrounding enum-constant detection logic to rely only on reference equality between label and nameExpr. Remove the label.isAncestorOf(nameExpr) alternative while preserving the existing inLabel handling and loop control.
🤖 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.
Outside diff comments:
In `@src/main/java/org/checkerframework/specimin/JavaParserUtil.java`:
- Around line 2575-2580: Update the label matching condition in the surrounding
enum-constant detection logic to rely only on reference equality between label
and nameExpr. Remove the label.isAncestorOf(nameExpr) alternative while
preserving the existing inLabel handling and loop control.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2856d32-f6ae-45a7-8ac7-fb57c308a1ed
📒 Files selected for processing (2)
src/main/java/org/checkerframework/specimin/JavaParserUtil.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.java
I tried this in my local environment, and I was also unable to repro. I also tried running Specimin many times to see if differences in output were causing these discrepancies, but all outputs were identical (ran 100 times on main, 50 times on this branch). |
|
Failure seems to be deterministic in CI. Debugging on a CI server is a pain, and this bug isn't one that we really care about (in the sense that it wasn't part of the ISSTA '25 eval), so I'm okay with cutting it out of the eval script altogether. See njit-jerse/specimin-evaluation#19. |
2 participants
This PR has the same goal as #411, and is a port of it to the new Specimin architecture after the refactor (#421).
Some of this code was generated with Claude (with the old PR #411, which I wrote by hand, as a reference), but I've reviewed it all and take responsibility for it.