Copilot commented on code in PR #2496:
URL: https://github.com/apache/groovy/pull/2496#discussion_r3153947627
##########
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java:
##########
@@ -1789,9 +1794,19 @@ private static GenericsType applyGenericsContext(final
Map<GenericsTypeName, Gen
GenericsType specType = spec.get(name);
if (specType != null) return specType;
if (hasNonTrivialBounds(gt)) {
- GenericsType newGT = new GenericsType(type,
applyGenericsContext(spec, gt.getUpperBounds()), applyGenericsContext(spec,
gt.getLowerBound()));
- newGT.setPlaceholder(true);
- return newGT;
+ // GROOVY-11022: avoid infinite recursion on F-bounded type
+ // parameters whose self-reference appears inside a wildcard
+ // bound (e.g. `<K extends Comparable<? super K>>`)
+ Deque<GenericsTypeName> expanding = EXPANDING_BOUNDS.get();
+ if (expanding.contains(name)) return gt;
+ expanding.push(name);
+ try {
+ GenericsType newGT = new GenericsType(type,
applyGenericsContext(spec, gt.getUpperBounds()), applyGenericsContext(spec,
gt.getLowerBound()));
+ newGT.setPlaceholder(true);
+ return newGT;
+ } finally {
+ expanding.pop();
Review Comment:
`EXPANDING_BOUNDS` is a static `ThreadLocal` that retains its `ArrayDeque`
(including its internal capacity) for the lifetime of the compilation thread.
Since this stack can grow in pathological cases, it would be better to clear
the ThreadLocal when the stack becomes empty (e.g., after `pop()`, if
`expanding.isEmpty()` then call `EXPANDING_BOUNDS.remove()`) to avoid retaining
an oversized backing array across later compilations on the same thread.
```suggestion
expanding.pop();
if (expanding.isEmpty()) {
EXPANDING_BOUNDS.remove();
}
```
##########
src/test/groovy/groovy/transform/stc/GenericsSTCTest.groovy:
##########
@@ -4616,6 +4616,40 @@ class GenericsSTCTest extends StaticTypeCheckingTestCase
{
}
}
+ // GROOVY-11022
+ @Test
+ void testNoStackOverflow3() {
+ // F-bounded type parameter with self-reference inside a `? super`
wildcard
+ assertScript '''
+ class C<K, V> {
+ public static <K extends Comparable<? super K>, V> C<K, V> m(K
k, V v) {
+ new C<K, V>()
+ }
+ }
+ class Main {
+ @groovy.transform.TypeChecked
+ static test() {
+ C.<Object, Integer>m(null, 1)
+ }
+ }
+ Main.test()
+ '''
+
+ // self-reference inside a `? extends` wildcard
+ assertScript '''
+ class C<K> {
+ public static <K extends Iterable<? extends K>> C<K> m(K k) {
+ new C<K>()
+ }
+ }
+ @groovy.transform.TypeChecked
+ void test() {
+ C.<List<Object>>m(null)
+ }
Review Comment:
The explicit type arguments used in these scripts (`Object` for `K extends
Comparable<? super K>` and `List<Object>` for `K extends Iterable<? extends
K>`) do not satisfy the declared bounds. This makes the regression test depend
on current (possibly lax) bounds-checking behavior and could start failing
if/when STC tightens validation. Consider switching the examples to types that
actually satisfy the bounds (e.g., `Integer` for the `Comparable<? super K>`
case; and a small `Self implements Iterable<Self>` helper type for the
`Iterable<? extends K>` case) so the test focuses solely on the
StackOverflowError regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
