paulk-asert commented on PR #3286:
URL: https://github.com/apache/groovy/pull/3286#issuecomment-6794299106
AI read (I am still processing):
> Thanks Scott. Typing `e.getClass()` as `Class<? extends |T|>` is the right
fix, and it is needed on `GROOVY_6_0_X`: GROOVY-12714 is on that branch but not
in the 6.0.0 tag, so the Grails shape compiles on 6.0.0 and fails on the branch
today. I ran a set of `@CompileStatic` probes on the 6.0.0 release, on
`GROOVY_6_0_X` and master, and on each with this branch applied (as a whole,
not commit by commit). The headline cases behave as described. Some things I'd
like addressed or decided before this goes in.
>
> **1. Static dispatch changes silently.**
>
> With overloads `f(Object)`, `f(CharSequence)` and `f(StringBuilder)`, and
`<T> T mk(Class<T>)`:
>
> ```groovy
> f(x.getClass().cast(o)) // master: "object" this branch: "sb"
> f(x.getClass().newInstance()) // master: "object" this branch: "sb"
> def y = mk(x.getClass()); f(y) // master: "object" this branch: "sb"
> ```
>
> The new result is what dynamic Groovy picks and what javac would pick, so
I think it is the right answer, but it is a runtime change with no compile
error. Please add it to the COMPATIBILITY entry and add a test that records it.
>
> **2. Calls that javac accepts are now rejected.**
>
> These compile on 6.0.0, `GROOVY_6_0_X` and master, javac 25.0.5 accepts
the Java forms, and this branch rejects them:
>
> ```groovy
> List<Class<?>> l = Collections.singletonList(x.getClass())
> List<Class<?>> l = Arrays.asList(a.getClass(), b.getClass())
> Optional<Class<?>> o = Optional.of(x.getClass())
> ```
>
> javac infers from the target type; STC infers `List<Class<? extends
String>>` bottom-up and then fails the assignment. The Groovy forms fail the
same way when returned or assigned as `List<Class<?>>`: `list.collect {
it.getClass() }`, `list*.getClass()` and `list*.class`. A list literal and
`List<Class>` still work. These are the cases I'm most concerned about, since
they are not "as in Java" and the spread and `collect` forms are what people
write. Can the assignment check accept a `Class<? extends X>` element for a
`Class<?>` one here, or is there another way to keep them compiling?
>
> **3. The type-variable rule is uneven.**
>
>`<T> Class<? extends T> typeOf(T t) { t.getClass() }` and a field `Class<?
extends T> type = t.getClass()` are now errors, as in javac. But `<T> Class<T>
typeOf(T t) { t.getClass() }` still compiles (javac rejects it), and so does
passing `t.getClass()` to `m(Class<? extends T>)` or to `Map<Class<? extends
T>, T>.put` inside a generic class (javac rejects the `put`). So the more
accurate declaration is the one that breaks, and only for assignment and
return. I'd prefer these to be consistent one way or the other.
>
> **4. The raw-bound rule reaches well beyond `getClass()`.**
>
> These compile today and are errors on this branch, as in javac:
>
> ```groovy
> List<? extends Map<?, ?>> a = rows // rows is a List<? extends
Map>
> List<? extends Map<String, Object>> b = rows
> Class<? extends Enum<?>> c = type // type is a Class<? extends
Enum>
> m(closureType) // m(Class<? extends
Closure<String>>), closureType is a Class<? extends Closure>
> ```
>
> Raw `Map`, `Closure`, `Comparable` and `Enum` are much more common in
Groovy code than in Java, so I expect this to be the most widely felt part of
the change. I'm fine with it for 7 given the COMPATIBILITY entry; see below for
6.
>
> **5. Backport to `GROOVY_6_0_X`.**
>
> I'd like to split this for the port rather than take it whole:
>
> - **Port:** the `getClass()` typing (`2ce6046aa3`, `f2d84108eb`), after
GROOVY-12754. That is the regression fix. Items 1 and 2 come with it, so they
need settling first.
> - **Master only:** the resolver change (`053e7ead2d`) and the
unbounded/lower-bounded wildcard check (`2b99f21e83`). Neither is needed for
the Grails case. The second makes `Enum.valueOf`, `EnumSet.allOf` and
`getAnnotation` with a `Class<?>`, `Enum.valueOf(field.getType(), name)`,
`Enum.valueOf(Class.forName(n), name)` and `Collections.sort(List<?>)` errors,
which is a lot of working reflection code for a patch release.
> - **To discuss:** the raw-bound commits (`7807d56395`, `0750119074`).
Daniel asked for them so that `Class<? extends List<String>> f(List<String> s)
{ s.getClass() }` does not compile. That return is unsound, but it also
compiled on 6.0.0, and these commits bring items 3 and 4 to 6.0.x. My
inclination is to leave them on master, but I'd like Daniel's view.
>
> For the record, the port does not fully restore 6.0.0 behaviour, which I
think is acceptable but should be in the 6.0.1 notes. These compile on 6.0.0
and remain errors on the branch with this change (javac rejects them too):
>
> ```groovy
> Class<String> c = s.getClass()
> Class<? extends Collection<?>> c = coll.getClass()
> reg(k.getClass()) // reg(Class<? extends Converter<?>>), k is a
Converter<Integer>
> ```
>
> **The resolver change** was the part I expected trouble from, since it
applies to dynamic code as well, and I found none. I ran 34 scenarios
(linked-node classes, subclass overrides, `@Delegate`/`@Lazy`, closures,
lambdas, anonymous classes, deep nesting), each dynamic and static, and all
gave the same result as master. Reflection over the generated classes had no
new failures, clients compiled against precompiled classes behaved the same,
and joint compilation with Java using `Outer<String>.Inner` fails on master and
works here. The `inClosure()` difference on 6.0.x reproduced as you described.
--
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]
