[
https://issues.apache.org/jira/browse/GROOVY-12319?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18109734#comment-18109734
]
ASF GitHub Bot commented on GROOVY-12319:
-----------------------------------------
paulk-asert commented on PR #2845:
URL: https://github.com/apache/groovy/pull/2845#issuecomment-5472695848
I am generally in favor of this change but want to draw attention to one
aspect before we merge as per this AI read.
> Alongside the three new syntax forms, the `GenericsVisitor` changes add
JLS well-formedness checks that run on **every class — dynamic Groovy and
scripts included, before AST transforms, with no opt-out**. I ran probes on
4.0.27, 5.0.8, 6.0.0-beta-3 and builds of both PR heads (`f0de882dd4`,
`86f1ab3e3c`): **eleven forms that compile and run today (and have since at
least 4.0) become compile errors**. Each is Java-consistent, and several expose
silently-wrong code, so break-and-document is defensible for all of them — but
we should decide that knowingly, per form:
>
> | Form | Behaviour ≤ beta-3 (dynamic) | Suggest |
> |---|---|---|
> | `new T[n]`, `new T[]{t}` in `class G<T>` | `Object[]` typed as `T[]` —
the Groovy shortcut for Java's `(T[]) new Object[n]` | **team call** — the one
form with a real constituency; a small carve-out in `isReifiable` for bare type
variables is possible |
> | `class E<T> extends Exception` | works; no JVM/Groovy reason to forbid,
pure javac parity | **team call** — cheapest to leave if we want to minimise
breakage |
> | `new List<String>[n]` | raw array, args ignored | break & document (`new
List<?>[n]` still works) |
> | `List<int>` (and nested) | dynamic: treated as raw | break & document
(STC already rejected it with a worse message) |
> | `new T()` / `T.class` | silently `Object` / `Object.class` | break &
document — exposes wrong code; point to the `Class<T>` token idiom |
> | `implements X<? extends …>` | works (superclass form was already
rejected) | break & document |
> | `new ArrayList<?>()` | works, harmless | break & document (parity) |
> | class type in additional bound `<T extends A & B>` | works | break &
document |
> | `Foo<String>.class` | evaluates to `Foo.class` | break & document, but
the message needs fixing (see below) |
> | `Outer.Inner<String>` with raw generic `Outer` (added in the follow-up)
| works | break & document (fix: `Outer<?>.Inner<String>`, expressible thanks
to this PR) |
>
> Not breaking, for the record: `new Comparator<int>() { … }` already failed
at runtime with a corrupt `Signature` (`GenericSignatureFormatError`) since 4.x
— the new error is a fix; the stricter `instanceof` forms and the
static-context `T` reference were already errors (clearer messages now);
third-party AST transforms are safe (they run after the `GenericsVisitor` phase
op — verified); and since the follow-up, inner-class signatures of existing
code are unchanged.
>
> Two message nits: `Foo<String>.class` still gets "Cannot refer to a static
member of a generic type through a parameterization" (the class-literal path
missed the message pass — javac says "cannot select from parameterized type");
and `new T[n]` reports "generic array creation **of Object**", which names the
erasure rather than the type variable and reads like a different complaint.
>
> One API note: the follow-up stores the enclosing type as a `ClassNode`
field and no longer writes the `"outer.class"` node metadata — fine by me, just
noting that any external code reading that key (GROOVY-10646 era) now sees
`null`.
Most breaking changes seem fine - we need to add a table like above in
release notes and possibly in `core-object-orientation` §Generics. I am happy
to work on these separately. It is really only the first two rows which I
wonder about - spelt out below.
> Both workarounds verified on the PR install (dynamic and
`@CompileStatic`). Here are the two team-call rows as complete, paste-ready
samples — each with the code that works today, the exact PR error, and the
verified workaround.
>
> ## Team call 1 — `new T[n]` / `new T[]{…}` (generic array creation)
>
> **Works on ≤ 6.0.0-beta-3 (and 5.x, 4.x), dynamic and `@CS`; errors on the
PR:**
>
> ```groovy
> // a typed fixed-capacity stack — the kind of generic utility class where
this idiom shows up
> class Stack1<T> {
> private T[] items
> private int size = 0
> Stack1(int capacity) { items = new T[capacity] } // PR: generic
array creation of Object
> void push(T t) { items[size++] = t }
> T pop() { items[--size] }
> T[] snapshot() { new T[] { items[0] } } // PR: generic
array creation of Object
> }
> def s = new Stack1<String>(4)
> s.push('a'); s.push('b')
> assert s.pop() == 'b'
> ```
>
> On beta-3 both array creations quietly build an `Object[]` (erasure) and
everything runs. On the PR each line fails with `generic array creation of
Object` — note the message names the erasure, not `T`, which is the wording nit
in the draft comment.
>
> **Workaround (the Java idiom; verified OK on the PR build, both modes):**
>
> ```groovy
> class Stack2<T> {
> private T[] items
> private int size = 0
> Stack2(int capacity) { items = (T[]) new Object[capacity] } //
reifiable creation + unchecked cast
> void push(T t) { items[size++] = t }
> T pop() { items[--size] }
> }
> def s = new Stack2<String>(4)
> s.push('a'); s.push('b')
> assert s.pop() == 'b'
> ```
>
> The team question: is forcing that rewrite worth the Java parity, or do we
carve out bare type variables in `isReifiable` (keeping `new List<String>[n]`
rejected while `new T[n]` stays legal)?
>
> ## Team call 2 — generic class extending `Throwable`
>
> **Works on ≤ beta-3, dynamic and `@CS`; errors on the PR:**
>
> ```groovy
> // a DSL-style exception carrying a typed payload — the plausible
real-world shape
> class ValidationException<T> extends RuntimeException {
> final T details // PR: A
generic class may not extend java.lang.Throwable
> ValidationException(String msg, T details) { super(msg); this.details
= details }
> }
>
> try {
> throw new ValidationException<Map>('bad input', [field: 'name'])
> } catch (ValidationException<Map> ignored) { // note:
parameterized catch never made sense…
> // …
> } catch (ValidationException e) {
> assert e.details.field == 'name'
> }
> ```
>
> Java forbids this (JLS 8.1.2) because `catch` clauses can't be reified —
but Groovy's `catch` is erased anyway, so the class runs fine on the JVM; the
rejection is pure javac parity. That's why this is the cheapest row to *leave*
if the team wants to minimise breakage.
>
> **Workaround (verified OK on the PR build, both modes):**
>
> ```groovy
> class ValidationException extends RuntimeException {
> private final Object payload
> ValidationException(String msg, Object payload) { super(msg);
this.payload = payload }
> def <T> T payload() { (T) payload } // generics move from the
class to the accessor
> }
>
> try {
> throw new ValidationException('bad input', [field: 'name'])
> } catch (ValidationException e) {
> Map m = e.<Map>payload()
> assert m.field == 'name'
> }
> ```
>
> (A variant that keeps static typing without the explicit type witness —
implementing `Supplier<Map>` or exposing a concretely-typed field — also
verified fine.)
> Java compatibility: remaining generic type syntax
> -------------------------------------------------
>
> Key: GROOVY-12319
> URL: https://issues.apache.org/jira/browse/GROOVY-12319
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> Groovy already accepts most Java generics. Three Java forms still fail: two
> in the parser, one in {{GenericsVisitor}}. Each has a local workaround.
> Method type arguments such as {{Helper.<String>identity( x )}} already work
> and are not part of this request.
> h3. 1. Diamond {{<>}} on an anonymous class
> Java 9+ (JEP 213, JLS 15.9.5) allows diamond when creating an anonymous
> class, if the target type supplies the type arguments:
> {code:java}
> Processor<String> p = new Processor<>() {
> public String process(String val) { return val.toUpperCase(); }
> };
> {code}
> This should work in assignment, as a method argument, and under
> {{@TypeChecked}} / {{@CompileStatic}}, for both interfaces and abstract
> classes.
> *Actual:* Groovy rejects it with {{Cannot use diamond <> with anonymous inner
> classes}}.
> *Workaround:* write the type arguments explicitly:
> {code:java}
> Processor<String> p = new Processor<String>() {
> public String process(String val) { return val.toUpperCase(); }
> };
> {code}
> GROOVY-6730 and GROOVY-7159 were false-positive STC errors when diamond was
> _not_ used. They did not add this form.
> h3. 2. Qualified parameterized inner types ("rare" types)
> Java allows an inner type to keep the enclosing type's arguments (JLS 4.5):
> {code:java}
> class Outer<T> {
> class Inner<U> {}
> }
> Outer<String>.Inner<Integer> x = new Outer<String>().new Inner<Integer>("v",
> 1);
> {code}
> The same qualification appears as a field or method type, nested in another
> type argument, as a superclass when the member is a non-static class, and in
> a qualified instance creation inside the outer class:
> {code:java}
> List<Outer<String>.Inner<Integer>> list;
> class Sub extends Outer<String>.Inner {
> Sub(Outer<String> o) { o.super(); }
> }
> // inside Outer:
> new Outer<T>.Inner<U>(...)
> {code}
> Selecting a static member type from a parameterization is a compile-time
> error (JLS 4.5.2 / 6.5.5). Nested interfaces and enums are implicitly static
> (JLS 9.5), so a nested interface must be selected from the raw enclosing name:
> {code:java}
> class Impl implements Outer.Inner<Integer> { ... } // legal
> class Bad implements Outer<String>.Inner<Integer> { ... } // error
> {code}
> *Actual:* Groovy fails to parse {{Outer<String>.Inner}} ({{Unexpected
> input}}).
> *Workaround:* a factory that returns {{Inner}} without naming
> {{Outer<T>.Inner}}.
> h3. 3. Explicit type arguments on constructors, {{this()}} and {{super()}}
> Java allows constructor type arguments independently of the class type
> arguments (JLS 15.9 / 8.8.7.1):
> {code:java}
> class Box {
> <T> Box(T t) {}
> Box() { <String>this("x"); }
> }
> class Derived extends Box {
> Derived() { <String>super("y"); }
> }
> class Outer {
> class Inner {
> <T> Inner(T t) {}
> }
> }
> new <String>Box("x");
> new Outer().new <String>Inner("z");
> {code}
> *Actual:* Groovy fails at {{new <}} with {{Unexpected input: '<'}}.
> *Workaround:* inference, e.g. {{new Box("x")}}.
> Constructor type arguments are already tracked by GROOVY-10501. The
> {{this()}} / {{super()}} / inner-{{new}} forms are the same JLS production
> and should be handled together.
> h3. Expected
> All three forms compile in dynamic Groovy and under {{@TypeChecked}} /
> {{@CompileStatic}}, matching javac on well-formed programs.
> The well-formedness rules are javac's (checked against javac 25). Parsing a
> rare type does not make every use of it legal.
> These remain legal. {{Inner}} is a non-static member of {{Outer}};
> {{Outer<?>}} is reifiable (JLS 4.7), so a non-static member type of that
> enclosing type is reifiable (JLS 15.10.1):
> {code:java}
> class Outer<T> {
> class Inner {}
> class InnerG<U> {}
> interface Iface<U> { U id(U u); }
> }
> Outer<?>.Inner[] a = new Outer<?>.Inner[0];
> Outer<?>.InnerG<?>[] b = new Outer<?>.InnerG<?>[0];
> Outer<?>.Inner field;
> Outer<String>.Inner concrete;
> class Impl implements Outer.Iface<Integer> {
> public Integer id(Integer u) { return u; }
> }
> {code}
> These remain compile errors:
> {code:java}
> new Object<>() {} // diamond on a non-generic type
> class C extends ArrayList<> {} // diamond on a class declaration
> List<> list; // diamond on a field
> new <String>Box<>("x") // diamond combined with
> constructor type arguments
> Outer<String, Integer>.Inner x; // wrong arity
> x instanceof Outer<String>.Inner // parameterized type is not
> reifiable
> new Outer<String>.Inner[1] // generic array creation (JLS
> 15.10.1)
> new Outer<?>.Nested[0] // static member from a
> parameterized type (JLS 6.5.5)
> Outer<?>.Nested z; // same rule as a type name
> new Outer<?>.InnerG[0] // raw generic member of a
> parameterized enclosing type
> class Bad implements Outer<String>.Iface<Integer> {} // nested interface is
> implicitly static (JLS 9.5)
> new java.util.Map<?,?>.Entry[0] // Map.Entry is a nested interface
> new Outer<?>().new Inner() // constructor type argument may
> not be a wildcard
> {code}
> The {{Nested}} cases assume {{static class Nested}} inside {{Outer}}. The
> {{InnerG}} array error is the raw-member form; the legal counterpart is {{new
> Outer<?>.InnerG<?>[0]}} above.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
