Fix problem with type var in a method reference#7729
Open
smillst wants to merge 25 commits into
Open
Conversation
Contributor
|
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 (1)
📝 WalkthroughWalkthroughThis PR replaces the old InvocationType with a new AbstractExecutableType abstraction and adds AbstractInvocationType and CompileTimeDeclarationType. It updates InferenceFactory to produce these types, threads them through InvocationTypeInference and constraint-reduction (createB2/createB3/createC), renames several parameters for consistency, and adds a regression test Issue7680. Javadoc and minor doc fixes are included. 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}
Contributor
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/typeinference8/types/MethodType.java`:
- Around line 43-62: The method getReturnType(Theta map) computes a
branch-specific Java returnType for diamond/new-class cases but then still
passes the stale returnTypeJava into the result constructors; update the
return-site to use the computed local returnType instead of returnTypeJava so
ProperType(...) and InferenceType.create(...) receive the correct underlying
Java type (modify the two return expressions in getReturnType to pass
returnType, keeping annotatedReturnType, map, and context unchanged).
🪄 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: b7686254-7dea-4e95-9208-6c6857a0006c
📒 Files selected for processing (2)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/MethodReferenceType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/MethodType.java
Contributor
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 (4)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceExecutableType.java (1)
98-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread
qualifierVarsinto thrown-type construction.
getThrownTypescurrently builds types with the overload that does not reuse this executable’s precomputedqualifierVars. That gives thrown types a different@Poly*variable mapping than the parameters/return type, which can make inference inconsistent for the same executable.♻️ Proposed fix
- thrown.add(InferenceType.create(t, iter.next(), map, context)); + thrown.add(InferenceType.create(t, iter.next(), map, qualifierVars, context, false));🤖 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/typeinference8/types/InferenceExecutableType.java` around lines 98 - 103, getThrownTypes builds thrown types using InferenceType.create(t, iter.next(), map, context) which ignores this executable's precomputed qualifierVars and yields inconsistent `@Poly`* mappings; update getThrownTypes to pass the executable's qualifierVars into the thrown-type construction (i.e., call the InferenceType.create overload that accepts qualifierVars, supplying the stored qualifierVars from this class) so thrown types share the same qualifier variable mapping as the parameters/return.framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceMethodType.java (1)
71-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve executable-level qualifier vars on the return type.
This path bypasses the precomputed
qualifierVars, so a polymorphic qualifier on the return can get a fresh variable instead of the one shared with the parameters. That changes inference behavior for@Poly*methods.♻️ Proposed fix
- if (map == null) { - return new ProperType(annotatedReturnType, returnType, context); - } - return InferenceType.create(annotatedReturnType, returnType, map, context); + return InferenceType.create( + annotatedReturnType, returnType, map, qualifierVars, context, false);🤖 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/typeinference8/types/InferenceMethodType.java` around lines 71 - 74, When map == null the code returns a new ProperType which bypasses this object's precomputed qualifierVars and causes polymorphic qualifiers on the return to get fresh variables; instead, preserve the existing qualifierVars by returning an InferenceType constructed with them. Replace the early return in InferenceMethodType (the branch that currently does `return new ProperType(annotatedReturnType, returnType, context)`) to call InferenceType.create(annotatedReturnType, returnType, qualifierVars, context) so the return type shares the same qualifierVars as the parameters.framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java (2)
121-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve executable-level qualifier vars on the method-reference return type.
Like
InferenceMethodType, this return-type path drops the sharedqualifierVars. Any@Poly*qualifier on the referenced method’s return type can stop matching the qualifier variables already assigned to its parameters.♻️ Proposed fix
- if (map == null) { - return new ProperType(annotatedReturnType, returnType, context); - } - return InferenceType.create(annotatedReturnType, returnType, map, context); + return InferenceType.create( + annotatedReturnType, returnType, map, qualifierVars, context, false);🤖 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/typeinference8/types/CompileTimeDeclarationType.java` around lines 121 - 124, The return-type path in CompileTimeDeclarationType drops executable-level qualifier variables: when map != null you call InferenceType.create(annotatedReturnType, returnType, map, context) which loses this object's qualifierVars and thus any `@Poly`* on the referenced method's return type; preserve them by passing the CompileTimeDeclarationType's qualifierVars into the created InferenceType (either by calling an overload of InferenceType.create that accepts qualifierVars or by constructing the InferenceType then assigning its qualifierVars) so that annotatedReturnType/returnType inference keeps the same executable-level qualifier variable set.
83-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOverride the arity-less parameter lookup for unbound member references.
InferenceExecutableType.getParameterTypes(map)passesannotatedExecutableType.getParameterTypes().size(), but this subtype prepends the receiver for unbound references. For an unbound varargs reference, that makessizetoo small by one, so the vararg slot is removed and never added back.♻️ Proposed fix
+ `@Override` + public List<AbstractType> getParameterTypes(Theta map) { + int size = annotatedExecutableType.getParameterTypes().size(); + if (MemberReferenceKind.getMemberReferenceKind(methodRef).isUnbound()) { + size++; + } + return getParameterTypes(map, size); + } + `@Override` public List<AbstractType> getParameterTypes(Theta map, int size) {🤖 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/typeinference8/types/CompileTimeDeclarationType.java` around lines 83 - 103, The method getParameterTypes incorrectly uses the incoming size without accounting for the prepended receiver when MemberReferenceKind.getMemberReferenceKind(methodRef).isUnbound() is true, causing varargs removal to drop the last slot; fix by incrementing the effective arity used for varargs handling when unbound (e.g., adjust size = size + 1 right after you prepend the receiver) so the subsequent TreeUtils.isVarargsCall branch and its loops that rely on size/params.size()/paramsJava.size() operate with the receiver accounted for; update references in getParameterTypes (methodRef, receiver, size, params, paramsJava, TreeUtils.isVarargsCall) accordingly.
🤖 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
`@framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java`:
- Around line 121-124: The return-type path in CompileTimeDeclarationType drops
executable-level qualifier variables: when map != null you call
InferenceType.create(annotatedReturnType, returnType, map, context) which loses
this object's qualifierVars and thus any `@Poly`* on the referenced method's
return type; preserve them by passing the CompileTimeDeclarationType's
qualifierVars into the created InferenceType (either by calling an overload of
InferenceType.create that accepts qualifierVars or by constructing the
InferenceType then assigning its qualifierVars) so that
annotatedReturnType/returnType inference keeps the same executable-level
qualifier variable set.
- Around line 83-103: The method getParameterTypes incorrectly uses the incoming
size without accounting for the prepended receiver when
MemberReferenceKind.getMemberReferenceKind(methodRef).isUnbound() is true,
causing varargs removal to drop the last slot; fix by incrementing the effective
arity used for varargs handling when unbound (e.g., adjust size = size + 1 right
after you prepend the receiver) so the subsequent TreeUtils.isVarargsCall branch
and its loops that rely on size/params.size()/paramsJava.size() operate with the
receiver accounted for; update references in getParameterTypes (methodRef,
receiver, size, params, paramsJava, TreeUtils.isVarargsCall) accordingly.
In
`@framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceExecutableType.java`:
- Around line 98-103: getThrownTypes builds thrown types using
InferenceType.create(t, iter.next(), map, context) which ignores this
executable's precomputed qualifierVars and yields inconsistent `@Poly`* mappings;
update getThrownTypes to pass the executable's qualifierVars into the
thrown-type construction (i.e., call the InferenceType.create overload that
accepts qualifierVars, supplying the stored qualifierVars from this class) so
thrown types share the same qualifier variable mapping as the parameters/return.
In
`@framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceMethodType.java`:
- Around line 71-74: When map == null the code returns a new ProperType which
bypasses this object's precomputed qualifierVars and causes polymorphic
qualifiers on the return to get fresh variables; instead, preserve the existing
qualifierVars by returning an InferenceType constructed with them. Replace the
early return in InferenceMethodType (the branch that currently does `return new
ProperType(annotatedReturnType, returnType, context)`) to call
InferenceType.create(annotatedReturnType, returnType, qualifierVars, context) so
the return type shares the same qualifierVars as the parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d1c8640-5e5c-4da0-837c-4f7016bf5643
📒 Files selected for processing (6)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/InvocationTypeInference.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/constraint/Expression.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceExecutableType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceFactory.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceMethodType.java
Contributor
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/typeinference8/types/CompileTimeDeclarationType.java`:
- Around line 74-76: Update the Javadoc in class CompileTimeDeclarationType for
the method that returns the method reference (the method whose Javadoc currently
reads "Returns the method reference for which this a compile-time declaration.")
by changing the phrase "for which this a" to "for which this is a" so the
sentence reads "Returns the method reference for which this is a compile-time
declaration."
🪄 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: 8be71a08-0b62-44f4-ac42-04830be751f3
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java
Member
|
@smillst Please look at the CodeRabbit "outside the diff" suggestions, then re-assign me. Thanks! |
Contributor
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)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java (1)
82-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix unbound-varargs sizing when
getParameterTypes(map)is called without explicitsize.Line 94 and Line 98 expand varargs using
size, but for unbound references the receiver is prepended at Line 88 while inherited defaultgetParameterTypes(map)still uses the pre-receiver parameter count. In the exact method-reference path (Expression.reduceMethodRef), this can yield too few formals for unbound varargs references.Suggested fix
+ `@Override` + public List<AbstractType> getParameterTypes(Theta map) { + int size = annotatedExecutableType.getParameterTypes().size(); + if (MemberReferenceKind.getMemberReferenceKind(methodRef).isUnbound()) { + size++; + } + return getParameterTypes(map, size); + } + `@Override` public List<AbstractType> getParameterTypes(Theta map, int size) {🤖 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/typeinference8/types/CompileTimeDeclarationType.java` around lines 82 - 104, The varargs expansion uses the incoming size parameter but the overload getParameterTypes(Theta map) calls the two-arg getParameterTypes without accounting for the extra receiver inserted for unbound method references; update the no-size overload to compute an effective size that equals the annotatedExecutableType parameter count plus 1 if MemberReferenceKind.getMemberReferenceKind(methodRef).isUnbound() before delegating to getParameterTypes(Theta map, int size) (so the receiver-added parameter is reflected when expanding varargs), leaving the existing getParameterTypes(Theta map, int size), TreeUtils.isVarargsCall(methodRef), receiver handling, and InferenceType.create(...) intact.
🤖 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
`@framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java`:
- Around line 82-104: The varargs expansion uses the incoming size parameter but
the overload getParameterTypes(Theta map) calls the two-arg getParameterTypes
without accounting for the extra receiver inserted for unbound method
references; update the no-size overload to compute an effective size that equals
the annotatedExecutableType parameter count plus 1 if
MemberReferenceKind.getMemberReferenceKind(methodRef).isUnbound() before
delegating to getParameterTypes(Theta map, int size) (so the receiver-added
parameter is reflected when expanding varargs), leaving the existing
getParameterTypes(Theta map, int size), TreeUtils.isVarargsCall(methodRef),
receiver handling, and InferenceType.create(...) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7ec1a77-6851-4407-8776-7c6fcdd18371
📒 Files selected for processing (1)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java
Contributor
|
Actionable comments posted: 0 |
mernst
requested changes
May 29, 2026
Contributor
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)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java (1)
44-51: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider making fields private.
The
receiverandmethodReffields have package-private visibility. Since there's already a getter formethodRef, consider making these fieldsprivatefor better encapsulation.♻️ Suggested visibility change
- AnnotatedTypeMirror receiver; + private final AnnotatedTypeMirror receiver; /** The method reference tree. */ - MemberReferenceTree methodRef; + private final MemberReferenceTree methodRef;🤖 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/typeinference8/types/CompileTimeDeclarationType.java` around lines 44 - 51, Make the fields 'receiver' and 'methodRef' in class CompileTimeDeclarationType private to improve encapsulation; keep the existing getter for methodRef (and add one for receiver if external code needs access) and update any internal references that currently access these package-private fields directly to use the getters (or adjust within the same class), ensuring no behavior change other than visibility.
♻️ Duplicate comments (1)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java (1)
53-54: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueJavadoc terminology mismatch.
The Javadoc says "Creates an invocation type for a method reference" but this class represents a compile-time declaration type, not an invocation type. Consider updating to "Creates a compile-time declaration type for a method reference" for consistency with the class name and purpose.
🤖 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/typeinference8/types/CompileTimeDeclarationType.java` around lines 53 - 54, Update the Javadoc for CompileTimeDeclarationType to reflect its actual purpose: change the sentence "Creates an invocation type for a method reference" to something like "Creates a compile-time declaration type for a method reference" so the terminology matches the class name CompileTimeDeclarationType and its role; ensure any other nearby Javadoc references to "invocation type" within this class are similarly adjusted to "compile-time declaration type" to keep consistency.
🤖 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
`@framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java`:
- Around line 44-51: Make the fields 'receiver' and 'methodRef' in class
CompileTimeDeclarationType private to improve encapsulation; keep the existing
getter for methodRef (and add one for receiver if external code needs access)
and update any internal references that currently access these package-private
fields directly to use the getters (or adjust within the same class), ensuring
no behavior change other than visibility.
---
Duplicate comments:
In
`@framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java`:
- Around line 53-54: Update the Javadoc for CompileTimeDeclarationType to
reflect its actual purpose: change the sentence "Creates an invocation type for
a method reference" to something like "Creates a compile-time declaration type
for a method reference" so the terminology matches the class name
CompileTimeDeclarationType and its role; ensure any other nearby Javadoc
references to "invocation type" within this class are similarly adjusted to
"compile-time declaration type" to keep consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70546306-de3f-469a-98d3-7894c80e7dd1
📒 Files selected for processing (11)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/DefaultTypeArgumentInference.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/InferenceResult.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/InvocationTypeInference.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/TypeArgumentInference.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/constraint/AdditionalArgument.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/constraint/Expression.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/AbstractType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceExecutableType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceFactory.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceInvocationType.java
Contributor
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 (2)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/AbstractInvocationType.java (1)
78-100: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider extracting the varargs check to avoid duplicate evaluation.
The method checks
TreeUtils.isVarargsCall(invocation)twice (lines 83 and 91). Extract the result to a boolean variable to improve clarity and avoid redundant calls.♻️ Proposed refactor
`@Override` public List<AbstractType> getParameterTypes(Theta map, int size) { List<AnnotatedTypeMirror> params = new ArrayList<>(annotatedExecutableType.getParameterTypes()); List<TypeMirror> paramsJava = new ArrayList<>(executableType.getParameterTypes()); + boolean isVarargs = TreeUtils.isVarargsCall(invocation); - if (TreeUtils.isVarargsCall(invocation)) { + if (isVarargs) { AnnotatedTypeMirror eltType = ((AnnotatedArrayType) params.remove(params.size() - 1)).getComponentType(); for (int i = params.size(); i < size; i++) { params.add(eltType); } } - if (TreeUtils.isVarargsCall(invocation)) { + if (isVarargs) { TypeMirror eltType = ((ArrayType) paramsJava.remove(paramsJava.size() - 1)).getComponentType(); for (int i = paramsJava.size(); i < size; i++) { paramsJava.add(eltType); } } return InferenceType.create(params, paramsJava, map, qualifierVars, context); }🤖 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/typeinference8/types/AbstractInvocationType.java` around lines 78 - 100, The method getParameterTypes currently calls TreeUtils.isVarargsCall(invocation) twice; store the result in a local boolean (e.g., boolean isVarargs = TreeUtils.isVarargsCall(invocation)) and use isVarargs in both varargs branches so you only compute it once, then proceed to adjust params and paramsJava as before (using eltType for AnnotatedArrayType and TypeMirror for ArrayType) and finally call InferenceType.create(params, paramsJava, map, qualifierVars, context).framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java (1)
83-108: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider extracting the varargs check to avoid duplicate evaluation.
The method checks
TreeUtils.isVarargsCall(methodRef)twice (lines 96 and 101). Extract the result to a boolean variable to improve clarity and avoid redundant calls.♻️ Proposed refactor
`@Override` public List<AbstractType> getParameterTypes(Theta map, int size) { List<AnnotatedTypeMirror> params = new ArrayList<>(); List<TypeMirror> paramsJava = new ArrayList<>(); if (MemberReferenceKind.getMemberReferenceKind(methodRef).isUnbound()) { // For unbound method references, i.e. Type::instanceMethod, the receiver is treated as the // first parameter. params.add(receiver); paramsJava.add(receiver.getUnderlyingType()); } params.addAll(annotatedExecutableType.getParameterTypes()); paramsJava.addAll(executableType.getParameterTypes()); + boolean isVarargs = TreeUtils.isVarargsCall(methodRef); - if (TreeUtils.isVarargsCall(methodRef)) { + if (isVarargs) { AnnotatedArrayType vararg = (AnnotatedArrayType) params.remove(params.size() - 1); for (int i = params.size(); i < size; i++) { params.add(vararg.getComponentType()); } + } + if (isVarargs) { ArrayType varargTM = (ArrayType) paramsJava.remove(paramsJava.size() - 1); for (int i = paramsJava.size(); i < size; i++) { paramsJava.add(varargTM.getComponentType()); } } return InferenceType.create(params, paramsJava, map, qualifierVars, context); }🤖 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/typeinference8/types/CompileTimeDeclarationType.java` around lines 83 - 108, In getParameterTypes (CompileTimeDeclarationType) avoid calling TreeUtils.isVarargsCall(methodRef) twice by storing its result in a boolean (e.g., boolean isVarargs = TreeUtils.isVarargsCall(methodRef)) and use isVarargs in the subsequent conditional and logic that handles varargs for both params and paramsJava; update the method to compute isVarargs once and keep all existing varargs handling (removing the duplicate call) so behavior of getParameterTypes, receiver handling, and InferenceType.create remains identical.
🤖 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/typeinference8/types/AbstractExecutableType.java`:
- Around line 42-45: Fix the grammar in the Javadoc for AbstractExecutableType:
update the sentence "annotation mirror should be represented by with {`@code`
QualifierVar}" to read either "annotation mirror should be represented by {`@code`
QualifierVar}" or "annotation mirror should be represented by a {`@code`
QualifierVar}" so the comment is grammatically correct and clear; edit the
Javadoc block above the mapping field in class AbstractExecutableType to apply
this change.
---
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/AbstractInvocationType.java`:
- Around line 78-100: The method getParameterTypes currently calls
TreeUtils.isVarargsCall(invocation) twice; store the result in a local boolean
(e.g., boolean isVarargs = TreeUtils.isVarargsCall(invocation)) and use
isVarargs in both varargs branches so you only compute it once, then proceed to
adjust params and paramsJava as before (using eltType for AnnotatedArrayType and
TypeMirror for ArrayType) and finally call InferenceType.create(params,
paramsJava, map, qualifierVars, context).
In
`@framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.java`:
- Around line 83-108: In getParameterTypes (CompileTimeDeclarationType) avoid
calling TreeUtils.isVarargsCall(methodRef) twice by storing its result in a
boolean (e.g., boolean isVarargs = TreeUtils.isVarargsCall(methodRef)) and use
isVarargs in the subsequent conditional and logic that handles varargs for both
params and paramsJava; update the method to compute isVarargs once and keep all
existing varargs handling (removing the duplicate call) so behavior of
getParameterTypes, receiver handling, and InferenceType.create remains
identical.
🪄 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: e4b65394-3f08-4ef3-b14a-fff79b7bb4f9
📒 Files selected for processing (7)
framework/src/main/java/org/checkerframework/framework/util/typeinference8/InvocationTypeInference.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/constraint/AdditionalArgument.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/constraint/Expression.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/AbstractExecutableType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/AbstractInvocationType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/InferenceFactory.java
Contributor
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/typeinference8/types/AbstractExecutableType.java (1)
89-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPotential bug:
getJavaType()may return the wrong type.The constructor's Javadoc (lines 53–55) explicitly states that
executableType"is not always equal toannotatedExecutableType.getUnderlyingType()." However, this method returnsannotatedExecutableType.getUnderlyingType()instead of the storedexecutableTypefield.If the distinction matters (and the Javadoc suggests it does), this should return
executableType.🐛 Proposed fix
public ExecutableType getJavaType() { - return annotatedExecutableType.getUnderlyingType(); + return executableType; }🤖 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/typeinference8/types/AbstractExecutableType.java` around lines 89 - 96, The getJavaType method in AbstractExecutableType currently returns annotatedExecutableType.getUnderlyingType(), but the class stores a separate field executableType (and the constructor docs state they may differ); change getJavaType to return the stored executableType field instead of calling annotatedExecutableType.getUnderlyingType() so callers receive the actual Java ExecutableType that was saved during construction.
🤖 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/typeinference8/types/CompileTimeDeclarationType.java`:
- Around line 35-46: The Javadoc in CompileTimeDeclarationType contains
unresolved Git conflict markers; remove the markers (<<<<<<<, |||||||, =======,
>>>>>>>) and collapse the variants into the intended single sentence for the
example (e.g., use the final, type-explicit wording), so the Javadoc reads
something like: "The function type is {`@code` int compare(Comparator<MyClass>
this, MyClass o1, MyClass o2)} whereas the compile-time declaration type is
{`@code` int compareByField(MyClass this, MyClass other)}." Ensure the comment
around the example in CompileTimeDeclarationType and any references to
compare/compareByField are clean and free of conflict artifacts.
---
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/framework/util/typeinference8/types/AbstractExecutableType.java`:
- Around line 89-96: The getJavaType method in AbstractExecutableType currently
returns annotatedExecutableType.getUnderlyingType(), but the class stores a
separate field executableType (and the constructor docs state they may differ);
change getJavaType to return the stored executableType field instead of calling
annotatedExecutableType.getUnderlyingType() so callers receive the actual Java
ExecutableType that was saved during construction.
🪄 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: 804c9bfd-753f-4dba-8a2d-677ece3907ac
📒 Files selected for processing (5)
framework/src/main/java/org/checkerframework/framework/util/element/ElementAnnotationUtil.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/AbstractExecutableType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/AbstractInvocationType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/CompileTimeDeclarationType.javaframework/src/main/java/org/checkerframework/framework/util/typeinference8/types/QualifierVar.java
mernst
previously approved these changes
May 31, 2026
| import org.checkerframework.javacutil.AnnotationMirrorSet; | ||
|
|
||
| /** | ||
| * An inference type for a method, constructor, or method reference. This is a wrapper around {@link |
Member
There was a problem hiding this comment.
Should this be "a method or constructor invocation" rather than "a method or constructor"?
|
|
||
| /** | ||
| * A mapping from polymorphic annotation to {@link QualifierVar}. It keeps track of which | ||
| * annotation mirror should be represented by which {@code QualifierVar}. |
Member
There was a problem hiding this comment.
"should be" assumes that the reader already understands the design and the code. Please be more specific.
I think this is computed from the invocation, in which case maybe the map represents the instantiation?
mernst
approved these changes
May 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7680.