paulk-asert commented on PR #3285:
URL: https://github.com/apache/groovy/pull/3285#issuecomment-6793082468
Thanks Scott, the capture-based revision is a big improvement. I ran a set
of `@CompileStatic` probes against master and against this branch (and the same
patch applied to `GROOVY_6_0_X`; results were identical on both). The headline
cases behave as described, and subtype arguments, multi-parameter methods,
constructors, class-level bounds and DGM calls on wildcard receivers all came
out fine. Two things before this goes further.
**1. A bound that names another type parameter is skipped and never
checked.**
In `compatibleCapture`:
```java
if (bound.isGenericsPlaceHolder()) continue; // another type parameter
```
Both of these are rejected on master and compile on this branch:
```groovy
static <T extends Number, U extends T> U v(List<U> b) { b.get(0) }
static g(List<? extends String> l) { Number n = v(l); n }
// g(['x']) -> GroovyCastException: Cannot cast object 'x' with class
'java.lang.String' to class 'java.lang.Number'
static <T extends Number, U extends List<T>> int v(Class<U> c) { 1 }
static g(Class<? extends ArrayList<String>> c) { v(c) }
```
javac 25.0.5 rejects both ("inference variable T has incompatible bounds"),
and accepts the same calls with `List<? extends Integer>` and `Class<? extends
ArrayList<Integer>>`. Could you add them to `GenericsSTCTest` and
`GenericsJavaCompatibilityTest` and resolve the bound through the other type
parameter's own bound rather than skipping it? This is the same kind of hole as
`narrow`, on calls the PR newly accepts, so I'd like it closed before merging
to master.
**2. Backport to `GROOVY_6_0_X`.**
I'd prefer not to cherry-pick this as it stands. Matching javac here means
that code which compiles and runs correctly on 6.0.0 becomes a compile error in
a patch release:
- `Enum.valueOf(type, name)` and `EnumSet.allOf(type)` with a `Class<?
extends Enum<?>>`, which is a natural way to type an "any enum class" parameter
or field
- `Collections.sort(list)` with a `List<? extends Comparable<?>>`
- any wildcard declared looser than the method needs, e.g. a `Class<?
extends Runnable>` holding `Thread` passed to `<T extends Thread> m(Class<T>)`
The workarounds are easy and the COMPATIBILITY entry describes them well,
but that entry is rightly headed "Groovy 7", and #3286 extends the same
tightening to `Class<?>` and `List<?>`.
If GROOVY-12755 needs the accepting half on 6.0.x, one option is an
accept-only variant for that branch: when the capture check fails, fall through
to the existing `compatibleConnection` path instead of returning incompatible.
That admits the calls that were wrongly rejected without rejecting anything
6.0.0 accepts. It leaves the `narrow` miscompile in place on 6, which is
existing behaviour. I haven't built that variant, so treat it as a suggestion;
happy to hear other views, @daniellansun included, since this differs from the
earlier "cherry-pick once it matches javac".
One observation, not a request: with `m(List<T extends Integer>)` and
`m(Collection<?>)`, passing a `List<? extends Number>` is now an error rather
than selecting the second overload as javac would. That's how the checker
already treats a failed generics check, and it does mean there is no silent
re-dispatch, so I'm fine leaving it.
--
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]
