paulk-asert commented on PR #3054:
URL: https://github.com/apache/groovy/pull/3054#issuecomment-6451570311
AI read:
> Thanks Eric, I think the direction is promising. The predicate gating on
`type.dim0()` holds up in practice. Untyped declarations, `Object o = {}`, and
closures with params, `->`, `;` or labels all stay closures, so the change only
kicks in where it should. For non-closure element types it's a clear win:
>
> | Code | master | PR |
> |---|---|---|
> | `String[] a = {}` | 1-element array holding the closure | `[]` |
> | `Boolean[] a = { false }` | `[true]` (closure → asBoolean) | `[false]` |
> | `final int[] a = {1}`, `int[] a = {1,}`, `Object[] a = {1, 2}` | fail |
work |
> | `@CompileStatic String[] s = { 'a' }` | STC error | works |
>
> The friction is where the element type can accept a closure, i.e. the
GROOVY-8561 territory from 2018 (PR 691 didn't get the three +1s back then).
Here are the edge cases I found, all run against your branch:
>
> **Behaviour changes for Closure/SAM/Object element types**
>
> | Code | master | PR |
> |---|---|---|
> | `Closure[] a = {}` | `[Closure]` | `[]` |
> | `def m(Closure[] cs = {})` | `[Closure]` | `[]` |
> | `Object[] o = {}` | `[Closure]` | `[]` |
> | `Closure[] a = { 1 }` | `[Closure]` | runtime failure |
> | `Closure[] a = { it }` | `[Closure]` | runtime failure (no `it`) |
> | `String[] s = { it }` | 1-element array holding the closure | runtime
failure |
> | `Runnable[] r = { println 'x' }` | `[Closure]` | compile error (command
chain not allowed in array initializer) |
> | `Closure[] a = { println('x') }` | `[Closure]` | **prints `x` at
declaration time and stores `[null]`** |
>
> The last one worries me most because it changes behaviour silently. Any
`Closure[]`/`Runnable[]`/`Callable[]` declaration whose single body expression
isn't a command chain now runs eagerly, and its result (often `null`) passes
the cast.
>
> **Body shape decides the meaning.** `Closure[] a = { it }` is an array but
`{ it -> it }` is a closure. `String[] s = { 'a' }` is an array but `{ 'a'; 'b'
}` and `{ 'a' }.call()` are closures. It's consistent with the grammar, but
hard to explain to users.
>
> **Nested braces crash the parser.** `Object[] o = { {} }` and `Closure[]
cs = { {}, {} }` fail with "General error during parsing: Cannot invoke
ClassNode.makeArray() because base is null". This already happens on master
with the long form (`new Object[]{{}}`), see the `//if (subType == null)
produce closure or throw exception` TODO in `visitArrayInitializer`. Your
branch just makes it much easier to hit. It also blocks the escape hatch 8561
documented (`Closure[] fns = { { } }`). Taking the "produce closure" branch of
that TODO would fix the crash and restore the escape hatch.
>
> **Other bits**
> - Only a single declarator is handled: `int[] a = {1}, b = {2}` (valid
Java) falls through to the closure path and fails at runtime with a cast error.
> - `def m( {` now reports the missing `)` at line 2 col 2 instead of at the
`{` (line 1 col 8). That comes from the `formalParameter` restructuring.
> - `String[] a = { ['a', 'b'] }` gives `["[a, b]"]` (length 1, element is a
`String`) in both dynamic and `@CompileStatic`: the list is coerced via
toString. The long form `new String[]{ ['a','b'] }` does exactly that on master
too, so it isn't new, but the terse form makes it easier to write, and Java
would reject it.
>
> **Possible paths forward**
> - **A. Keep the current semantics (Java-faithful).** This has the simplest
rule but is a breaking change for Closure/SAM/Object arrays, including the
silent eager-evaluation case. We'd need a dev-list decision (given the 8561
precedent), a COMPATIBILITY note, and ideally a compile-time diagnostic when a
Closure/SAM-typed array element isn't a closure/lambda/method ref. That
diagnostic can't be exact in dynamic code, e.g. `{ getHandler() }` is
legitimate.
> - **B. Type-aware fallback (my preference).** Use the array reading,
except when the initializer is empty or has a single element *and* the leaf
component type is `Closure`, a SAM type, or (TBD) `Object`. In that case keep
today's closure promotion. Multi-element and nested initializers would always
be arrays. Nothing that works today breaks, and Java users get the forms they
actually paste (`String[]`, `int[]`, `int[][]`, …). The catch is that SAM-ness
is only known after resolution. AstBuilder could build both readings for the
empty/single-element shapes and pick one post-resolution. I'm not sure where
that hook best lives, and I'm curious what you think.
> - **C. Only shapes no closure body can parse as.** Treat the braces as an
array only when there's a top-level comma or nested braces. It's cheap and
non-breaking, but `String[] s = {}`, probably the most common pasted Java
idiom, would keep today's odd result, so it doesn't really meet the ticket's
goal.
>
> Whichever way we go, fixing the nested-brace NPE seems worthwhile on its
own, since it's a crash on master today. Tests pinning down the chosen
semantics for `{}`, `{ it }`, `{ -> }`, `{ { } }`, `Runnable[] r = { … }` and
`Object[] o = {}` would also help.
--
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]
