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.)
--
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]
