Fix Specimin crashes relating to JavaParser bugs#452
Conversation
…relating to MethodAmbiguityException
|
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:
📝 WalkthroughWalkthroughThis PR adds a centralized 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
We should add a lint rule to the build for this, probably as a custom ErrorProne check (since we use ErrorProne for linting), because the odds that every future PR will remember this are low. |
kelloggm
left a comment
kelloggm
left a comment
There was a problem hiding this comment.
New test cases look reasonable to me.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/org/checkerframework/specimin/Resolver.java (2)
237-258: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid matching the exception text here.
ex.toString().contains("ReflectionMethodDeclaration")ties the fallback to JavaParser’s message format, so a message change can make the JDK-specific ambiguity path rethrow instead of returningnull.🤖 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/Resolver.java` around lines 237 - 258, The fallback in handleMethodAmbiguityException should not depend on ex.toString() containing "ReflectionMethodDeclaration", since that couples behavior to exception text. Update the ambiguity handling in Resolver.handleMethodAmbiguityException to use a more stable check for the JDK/reflection-specific case, and keep the MethodCallExpr fallback with JavaParserUtil.tryFindSingleCallableForNodeWithUnresolvableArguments for other ambiguities while still returning null for the reflection-only path.
90-109: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftGuard the fallback result kind
tryAlternativeResolutionForUnsolvableNode()can return aResolvedTypefrom the constraint-qualified-expression path, soResolver.resolve()may hand a type back to callers that are statically expectingResolvedValueDeclarationorResolvedMethodDeclaration. That makes the generic cast atResolver.java:101fail with aClassCastExceptionat assignment sites. Split the fallback by expected return kind or reject non-declaration results before casting.🤖 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/Resolver.java` around lines 90 - 109, The fallback in Resolver.resolve() can return a ResolvedType from tryAlternativeResolutionForUnsolvableNode(), which then gets blindly cast to T and can break callers expecting a declaration type. Update Resolver.resolve() and the fallback path to distinguish the expected result kind before casting, or split the fallback logic so declaration-resolving calls only return ResolvedValueDeclaration/ResolvedMethodDeclaration results. Use the existing symbols tryAlternativeResolutionForUnsolvableNode(), handleIllegalStateException(), and resolve() to locate the cast and add a guard or separate handling for non-declaration outcomes.src/main/java/org/checkerframework/specimin/StandardTypeRuleDependencyMap.java (1)
581-600: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFallback is skipped when the adjusted signature is unavailable. If
getSignatureFromResolvedMethodWithTypeVariablesMap(...)throws afteroriginal.getSignature()succeeds,signaturestays non-null and this branch drops the method beforeareAstAndResolvedMethodLikelyEqual(...)can run. Require both signatures before the exact-match check, or let the partial-failure case fall through to the simple-name comparison.🤖 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/StandardTypeRuleDependencyMap.java` around lines 581 - 600, The exact-signature branch in StandardTypeRuleDependencyMap should only run when both the original signature and the adjusted signature are available; currently a failure in JavaParserUtil.getSignatureFromResolvedMethodWithTypeVariablesMap(...) can leave signature non-null and skip the fallback path. Update the logic around Resolver.resolve(method), original.getSignature(), and the signature comparison so the exact-match check requires signatureOfDeclarationWithTypeParamsAdjusted as well, or otherwise allow the partial-failure case to fall through to areAstAndResolvedMethodLikelyEqual(...) and the simple-name comparison.src/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.java (1)
2936-2942: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap the remaining
resolve()method references inResolver.resolve(...)
NoJavaParserResolveonly flags direct calls, so thelhs::resolve/type::resolvesuppliers still bypass it. They can also throwMethodAmbiguityExceptionorUnsupportedOperationExceptionhere, andhandleLHSAndRHSRelationship()only catchesUnsolvedSymbolException | IllegalStateException.🤖 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/unsolved/UnsolvedSymbolGenerator.java` around lines 2936 - 2942, The remaining resolve suppliers in UnsolvedSymbolGenerator still use direct method references like lhs::resolve and type::resolve, which bypass NoJavaParserResolve and can throw unchecked exceptions. Update the affected supplier assignments in handleLHSAndRHSRelationship() and any similar resolve-related paths to call Resolver.resolve(...) explicitly instead of passing resolve method references, so all resolution goes through the same wrapper and exception handling stays consistent.
🤖 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 `@error-prone-checks/build.gradle`:
- Around line 9-11: The `javaparser-symbol-solver-core` dependency is currently
declared as a main `implementation` dependency even though only the test sources
appear to need the real JavaParser types. Update
`error-prone-checks/build.gradle` by moving that dependency from
`implementation` to `testImplementation` unless a main-source class such as
`NoJavaParserCalculateResolvedType` or `NoJavaParserResolve` truly compiles
against it.
In `@src/main/java/org/checkerframework/specimin/Slicer.java`:
- Around line 310-311: Guard the final-field resolution in Slicer so an
unresolved field type does not abort slicing; replace the direct
resolveGuaranteeNonNull call on fieldDeclarator with a safe Resolver.resolve
path (or catch UnsolvedSymbolException), and only perform the “already set”
check and add the field when the returned ResolvedFieldDeclaration is non-null.
---
Outside diff comments:
In `@src/main/java/org/checkerframework/specimin/Resolver.java`:
- Around line 237-258: The fallback in handleMethodAmbiguityException should not
depend on ex.toString() containing "ReflectionMethodDeclaration", since that
couples behavior to exception text. Update the ambiguity handling in
Resolver.handleMethodAmbiguityException to use a more stable check for the
JDK/reflection-specific case, and keep the MethodCallExpr fallback with
JavaParserUtil.tryFindSingleCallableForNodeWithUnresolvableArguments for other
ambiguities while still returning null for the reflection-only path.
- Around line 90-109: The fallback in Resolver.resolve() can return a
ResolvedType from tryAlternativeResolutionForUnsolvableNode(), which then gets
blindly cast to T and can break callers expecting a declaration type. Update
Resolver.resolve() and the fallback path to distinguish the expected result kind
before casting, or split the fallback logic so declaration-resolving calls only
return ResolvedValueDeclaration/ResolvedMethodDeclaration results. Use the
existing symbols tryAlternativeResolutionForUnsolvableNode(),
handleIllegalStateException(), and resolve() to locate the cast and add a guard
or separate handling for non-declaration outcomes.
In
`@src/main/java/org/checkerframework/specimin/StandardTypeRuleDependencyMap.java`:
- Around line 581-600: The exact-signature branch in
StandardTypeRuleDependencyMap should only run when both the original signature
and the adjusted signature are available; currently a failure in
JavaParserUtil.getSignatureFromResolvedMethodWithTypeVariablesMap(...) can leave
signature non-null and skip the fallback path. Update the logic around
Resolver.resolve(method), original.getSignature(), and the signature comparison
so the exact-match check requires signatureOfDeclarationWithTypeParamsAdjusted
as well, or otherwise allow the partial-failure case to fall through to
areAstAndResolvedMethodLikelyEqual(...) and the simple-name comparison.
In
`@src/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.java`:
- Around line 2936-2942: The remaining resolve suppliers in
UnsolvedSymbolGenerator still use direct method references like lhs::resolve and
type::resolve, which bypass NoJavaParserResolve and can throw unchecked
exceptions. Update the affected supplier assignments in
handleLHSAndRHSRelationship() and any similar resolve-related paths to call
Resolver.resolve(...) explicitly instead of passing resolve method references,
so all resolution goes through the same wrapper and exception handling stays
consistent.
🪄 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: d39bfcb0-f63f-4cc4-a04c-6c5a43479778
📒 Files selected for processing (13)
build.gradleerror-prone-checks/build.gradleerror-prone-checks/src/main/java/org/checkerframework/specimin/errorprone/NoJavaParserCalculateResolvedType.javaerror-prone-checks/src/main/java/org/checkerframework/specimin/errorprone/NoJavaParserResolve.javaerror-prone-checks/src/test/java/org/checkerframework/specimin/errorprone/NoJavaParserCalculateResolvedTypeTest.javaerror-prone-checks/src/test/java/org/checkerframework/specimin/errorprone/NoJavaParserResolveTest.javasettings.gradlesrc/main/java/org/checkerframework/specimin/JavaParserUtil.javasrc/main/java/org/checkerframework/specimin/Resolver.javasrc/main/java/org/checkerframework/specimin/Slicer.javasrc/main/java/org/checkerframework/specimin/StandardTypeRuleDependencyMap.javasrc/main/java/org/checkerframework/specimin/unsolved/FullyQualifiedNameGenerator.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.java
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 (2)
error-prone-checks/src/main/java/org/checkerframework/specimin/errorprone/NoJavaParserCalculateResolvedType.java (1)
25-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared boilerplate with
NoJavaParserResolve.This class and
NoJavaParserResolve.javashare near-identical structure: aRESOLVE_MATCHERfield, dualmatchMethodInvocation/matchMemberReferenceentry points delegating to a privatematchhelper, and abuildDescription(...).setMessage(...).build()pattern. Consider extracting a small abstract base checker (parameterized by the matcher and message) to avoid maintaining two copies of this wiring.♻️ Sketch of shared base class
public abstract class AbstractNoResolveCheck extends BugChecker implements MethodInvocationTreeMatcher, MemberReferenceTreeMatcher { protected abstract Matcher<ExpressionTree> matcher(); protected abstract String message(); `@Override` public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { return match(tree, state); } `@Override` public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { return match(tree, state); } private Description match(ExpressionTree tree, VisitorState state) { if (!matcher().matches(tree, state)) { return Description.NO_MATCH; } return buildDescription(tree).setMessage(message()).build(); } }🤖 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 `@error-prone-checks/src/main/java/org/checkerframework/specimin/errorprone/NoJavaParserCalculateResolvedType.java` around lines 25 - 53, This checker duplicates the same match-and-report boilerplate found in NoJavaParserResolve, so extract the shared wiring into a small abstract base checker and make NoJavaParserCalculateResolvedType extend it. Move the common RESOLVE_MATCHER-driven logic, the matchMethodInvocation/matchMemberReference delegation, and the buildDescription(...).setMessage(...).build() flow into the shared base, then keep only the specific matcher and diagnostic text in NoJavaParserCalculateResolvedType.src/main/java/org/checkerframework/specimin/JavaParserUtil.java (1)
1708-1737: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
getType()failures skip the unsolved-parameter mismatch checkWhen
resolvedParam.getType()throws,isParamTypeUnsolvedis set and the earlycontinuebypasses the latertypeInCall != nullrejection. That leaves the known-argument mismatch enforced only on theresolvedParam == nullpath. Move the unsolved-parameter check before thecontinue, or split the two unresolved cases so they behave the same.🤖 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 1708 - 1737, The parameter-matching logic in JavaParserUtil’s callable loop skips the unsolved-parameter mismatch check when ResolvedParameterDeclaration.getType() throws, because the early continue bypasses the later isParamTypeUnsolved handling. Update the logic around resolve(callable.getParameter(i)) and resolvedParam.getType() so the unresolved-parameter case is handled consistently before returning to the next iteration, ensuring both a null resolvedParam and a getType() failure reject non-null typeInCall the same way.
🤖 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
`@error-prone-checks/src/main/java/org/checkerframework/specimin/errorprone/NoJavaParserCalculateResolvedType.java`:
- Around line 25-53: This checker duplicates the same match-and-report
boilerplate found in NoJavaParserResolve, so extract the shared wiring into a
small abstract base checker and make NoJavaParserCalculateResolvedType extend
it. Move the common RESOLVE_MATCHER-driven logic, the
matchMethodInvocation/matchMemberReference delegation, and the
buildDescription(...).setMessage(...).build() flow into the shared base, then
keep only the specific matcher and diagnostic text in
NoJavaParserCalculateResolvedType.
In `@src/main/java/org/checkerframework/specimin/JavaParserUtil.java`:
- Around line 1708-1737: The parameter-matching logic in JavaParserUtil’s
callable loop skips the unsolved-parameter mismatch check when
ResolvedParameterDeclaration.getType() throws, because the early continue
bypasses the later isParamTypeUnsolved handling. Update the logic around
resolve(callable.getParameter(i)) and resolvedParam.getType() so the
unresolved-parameter case is handled consistently before returning to the next
iteration, ensuring both a null resolvedParam and a getType() failure reject
non-null typeInCall the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e474df2-2d33-47dd-9da9-61d55050652e
📒 Files selected for processing (7)
error-prone-checks/build.gradleerror-prone-checks/src/main/java/org/checkerframework/specimin/errorprone/NoJavaParserCalculateResolvedType.javaerror-prone-checks/src/main/java/org/checkerframework/specimin/errorprone/NoJavaParserResolve.javaerror-prone-checks/src/test/java/org/checkerframework/specimin/errorprone/NoJavaParserCalculateResolvedTypeTest.javaerror-prone-checks/src/test/java/org/checkerframework/specimin/errorprone/NoJavaParserResolveTest.javasrc/main/java/org/checkerframework/specimin/JavaParserUtil.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.java
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)
1391-1397: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
getSuperClass(constructorCall)before resolving the parent constructor.getSuperClass(...)throws when the enclosing class has no explicitextends, sosuper()in a class that implicitly extendsObjectwill now crash this path. Keep the old guard here or skip resolution when there is no superclass.🤖 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 1391 - 1397, The superclass lookup in the constructor-call handling path is not guarded, so `getSuperClass(constructorCall)` can throw for classes that implicitly extend `Object`. Update the logic in `JavaParserUtil` around `Resolver.resolve(...)` and `getTypeFromQualifiedName(...)` to first check whether an explicit superclass exists before attempting resolution, or skip parent resolution entirely when there is none. Keep the existing behavior for classes with an explicit `extends` while preventing `super()` in default-inheriting classes from crashing this 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.
Outside diff comments:
In `@src/main/java/org/checkerframework/specimin/JavaParserUtil.java`:
- Around line 1391-1397: The superclass lookup in the constructor-call handling
path is not guarded, so `getSuperClass(constructorCall)` can throw for classes
that implicitly extend `Object`. Update the logic in `JavaParserUtil` around
`Resolver.resolve(...)` and `getTypeFromQualifiedName(...)` to first check
whether an explicit superclass exists before attempting resolution, or skip
parent resolution entirely when there is none. Keep the existing behavior for
classes with an explicit `extends` while preventing `super()` in
default-inheriting classes from crashing this path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd5368fd-5940-42e8-9503-41db6a7ea6ec
📒 Files selected for processing (1)
src/main/java/org/checkerframework/specimin/JavaParserUtil.java
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/JavaParserUtil.java (1)
1711-1749: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not skip the mismatch after parameter-type resolution fails.
Line 1724 continues the parameter loop when
getType()throws, bypassing the mismatch check at Line 1746. This retains a callable despite a known argument type and an unresolved parameter type, potentially selecting the wrong overload.Proposed fix
- if (typeInCall == null || isParamTypeUnsolved || resolvedParameterType == null) { + if (typeInCall == null) { continue; } + if (isParamTypeUnsolved || resolvedParameterType == null) { + isAMatch = false; + 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 1711 - 1749, Ensure parameter matching rejects unresolved parameter types when a call argument type is known: in the parameter loop containing Resolver.resolve and getType, do not continue when isParamTypeUnsolved and typeInCall is non-null; allow execution to reach the existing mismatch check that sets isAMatch to false and breaks. Preserve skipping only when the argument type is also unavailable.
🤖 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 1736-1740: Update the constraint handling in JavaParserUtil’s
overload-resolution logic: when typeInCall.isConstraint() and
resolvedParameterType.isReference(), inspect
typeInCall.asConstraintType().getBound() and only continue accepting the
candidate when the bound is absent or compatible with resolvedParameterType;
reject incompatible concrete bounds. Add a regression test covering an
incompatible pair of reference overloads.
---
Outside diff comments:
In `@src/main/java/org/checkerframework/specimin/JavaParserUtil.java`:
- Around line 1711-1749: Ensure parameter matching rejects unresolved parameter
types when a call argument type is known: in the parameter loop containing
Resolver.resolve and getType, do not continue when isParamTypeUnsolved and
typeInCall is non-null; allow execution to reach the existing mismatch check
that sets isAMatch to false and breaks. Preserve skipping only when the argument
type is also unavailable.
🪄 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: 88014612-5d14-4f74-8947-9286504ee08f
📒 Files selected for processing (1)
src/main/java/org/checkerframework/specimin/JavaParserUtil.java
This adds support for Java's `permits` keyword, noticed when I was working on the false positives project. Specifically: 1. `permits` clauses are not removed, and the classes referenced within (if solvable) are included in the slice, similar to how classes in `extends`/`implements` are currently handled 2. Modifies unsolved type generation for types found in `permits` clauses. By default, they generate with two alternates--`final` and `non-sealed`--with best effort preferring `final` unless evidence is found against `final`; it then falls back to `non-sealed`. `sealed` is currently not in this list because this requires finding a valid `permits` clause for that unsolved symbol, which seems difficult to do. **NOTE:** Merge this PR after #452, because this branch is based off the branch in that PR so I wouldn't have to deal with any merge conflicts later on. **_The last three commits (db29e06, 9ab9cb9, 9e31b9d) are the only ones with changes relevant to this PR,_** so only those should be reviewed in this PR.
2 participants
Fixes some of the crashes in #421. Namely, these two cases:
Also, I moved resolution logic into a single class (
Resolver#resolve(Resolvable)) which handles these cases (alongside some other cases that JavaParser cannot handle), so future work in Specimin should use that method instead of JavaParser'sResolvable#resolve().