Add support for the permits keyword#453
Conversation
…relating to MethodAmbiguityException
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change migrates dependency discovery and symbol inference to Suggested reviewers: 🚥 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: 7
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/unsolved/FullyQualifiedNameGenerator.java (1)
1547-1576: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale javadoc: method no longer throws
UnsolvedSymbolException.The javadoc still states this method "will also throw an UnsolvedSymbolException if .resolve() fails," but the implementation now calls
Resolver.resolve((Resolvable<?>) expr), which returnsnullinstead of throwing on failure (already handled by the subsequent null check/fallbacks at Lines 1564-1576). The comment should be updated to avoid misleading future callers.📝 Proposed fix
/** * Given an expression that can be resolved (but calculateResolvedType() fails), return the best - * shot at its type based on a resolved declaration. May return null if a type cannot be found - * from the resolved declaration. This method will also throw an UnsolvedSymbolException if - * .resolve() fails. + * shot at its type based on a resolved declaration. May return null if a type cannot be found + * from the resolved declaration, including when the expression itself cannot be resolved. * * `@param` expr A resolvable expression * `@return` A set of FQNs, or null if unfound */🤖 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/FullyQualifiedNameGenerator.java` around lines 1547 - 1576, Update the Javadoc for getFQNsForTypeOfSolvableExpression to remove the statement that the method throws UnsolvedSymbolException when resolution fails, since Resolver.resolve returns null and the method handles that case through its fallback logic.
🤖 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`:
- Line 388: In the logic containing Resolver.calculateResolvedType(expr),
resolve the member expression’s scope/qualifier instead of expr itself; use the
resulting declaring-type candidate when comparing against scope so expressions
like Foo.member identify Foo correctly. Preserve existing handling for
non-member expressions.
In `@src/main/java/org/checkerframework/specimin/Resolver.java`:
- Around line 237-258: Replace the fragile
ex.toString().contains("ReflectionMethodDeclaration") check in
handleMethodAmbiguityException with JavaParser 3.28.2’s structured
MethodAmbiguityException candidate API, if available, inspecting the ambiguous
declarations for reflection-based candidates. Preserve the existing fallback
behavior: resolve a single callable for unresolved MethodCallExpr arguments,
rethrow other ambiguities, and return null only for reflection-based ambiguity;
if no structured API exists, document that limitation and use the safest
available typed inspection.
- Around line 121-160: Update resolveGuaranteeNonNull to catch
UnsolvedSymbolException and invoke the same
tryAlternativeResolutionForUnsolvableNode fallback used by resolve(). Preserve
the existing behavior for genuinely unresolved nodes by rethrowing the original
exception when alternative resolution returns null, then assign the recovered
value to result and retain the final non-null check.
In
`@src/main/java/org/checkerframework/specimin/StandardTypeRuleDependencyMap.java`:
- Around line 584-603: Update the signature-selection logic after resolving the
method so exact matching is attempted only when both `signature` and
`signatureOfDeclarationWithTypeParamsAdjusted` are non-null; otherwise, enter
the existing simple-name fallback branch. Ensure an `UnsolvedSymbolException`
from either signature computation cannot prevent fallback and cause an
overridden method to be omitted.
In
`@src/main/java/org/checkerframework/specimin/unsolved/UnsolvedClassOrInterface.java`:
- Around line 253-265: Prevent invalid final interface output in
UnsolvedClassOrInterface.toString(): when sealedness is Sealedness.FINAL and
typeOfType is INTERFACE, do not emit the final modifier; add a defensive guard
consistent with the existing Sealedness.SEALED handling. Preserve the primary
sealedness behavior elsewhere, particularly
UnsolvedSymbolGenerator.addInformation, while ensuring generated interface
declarations never contain final.
In
`@src/main/java/org/checkerframework/specimin/unsolved/UnsolvedClassOrInterfaceAlternates.java`:
- Around line 194-220: Update ensureSuperType, addSuperType, forceSuperClass,
and forceSuperInterface so locking the inferred superclass does not prevent
later interface relationships from accumulating. Preserve the lock against
conflicting extends/superclass changes, but allow compatible implements
additions after the sealed-permits pass; avoid clearing valid interface
relationships when establishing the superclass.
In
`@src/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.java`:
- Around line 2342-2357: In the permitted-type handling logic, move
syntheticType.addSealedness(Sealedness.FINAL) inside the existing
!decl.isInterface() branch so it is only applied when the permitted declaration
is a class; keep NON_SEALED handling outside that branch as appropriate. Update
the logic around findExistingAndUpdateFQNs and
UnsolvedClassOrInterfaceAlternates to prevent interfaces from rendering as
final.
---
Outside diff comments:
In
`@src/main/java/org/checkerframework/specimin/unsolved/FullyQualifiedNameGenerator.java`:
- Around line 1547-1576: Update the Javadoc for
getFQNsForTypeOfSolvableExpression to remove the statement that the method
throws UnsolvedSymbolException when resolution fails, since Resolver.resolve
returns null and the method handles that case through its fallback logic.
🪄 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: 1aaf0627-9b7e-46d8-bf12-cd870d28aaee
📒 Files selected for processing (33)
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/SpeciminRunner.javasrc/main/java/org/checkerframework/specimin/StandardTypeRuleDependencyMap.javasrc/main/java/org/checkerframework/specimin/unsolved/FullyQualifiedNameGenerator.javasrc/main/java/org/checkerframework/specimin/unsolved/Sealedness.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedClassOrInterface.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedClassOrInterfaceAlternates.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.javasrc/test/java/org/checkerframework/specimin/AnnotationMethodUsageTest.javasrc/test/java/org/checkerframework/specimin/PermitsTest.javasrc/test/java/org/checkerframework/specimin/PrimitiveArrayMethodOverloadsTest.javasrc/test/java/org/checkerframework/specimin/UnsolvedPermitsTest.javasrc/test/resources/annotationmethodusage/expected/com/example/Anno.javasrc/test/resources/annotationmethodusage/expected/com/example/Simple.javasrc/test/resources/annotationmethodusage/input/com/example/Anno.javasrc/test/resources/annotationmethodusage/input/com/example/Simple.javasrc/test/resources/permits/expected/com/example/Foo.javasrc/test/resources/permits/input/com/example/Foo.javasrc/test/resources/primitivearraymethodoverloads/expected/com/example/Simple.javasrc/test/resources/primitivearraymethodoverloads/input/com/example/Simple.javasrc/test/resources/unsolvedpermits/expected/com/example/Bar.javasrc/test/resources/unsolvedpermits/expected/com/example/Baz.javasrc/test/resources/unsolvedpermits/expected/com/example/Foo.javasrc/test/resources/unsolvedpermits/input/com/example/Foo.java
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 (1)
src/main/java/org/checkerframework/specimin/JavaParserUtil.java (1)
1691-1756: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAsymmetric handling of unresolved parameter types can silently miss real mismatches.
In the per-parameter loop, when
resolvedParam == nullfrom the start, control falls through to the final checkif (isParamTypeUnsolved && typeInCall != null) { isAMatch = false; break; }, correctly treating "known call-arg type vs. unknown param type" as a non-match.However, when
resolvedParam != nullbutresolvedParam.getType()throwsUnsolvedSymbolException(soisParamTypeUnsolvedbecomestrueonly inside theif (resolvedParam != null)block), thecontinue;atif (typeInCall == null || isParamTypeUnsolved || resolvedParameterType == null) { continue; }skips straight to the next parameter — bypassing that same final check entirely for this iteration. The two logically-equivalent "parameter type could not be determined" scenarios are handled inconsistently: one marksisAMatch = false, the other silently treats it as compatible. This can cause an unrelated/incorrect callable to be reported as a match intryFindSingleCallableForNodeWithUnresolvableArguments, which is used pervasively for unresolvable-argument resolution.🐛 Proposed fix
for (int i = 0; i < callable.getParameters().size(); i++) { ResolvedType typeInCall = parameterTypes.get(i); ResolvedParameterDeclaration resolvedParam = Resolver.resolve(callable.getParameter(i)); boolean isParamTypeUnsolved = resolvedParam == null; + ResolvedType resolvedParameterType = null; if (resolvedParam != null) { - ResolvedType resolvedParameterType = null; try { resolvedParameterType = resolvedParam.getType(); } catch (UnsolvedSymbolException ex) { isParamTypeUnsolved = true; // getType() may throw an UnsolvedSymbolException } - - if (typeInCall == null || isParamTypeUnsolved || resolvedParameterType == null) { - continue; - } - - if (!resolvedParameterType.isAssignableBy(typeInCall)) { - // If either is a type variable and the other is a reference type, it is likely valid - // Note that isAssignableBy will return false in those cases - if (typeInCall.isTypeVariable() && resolvedParameterType.isReference()) { - continue; - } - if (resolvedParameterType.isTypeVariable() && typeInCall.isReference()) { - continue; - } - // JavaParser can't handle constraint types well. This isn't perfect (i.e., doesn't - // properly match bounds), but it should work for most cases. - if (typeInCall.isConstraint() && resolvedParameterType.isReference()) { - continue; - } - isAMatch = false; - break; - } } - if (isParamTypeUnsolved && typeInCall != null) { + if (!isParamTypeUnsolved && typeInCall != null && resolvedParameterType != null) { + if (!resolvedParameterType.isAssignableBy(typeInCall)) { + // If either is a type variable and the other is a reference type, it is likely valid + // Note that isAssignableBy will return false in those cases + boolean likelyValidMismatch = + (typeInCall.isTypeVariable() && resolvedParameterType.isReference()) + || (resolvedParameterType.isTypeVariable() && typeInCall.isReference()) + // JavaParser can't handle constraint types well. This isn't perfect (i.e., + // doesn't properly match bounds), but it should work for most cases. + || (typeInCall.isConstraint() && resolvedParameterType.isReference()); + + if (!likelyValidMismatch) { + isAMatch = false; + break; + } + } + } else if (isParamTypeUnsolved && typeInCall != 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 1691 - 1756, Update addAllMatchingCallablesToListImpl so a parameter whose resolvedParam.getType() throws UnsolvedSymbolException follows the same mismatch behavior as an initially null resolvedParam: when typeInCall is non-null, set isAMatch to false and stop evaluating the callable instead of continuing past the final unresolved-type check. Preserve matching when typeInCall is null.
🤖 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/unsolved/UnsolvedClassOrInterfaceAlternates.java`:
- Around line 201-219: Update ensureSuperClass in
UnsolvedClassOrInterfaceAlternates to remove existing EXTENDS relationship keys
via superTypeRelationships.keySet().removeAll(toRemove), ensuring the lock
leaves only the forced superclass. Then update addSuperType so UNKNOWN
superclass relationships received after superClassAlreadyLocked are ignored or
safely classified and cannot be emitted as possible extends clauses during
alternate generation.
- Around line 356-385: Update addSealedness in
UnsolvedClassOrInterfaceAlternates so Sealedness.FINAL is preserved when an
alternate has UNKNOWN type, allowing the later split to retain a class-final
candidate. Continue removing or skipping FINAL for known interface and other
invalid alternatives, while leaving existing class handling unchanged.
---
Outside diff comments:
In `@src/main/java/org/checkerframework/specimin/JavaParserUtil.java`:
- Around line 1691-1756: Update addAllMatchingCallablesToListImpl so a parameter
whose resolvedParam.getType() throws UnsolvedSymbolException follows the same
mismatch behavior as an initially null resolvedParam: when typeInCall is
non-null, set isAMatch to false and stop evaluating the callable instead of
continuing past the final unresolved-type check. Preserve matching when
typeInCall is null.
🪄 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: b3403d4f-effc-4d2d-8dc4-f3320eda39b9
📒 Files selected for processing (16)
src/main/java/org/checkerframework/specimin/JavaParserUtil.javasrc/main/java/org/checkerframework/specimin/Resolver.javasrc/main/java/org/checkerframework/specimin/StandardTypeRuleDependencyMap.javasrc/main/java/org/checkerframework/specimin/unsolved/FullyQualifiedNameGenerator.javasrc/main/java/org/checkerframework/specimin/unsolved/Sealedness.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedClassOrInterface.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedClassOrInterfaceAlternates.javasrc/main/java/org/checkerframework/specimin/unsolved/UnsolvedSymbolGenerator.javasrc/test/java/org/checkerframework/specimin/PermitsTest.javasrc/test/java/org/checkerframework/specimin/UnsolvedPermitsTest.javasrc/test/resources/permits/expected/com/example/Foo.javasrc/test/resources/permits/input/com/example/Foo.javasrc/test/resources/unsolvedpermits/expected/com/example/Bar.javasrc/test/resources/unsolvedpermits/expected/com/example/Baz.javasrc/test/resources/unsolvedpermits/expected/com/example/Foo.javasrc/test/resources/unsolvedpermits/input/com/example/Foo.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/unsolved/UnsolvedClassOrInterfaceAlternates.java (1)
279-285: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUNKNOWN relationships still bypass the superclass lock.
The
removebug from the prior review is fixed (lines 215–217 now iterate), and EXTENDS insertion is correctly guarded by!superClassAlreadyLockedon line 279. However, whencommonTypeisnull(e.g., all super types areSolvedMemberType, or mixed unsolved types with differingUnsolvedClassOrInterfaceType), theelse ifat line 283 still inserts anUNKNOWNrelationship regardless of the lock. IncreateAlternatesBasedOnSuperTypeRelationshipsForClass(lines 645–656),UNKNOWNentries are processed as potentialextendsclauses, which could yield alternates extending a second class alongside the locked superclass — invalid Java.This was the second half of the concern raised in the prior review at lines 201–221, marked as addressed but only the
removeaspect was resolved.🔧 Proposed fix: treat UNKNOWN as IMPLEMENTS when the superclass is locked
if (commonType == UnsolvedClassOrInterfaceType.CLASS && !superClassAlreadyLocked) { superTypeRelationships.put(sanitizedSuperTypes, SuperTypeRelationship.EXTENDS); } else if (commonType == UnsolvedClassOrInterfaceType.INTERFACE) { superTypeRelationships.put(sanitizedSuperTypes, SuperTypeRelationship.IMPLEMENTS); - } else if (superTypeRelationships.get(superTypes) == null) { + } else if (superClassAlreadyLocked) { + // Superclass is locked; treat unknown relationships as implements to avoid + // creating invalid alternates with multiple superclasses. + superTypeRelationships.put(sanitizedSuperTypes, SuperTypeRelationship.IMPLEMENTS); + } else if (superTypeRelationships.get(superTypes) == null) { superTypeRelationships.put(sanitizedSuperTypes, SuperTypeRelationship.UNKNOWN); }🤖 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/UnsolvedClassOrInterfaceAlternates.java` around lines 279 - 285, Update the UNKNOWN relationship branch in the supertype relationship insertion logic to respect superClassAlreadyLocked: when the superclass is locked, record UNKNOWN supertypes as IMPLEMENTS instead of UNKNOWN; otherwise preserve the existing UNKNOWN behavior. Ensure createAlternatesBasedOnSuperTypeRelationshipsForClass cannot interpret a locked-class UNKNOWN entry as a second extends clause.
🤖 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/unsolved/UnsolvedClassOrInterfaceAlternates.java`:
- Around line 279-285: Update the UNKNOWN relationship branch in the supertype
relationship insertion logic to respect superClassAlreadyLocked: when the
superclass is locked, record UNKNOWN supertypes as IMPLEMENTS instead of
UNKNOWN; otherwise preserve the existing UNKNOWN behavior. Ensure
createAlternatesBasedOnSuperTypeRelationshipsForClass cannot interpret a
locked-class UNKNOWN entry as a second extends clause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 311ae711-19aa-461c-988b-07c82a0c2122
📒 Files selected for processing (1)
src/main/java/org/checkerframework/specimin/unsolved/UnsolvedClassOrInterfaceAlternates.java
2 participants
This adds support for Java's
permitskeyword, noticed when I was working on the false positives project.Specifically:
permitsclauses are not removed, and the classes referenced within (if solvable) are included in the slice, similar to how classes inextends/implementsare currently handledpermitsclauses. By default, they generate with two alternates--finalandnon-sealed--with best effort preferringfinalunless evidence is found againstfinal; it then falls back tonon-sealed.sealedis currently not in this list because this requires finding a validpermitsclause 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.