[
https://issues.apache.org/jira/browse/GROOVY-11492?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18133191#comment-18133191
]
ASF GitHub Bot commented on GROOVY-11492:
-----------------------------------------
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.
> Java compatibility: array initializers without "new Type[]"
> -----------------------------------------------------------
>
> Key: GROOVY-11492
> URL: https://issues.apache.org/jira/browse/GROOVY-11492
> Project: Groovy
> Issue Type: Improvement
> Components: parser-antlr4
> Reporter: Eric Milles
> Assignee: Eric Milles
> Priority: Major
> Labels: arrays
>
> There are a few cases where bare array initializers are not yet supported:
> # {{int[][] array = \{ \{1,2\}, \{3,4\} \};}}
> # {{@Target(\{ ElementType.FIELD, ElementType.METHOD \})}}
> # {code:groovy}@interface A {
> String[] strings() default {}
> }
> {code}
> These were implemented as part of [1] but the PR was closed. It may be worth
> looking back over what was done and re-implementing one or more of these
> cases. It was noted that {{Closure[] array = \{\};}} is one of the edge
> cases that leads to ambiguity.
> [1] https://github.com/apache/groovy/pull/691
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
