Fix problem with type var in a method reference#7729
Open
smillst wants to merge 13 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR replaces the concrete InvocationType with an abstract InferenceExecutableType and adds two concrete subclasses: InferenceMethodType (for method and constructor invocations) and CompileTimeDeclarationType (for method-reference compile-time declarations). InferenceFactory is updated to construct and return these concrete types. InvocationTypeInference and constraint-processing code paths are updated to accept and thread the new types. A regression test Issue7680 is added and a small Javadoc link update is included. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
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.