[
https://issues.apache.org/jira/browse/GROOVY-12755?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18142917#comment-18142917
]
ASF GitHub Bot commented on GROOVY-12755:
-----------------------------------------
paulk-asert commented on PR #3286:
URL: https://github.com/apache/groovy/pull/3286#issuecomment-6793520097
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.
> STC: e.getClass() is typed Class<?>, so since GROOVY-12714 it is rejected
> where a Class<? extends T> is expected
> ----------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12755
> URL: https://issues.apache.org/jira/browse/GROOVY-12755
> Project: Groovy
> Issue Type: Bug
> Components: Static Type Checker
> Affects Versions: 7.0.0-alpha-1, 6.0.1
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> JLS 4.3.2 gives a call {{e.getClass()}} the type {{Class<? extends |T|>}},
> where {{|T|}} is the erasure of the static type of {{e}}. Static type
> checking types it with the declared return type of {{Object#getClass()}},
> {{Class<?>}}. Until GROOVY-12714 the wildcard check accepted {{Class<?>}}
> where a {{Class<? extends T>}} was expected. Since GROOVY-12714 (GROOVY_6_0_X
> and master) it is rejected:
> {code:groovy}
> @groovy.transform.CompileStatic
> class C {
> interface Store {}
> static <T> T with(Class<? extends Store> type, Closure<T> body) {
> body.call() }
> static class Adapter { Adapter(Class<? extends Store> type) {} }
> static Object viaMethod(Store store) { with(store.getClass()) { 1 } }
> static Object viaProperty(Store store) { with(store.class) { 1 } }
> static Object viaConstructor(Store store) { new Adapter(store.getClass())
> }
> static Class<? extends Store> viaReturn(Store store) { store.getClass() }
> }
> {code}
> {noformat}
> [Static type checking] - Cannot call <T> C#with(java.lang.Class<? extends
> C.Store>, groovy.lang.Closure<T>) with arguments [java.lang.Class<?>,
> groovy.lang.Closure]
> [Static type checking] - Cannot call <T> C#with(java.lang.Class<? extends
> C.Store>, groovy.lang.Closure<T>) with arguments [java.lang.Class<?>,
> groovy.lang.Closure]
> [Static type checking] - Cannot call C$Adapter#<init>(java.lang.Class<?
> extends C.Store>) with arguments [java.lang.Class<?>]
> [Static type checking] - Incompatible generic argument types. Cannot assign
> java.lang.Class<?> to: java.lang.Class<? extends C.Store>
> {noformat}
> javac accepts the equivalent Java. Grails no longer compiles against
> GROOVY_6_0_X: it passes {{datastore.getClass()}} to a {{Class<? extends
> Datastore>}} parameter and {{registrar.getClass()}} to a {{Class<? extends
> BeanRegistrar>}} one
> (https://github.com/apache/grails-core/actions/runs/42702095261).
> Typing {{e.getClass()}} and {{e.class}} as {{Class<? extends |T|>}} makes
> more calls pass an upper-bounded wildcard for a bounded type parameter, such
> as {{foo(list.class)}} for {{<T extends List<Integer>> foo(Class<T>)}}, which
> GROOVY-12754 rejected. So this depends on GROOVY-12754.
> The inferred type of {{list.class}} for a raw {{List list}} changes from
> {{Class<? extends Object>}} to {{Class<? extends List>}}, and the GROOVY-5415
> error for {{a.bar(this.getClass())}} now names {{Class<? extends ClassB>}};
> the two tests that assert those types are updated.
> PR: https://github.com/apache/groovy/pull/3286
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
