New overload canonicalAnnotation(AnnotationMirror, TypeMirror)#7685
New overload canonicalAnnotation(AnnotationMirror, TypeMirror)#7685mernst wants to merge 12 commits intotypetools:masterfrom
canonicalAnnotation(AnnotationMirror, TypeMirror)#7685Conversation
📝 WalkthroughWalkthroughAdds a public overload Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java`:
- Around line 173-175: getExprType's declared non-null contract is incorrect:
the method can accept and return null but its signature and local variables
(exprType) are non-nullable; change getExprType's signature to accept and return
`@Nullable` (or the project’s nullable annotation), mark the local variables named
exprType as `@Nullable` where getExprType is called (e.g., in the calls that pass
methodDecl and then call getQualifierEnforcedByContractAnnotation), and update
callers (including the other occurrences around the uses of contractAnno/anno)
to null-check exprType before using it or propagate null safely; also add the
nullable import if missing and adjust logic in
getQualifierEnforcedByContractAnnotation or its callers to handle a nullable
exprType.
- Around line 128-134: The constructor special-case that sets methodDecl to null
prevents type-aware canonicalization (getExprType(..., null) yields null and
canonicalAnnotation(a, null) falls back), so remove the branch and always obtain
the MethodTree for the executableElement; replace the conditional that sets
methodDecl to null for "<init>" with a single assignment methodDecl =
(MethodTree) factory.declarationFromElement(executableElement) so getExprType
and canonicalAnnotation receive the actual methodDecl/exprType and constructors
get the same type-dependent canonicalization as methods.
🪄 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: cdaeea18-1a2a-41fb-b505-a2b26ad047e9
📒 Files selected for processing (5)
checker/src/main/java/org/checkerframework/checker/units/UnitsAnnotationClassLoader.javadocs/CHANGELOG.mdframework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.javaframework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.javaframework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/CHANGELOG.md`:
- Around line 6-17: The changelog for Version 4.1.1 contains empty sections "###
User-visible changes" and "### Closed issues"; update the Version 4.1.1 entry in
docs/CHANGELOG.md to either remove those empty headers or add explicit
placeholder text (e.g., "None." or "No closed issues.") so readers don't assume
a formatting error, and keep the existing note about the new overload
canonicalAnnotation(AnnotationMirror, TypeMirror) in AnnotatedTypeFactory
unchanged.
- Around line 12-15: Update the changelog entry for AnnotatedTypeFactory to
match the API precisely: change the overload signature from
canonicalAnnotation(AnnotationMirror, TypeMirror) to
canonicalAnnotation(AnnotationMirror, `@Nullable` TypeMirror tm) (including the
`@Nullable` annotation and optional parameter name tm) so readers understand the
nullability contract of the second parameter and avoid ambiguity about override
behavior.
🪄 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: 04cd12b3-b1dc-4ede-891d-90721ddcce66
📒 Files selected for processing (1)
docs/CHANGELOG.md
There was a problem hiding this comment.
♻️ Duplicate comments (2)
framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java (2)
173-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
getExprTypenullability contract is still violated at both call sites.
getExprTypereturns@Nullable TypeMirror, but Line 173 and Line 239 assign it to non-nullTypeMirror. This is inconsistent with the method contract and can fail nullness checking.Suggested fix
- TypeMirror exprType = getExprType(expr, methodDecl); + `@Nullable` TypeMirror exprType = getExprType(expr, methodDecl); @@ - TypeMirror exprType = getExprType(expr, methodDecl); + `@Nullable` TypeMirror exprType = getExprType(expr, methodDecl);Also applies to: 239-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java` around lines 173 - 175, getExprType returns `@Nullable` TypeMirror but its result is assigned to a non-null TypeMirror and passed into getQualifierEnforcedByContractAnnotation; change the handling so you declare the local as `@Nullable` TypeMirror exprType = getExprType(expr, methodDecl) (and similarly at the second call site), then add a null check (e.g., if (exprType == null) { /* skip/continue/return as appropriate for the surrounding logic */ }) before invoking getQualifierEnforcedByContractAnnotation(contractAnno, anno, exprType) so you never pass a null TypeMirror to that method.
128-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstructors currently bypass type-aware canonicalization.
Line 129–132 forces
methodDecltonullfor constructors, which makesgetExprTypereturnnulland prevents constructor contracts from using type-aware canonicalization.Suggested fix
- MethodTree methodDecl; - if (executableElement.getSimpleName().toString().equals("<init>")) { - // It's a constructor, not a method. - methodDecl = null; - } else { - methodDecl = (MethodTree) factory.declarationFromElement(executableElement); - } + `@Nullable` MethodTree methodDecl = + (MethodTree) factory.declarationFromElement(executableElement);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java` around lines 128 - 134, The code is forcing methodDecl to null for constructors which prevents type-aware canonicalization; instead, stop special-casing "<init>" and obtain the declaration via factory.declarationFromElement(executableElement), assigning it to methodDecl only if the returned Tree is a MethodTree (or otherwise leave it null), so constructors that have a MethodTree (or appropriate tree) can be used by getExprType; update the block around executableElement/methodDecl in ContractsFromMethod to perform a safe instanceof check on the declaration rather than hardcoding null for constructors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java`:
- Around line 173-175: getExprType returns `@Nullable` TypeMirror but its result
is assigned to a non-null TypeMirror and passed into
getQualifierEnforcedByContractAnnotation; change the handling so you declare the
local as `@Nullable` TypeMirror exprType = getExprType(expr, methodDecl) (and
similarly at the second call site), then add a null check (e.g., if (exprType ==
null) { /* skip/continue/return as appropriate for the surrounding logic */ })
before invoking getQualifierEnforcedByContractAnnotation(contractAnno, anno,
exprType) so you never pass a null TypeMirror to that method.
- Around line 128-134: The code is forcing methodDecl to null for constructors
which prevents type-aware canonicalization; instead, stop special-casing
"<init>" and obtain the declaration via
factory.declarationFromElement(executableElement), assigning it to methodDecl
only if the returned Tree is a MethodTree (or otherwise leave it null), so
constructors that have a MethodTree (or appropriate tree) can be used by
getExprType; update the block around executableElement/methodDecl in
ContractsFromMethod to perform a safe instanceof check on the declaration rather
than hardcoding null for constructors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fb6a67b-d7d9-4c5a-b54f-19edb63caf52
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java
| return null; | ||
| } else { | ||
| try { | ||
| JavaExpression jExpr = |
There was a problem hiding this comment.
This seems wrong. Contracts are read both at call sites and declarations, but methodDecl is only non-null at the method declaration. You could instead canonicalize when annotation is viewpoint adapted. (see call sites of Contract#viewpointAdaptDependentTypeAnnotation.) This is also where the expression in the contract is already parsed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java`:
- Around line 128-134: Add the missing import for
javax.lang.model.element.ElementKind to resolve the reference to
ElementKind.CONSTRUCTOR used in ContractsFromMethod where
executableElement.getKind() is checked; locate the class ContractsFromMethod and
add the import so the use of ElementKind, MethodTree,
factory.declarationFromElement(executableElement) and the variable methodDecl
compile correctly.
🪄 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: f649e7b9-ab01-489e-9695-69ec4d8f291d
📒 Files selected for processing (3)
docs/CHANGELOG.mdframework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.javaframework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.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)
framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java (1)
296-313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
@Nullable Map<String, String> argumentRenamingis misleading and risks NPE if ever truly null.The parameter is annotated
@Nullable, but at line 306argumentRenaming.isEmpty()is invoked unguarded — whenargumentAnno != null && argumentRenaming == null, this NPEs. The only caller (lines 270-275) always passes a non-null map (usingCollections.emptyMap()for the null-argumentAnnocase), so either drop the@Nullablehere (and update the Javadoc on lines 290-291) or add an explicit null guard. Since the method is private, the simpler fix is to make the parameter non-null.Suggested fix
private `@Nullable` AnnotationMirror getQualifierEnforcedByContractAnnotation( AnnotationMirror contractAnno, `@Nullable` AnnotationMirror argumentAnno, - `@Nullable` Map<String, String> argumentRenaming, + Map<String, String> argumentRenaming, `@Nullable` TypeMirror type) {…and adjust the Javadoc:
- * `@param` argumentRenaming renaming of argument names, which maps from names in {`@code` - * argumentAnno} to names used in the returned annotation, or {`@code` null} + * `@param` argumentRenaming renaming of argument names, which maps from names in {`@code` + * argumentAnno} to names used in the returned annotation; empty if {`@code` argumentAnno} is + * {`@code` null}🤖 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 `@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java` around lines 296 - 313, The parameter argumentRenaming of getQualifierEnforcedByContractAnnotation is marked `@Nullable` but is dereferenced (argumentRenaming.isEmpty()), risking NPE; change the signature to make argumentRenaming non-null (Map<String, String> argumentRenaming) and remove the `@Nullable` annotation, and update the method Javadoc to state the map is non-null (callers already pass Collections.emptyMap()), leaving the current isEmpty() logic intact; no runtime null-guard needed, but ensure any other callers (if any) are updated to pass a non-null map.
🤖 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
`@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java`:
- Around line 197-209: getExprType currently swallows
JavaExpressionParseException and returns null, which loses parse diagnostics and
causes contracts to be dropped silently; update getExprType (and its Javadoc) so
it documents that it returns null when methodDecl is null or on parse failure,
and change the implementation to surface the failure instead of silently
discarding it (either by returning an Optional with an attached diagnostic,
logging the parse error via the checker/logger, or rethrowing a checked
exception) and add a simple caching layer keyed by (expr, methodDecl) to avoid
re-parsing the same expression during viewpoint adaptation; refer to method
getExprType, class JavaExpression/StringToJavaExpression, and the
JavaExpressionParseException type when making these changes.
---
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java`:
- Around line 296-313: The parameter argumentRenaming of
getQualifierEnforcedByContractAnnotation is marked `@Nullable` but is dereferenced
(argumentRenaming.isEmpty()), risking NPE; change the signature to make
argumentRenaming non-null (Map<String, String> argumentRenaming) and remove the
`@Nullable` annotation, and update the method Javadoc to state the map is non-null
(callers already pass Collections.emptyMap()), leaving the current isEmpty()
logic intact; no runtime null-guard needed, but ensure any other callers (if
any) are updated to pass a non-null map.
🪄 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: f54c1e38-639b-4409-96c8-1637fa2bacc2
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java
| private @Nullable TypeMirror getExprType(String expr, @Nullable MethodTree methodDecl) { | ||
| if (methodDecl == null) { | ||
| return null; | ||
| } else { | ||
| try { | ||
| JavaExpression jExpr = | ||
| StringToJavaExpression.atMethodBody(expr, methodDecl, factory.getChecker()); | ||
| return jExpr.getType(); | ||
| } catch (JavaExpressionParseException e) { | ||
| return null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
getExprType silently swallows parse failures and duplicates work.
JavaExpressionParseException is caught and discarded, returning null. Combined with the if (enforcedQualifier == null) continue; guards added at lines 177-179 and 243-245, a parse failure now silently drops the entire contract instead of surfacing or being handled where contracts are actually adapted. The Javadoc should also state that null is returned on parse failure or when methodDecl is null. As an aside, this parsing duplicates work performed later during viewpoint adaptation; if you keep this approach, consider at least caching/diagnosing the failure rather than dropping contracts wordlessly.
Suggested Javadoc tightening
/**
- * Returns the type of the given expression.
+ * Returns the type of the given expression, or {`@code` null} if {`@code` methodDecl} is {`@code`
+ * null} or the expression cannot be parsed.
*
* `@param` expr an expression
* `@param` methodDecl the method at which to interpret the expression
- * `@return` the type of the given expression
+ * `@return` the type of the given expression, or {`@code` null} if it cannot be determined
*/🤖 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
`@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java`
around lines 197 - 209, getExprType currently swallows
JavaExpressionParseException and returns null, which loses parse diagnostics and
causes contracts to be dropped silently; update getExprType (and its Javadoc) so
it documents that it returns null when methodDecl is null or on parse failure,
and change the implementation to surface the failure instead of silently
discarding it (either by returning an Optional with an attached diagnostic,
logging the parse error via the checker/logger, or rethrowing a checked
exception) and add a simple caching layer keyed by (expr, methodDecl) to avoid
re-parsing the same expression during viewpoint adaptation; refer to method
getExprType, class JavaExpression/StringToJavaExpression, and the
JavaExpressionParseException type when making these changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java`:
- Around line 129-135: Declare methodDecl as `@Nullable` MethodTree to reflect
that it may be null and the cast from factory.declarationFromElement(...) is
nullable; modify the local declaration of methodDecl (currently "MethodTree
methodDecl") to "@Nullable MethodTree methodDecl" and preserve the existing null
assignment for constructors and the cast from
factory.declarationFromElement(executableElement) so the nullness checker is
satisfied.
🪄 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: e73db1a4-a391-43a1-81df-915e16c9d421
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java
| MethodTree methodDecl; | ||
| if (executableElement.getKind() == ElementKind.CONSTRUCTOR) { | ||
| // It's a constructor, not a method. | ||
| methodDecl = null; | ||
| } else { | ||
| methodDecl = (MethodTree) factory.declarationFromElement(executableElement); | ||
| } |
There was a problem hiding this comment.
methodDecl must be declared @Nullable MethodTree to satisfy the framework's own nullness checker.
Two nullness violations exist:
- Line 132 assigns
nullto aMethodTreelocal (non-null by default in CF's own type system). - Line 134 casts the return of
factory.declarationFromElement(...), which is@Nullable Tree, so the result is@Nullable MethodTree.
🛡️ Proposed fix
- MethodTree methodDecl;
+ `@Nullable` MethodTree methodDecl;
if (executableElement.getKind() == ElementKind.CONSTRUCTOR) {🤖 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
`@framework/src/main/java/org/checkerframework/framework/util/ContractsFromMethod.java`
around lines 129 - 135, Declare methodDecl as `@Nullable` MethodTree to reflect
that it may be null and the cast from factory.declarationFromElement(...) is
nullable; modify the local declaration of methodDecl (currently "MethodTree
methodDecl") to "@Nullable MethodTree methodDecl" and preserve the existing null
assignment for constructors and the cast from
factory.declarationFromElement(executableElement) so the nullness checker is
satisfied.
2 participants
No description provided.