daniellansun commented on PR #3286:
URL: https://github.com/apache/groovy/pull/3286#issuecomment-6769352231
Thank you, Scott. Typing `e.getClass()` as `Class<? extends |T|>` is the
right fix, and putting it in `inferReturnTypeGenerics` is the right place: both
the call and a direct `.class` property already come through there, and
`Object#getClass` is final, so limiting the special case to a zero-argument
`getClass` declared on `Object` leaves any other method of that name alone.
I checked the result against JLS 4.3.2 / 15.12.2.6 / 4.6 and against javac.
The common cases match. A `Store` receiver is usable as `Class<? extends
Store>`. `List<String>` and raw `List` are `Class<? extends List>`, with the
arguments dropped. Arrays keep their dimensions, and `int[]` stays `int[]`. A
type variable becomes its leftmost bound, including `T extends Number &
Runnable` (so `Class<? extends Runnable>` is correctly rejected).
`super.getClass()` in a subclass is `Class<? extends Super>`, and an
unqualified `getClass()` is `Class<? extends Sub>`. `String.class` stays
`Class<String>`; `String.getClass()` is `Class<? extends Class>`. Flow typing,
wildcard receivers, enum / annotation / sealed receivers, and a closure or
`with` block all follow the receiver that STC actually resolved. The updated
raw-`List` expectation and the GROOVY-5415 diagnostic (`Class<? extends
ClassB>`) are the types this rule produces. The new tests fail if the special
case is removed, which is what
we want.
Two things are still wrong, and they are user-visible.
**Enclosing type arguments are not erased.** JLS 4.6: the erasure of a
nested type `T.C` is `|T|.C`. javac rejects `Class<? extends
Outer<String>.Inner> c = inner.getClass()` and rejects `Class<? extends Inner>`
inside `Outer<T>.Inner` (that name means `Outer<T>.Inner`). It accepts `Class<?
extends Outer.Inner>`. The inferred type here is `Class<? extends
Outer<String>.Inner>`. `getPlainNodeReference` copies the outer class type
through, and `erasureOf` never erases it. Top-level `Box<E>.getClass()`
correctly drops `E`. Please erase the outer type in that helper and add both
the negative and the positive test.
**The raw bound is then accepted as a parameterized bound.** For
`List<String> strings`, the inferred type `Class<? extends List>` is what javac
computes. javac nevertheless errors, it does not warn, on:
- `Class<? extends List<String>> f(List<String> s) { return s.getClass(); }`
- `Class<? extends Comparable<T>> f(T t) { return t.getClass(); }` for `T
extends Comparable<T>`
- `Class<? extends Outer<T>> f() { return getClass(); }` inside `Outer<T>`
All three compile with this patch. They did not compile before it:
`getClass()` was `Class<?>`, and that is rejected for `Class<? extends List>`.
The same return check accepts a hand-written `Class<? extends List>` as
`Class<? extends List<String>>`, and a hand-written `Class<? extends Number>`
as `Class<? extends T>` for `T extends Number`. So the hole is in containment
(`checkReturnValue` / `checkTypeGenerics`), and this change is what feeds
`getClass()` into it. JLS 4.5.1 / 4.10.2: `? extends List` is not contained in
`? extends List<String>`, and `? extends Number` is not contained in `? extends
T`. `Class<?>` is already rejected for `Class<? extends Store>`; the raw bound
needs that same strictness. GROOVY-12754 does not cover this path. I don't want
to merge 12755 while those returns succeed. Fixing the comparison can be this
PR or a prerequisite that lands first.
Please also cover these in the tests. The new Java compatibility test shows
that javac and STC both accept the program and return the same runtime string.
It does not record the inferred type, so a raw bound that we then treat as
parameterized still passes. The negative `Object` test passes even without the
special case.
Two smaller gaps in the same area:
- A cast receiver `(Number & Runnable)` is typed `Class<? extends
IntersectionType>` and is accepted as both `Class<? extends Number>` and
`Class<? extends Runnable>`. javac allows only the leftmost bound. A type
variable `T extends Number & Runnable` is already correct. JLS 4.6 never
mentions intersection erasure; 4.9's notional class and javac both use the
leftmost class. Please reduce the intersection in `erasureOf`.
- `list*.getClass()` is `List<Class<? extends String>>`, and `list*.class`
is `List<Class<?>>`. Spread properties record `Object#getClass`'s declared
return type in `PropertyLookup.visitMethod` and skip `inferReturnTypeGenerics`.
Direct `.class` is fine. The property spelling is the one people write.
GROOVY-12754 has to be in place before this one. With the new type and that
adjustment disabled, `foo(l.class, l)` for `<T extends List<Integer>> T
foo(Class<T>, …)` fails, and so does `<T extends CharSequence> m(Class<T>)` on
`Class<? extends String>`. javac accepts the first with an unchecked warning
and the second with none. The Grails shape, a parameter already declared
`Class<? extends Datastore>`, does not need 12754. The generic-method shape
does. 12754 does not fix the nested-type or raw-bound issues above.
On the CI red: every failing `lts` job is
`StaticCompileRuntimePackageTest.testSystemProperty`, and the option map has no
`runtimePackageCheck` entry. This commit does not touch
`CompilerConfiguration`. That read was removed on master and later restored.
The PR builds were merging a master that still had the deletion. I don't regard
that as a defect here; rebasing onto current master should clear it.
Primitive receivers are a Groovy extension (javac refuses to dereference an
`int`). Boxing to `Class<? extends Integer>` / `Boolean` / `Character`, and not
boxing `int[]`, matches `getClass` on the value Groovy actually passes. Static
`this` as `Class<C>`, and `getClass()` inside a closure or `with` block, follow
the receiver STC already chose. `null.getClass()` is `Class<? extends Object>`,
which is Groovy's null receiver rather than the JLS error. `int.class` and
`void.class` are still `Class<int>` and `Class<void>` rather than
`Class<Integer>` and `Class<Void>` (JLS 15.8.2). That is the class-literal
path, not this patch; please leave literals off the `getClass` branch so
`String.class` remains `Class<String>`.
I think this is close. The nested erasure and the raw-bound return are the
part I want to see changed before a merge.
--
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]
