G
YouTube TikTok Games
Skip to content

GROOVY-12727: STC: infer a generic method's type arguments from the c… - #3265

Open
paulk-asert wants to merge 1 commit into
apache:masterfrom
paulk-asert:groovy12727
Open

paulk-asert wants to merge 1 commit into
apache:masterfrom
paulk-asert:groovy12727

Conversation

@paulk-asert

Copy link
Copy Markdown
Contributor

…all's target type when a lambda or closure argument needs them

…all's target type when a lambda or closure argument needs them
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.4101%. Comparing base (5222105) to head (60711c7).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...roovy/transform/stc/StaticTypeCheckingVisitor.java 96.1538% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##               master      #3265        +/-   ##
==================================================
+ Coverage     74.4085%   74.4101%   +0.0016%     
- Complexity      44367      44386        +19     
==================================================
  Files            1699       1699                
  Lines          151050     151075        +25     
  Branches        28783      28792         +9     
==================================================
+ Hits           112394     112415        +21     
- Misses          28376      28378         +2     
- Partials        10280      10282         +2     
Files with missing lines Coverage Δ
...roovy/transform/stc/StaticTypeCheckingVisitor.java 88.0860% <96.1538%> (+0.0407%) ⬆️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@testlens-app

testlens-app Bot commented Nov 8, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 60711c7
▶️ Tests: 122876 executed
⚪️ Checks: 26/26 completed


Learn more about TestLens at testlens.app/docs.

@daniellansun

Copy link
Copy Markdown
Contributor

REQUEST_CHANGES

The Pekko assignment and return both type-check, for a lambda and for a closure, and GenericsSTCTest is green including the three new methods. The witness is written onto StaticMethodCallExpression and then read only from the enclosing-call stack, which visitStaticMethodCallExpression never pushes. By INSTRUCTION_SELECTION, StaticImportVisitor has already turned a same-class static call and an import static call into StaticMethodCallExpression, so those still fail with the original error:

static Beh<String> go(int max) { setup(c -> new Bot(c, max)) }
// Cannot call Bot#<init>(Ctx<String>, int) with arguments [Ctx<? extends Object>, int]

javac accepts that. import static java.util.Comparator.comparing then Comparator<String> c = comparing(s -> s.length()) fails here with Object#length() and Comparator assigned to Comparator. Qualified Behaviors.setup(...) stays a MethodCallExpression, which is the only shape the new tests hit.

Please push/pop the static call in visitStaticMethodCallExpression the same way visitMethodCallExpression does, and add those two calls beside testReturnTypeInferenceWithClosure5. makeArgumentList returns the same ArgumentListExpression the parser stored, so the identity check in extractGenericsConnectionsFromCallTarget will then succeed.

A few targets the helper’s comment says it determines are still unresolved. I compared them with javac on this tip:

  • <T extends CharSequence> List<T> ids(Function<T,T>) assigned to List<String> leaves the lambda parameter as CharSequence (CharSequence#substring is missing; the assignment is List to List). javac uses T = String. An out-of-bound target (T extends Number against a call whose parameter needs String) stays at Number, so this is under-acceptance, not a bad instantiation. putIfAbsent still does not check the witness against T’s upper bounds.
  • List<List<T>> assigned to List<List<String>>, and Map<A,B> assigned to Map<String,Integer>, both leave the variables at Object (Object#length(), List<List>, Map<Object,Object>). The return type still carries placeholders named T, A, and B. ArrayList versus List works; the nested and two-variable cases do not. Map<T,T> versus Map<String,Integer> is rejected, which matches javac, but only as a last-write in storeGenericsConnection rather than a conflict error.
  • Beh<? super String> b = Behaviors.setup(c -> new Bot(c)) is still Ctx<? extends Object>. javac accepts it: ? super String pins T to String, and Beh is a subtype of Beh<? super String>. Skipping every wildcard is right for Beh<? extends Object> (test 7, and javac agrees) and too coarse for a concrete ? super or ? extends bound.
  • A ternary in an assignment, a call passed to take(Beh), and Optional.of(setup(...)) still show Ctx<? extends Object>. The target is applied to the ternary node and not to the branch, and checkForTargetType still has the TODO for argument parameters. A cast does infer T, because visitCastExpression calls applyTargetType; javac does not treat a cast as a poly context for the invocation (JLS 15.12) and rejects (Beh<String>) setup(c -> new Bot(c)). The Groovy result is sound. Worth a comment so the cast hook is intentional.
  • inferReturnTypeGenerics does not see CALL_TARGET_TYPE. List<CharSequence> xs = cons(s -> 'x') for Function<? super T, ? extends T> is inferred as List and rejected. javac uses T = CharSequence.
  • GROOVY-11446 still type-checks (List<Number> x = m(d) for List<?> d and <T> List<T> m(List<? extends T>)). There is no SAM argument, so this patch does not touch it. javac rejects it. The lambda form of that pattern is rejected here. Not a regression; not fixed either.

    I would keep Closure7 as the unbounded-wildcard rejection, and add: same-class static and import static; T extends CharSequence against List; List<List> and Map<A,B>; Beh<? super String> must pass; the Map<T,T> / Map<String,Integer> conflict must fail; one ternary and one argument context. String as a target is a weak assertion on its own, because STC will accept almost any value as a String. The Bot constructor in Closure5 is the right kind of check.

    The ReturnAdder widening looks safe: the listener constructor does not rewrite the method, and an explicit return is stamped only from visitReturnStatement. I do not see a merge conflict with the PECS work on the lines this patch adds; that branch is not an ancestor of 60711c7. Once a ? super bound is stored as a witness, it will meet the tighter compatibleConnection on that branch and should be rechecked.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants