[
https://issues.apache.org/jira/browse/GROOVY-12240?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18102941#comment-18102941
]
ASF GitHub Bot commented on GROOVY-12240:
-----------------------------------------
codeconsole opened a new pull request, #2772:
URL: https://github.com/apache/groovy/pull/2772
https://issues.apache.org/jira/browse/GROOVY-12240
### Motivation
`EnumVisitor` creates every enum constant through a synthetic helper:
```groovy
def $INIT(Object[] para) {
return this(*para)
}
```
`this(*para)` is a spread constructor call, so
`InvocationWriter.makeDirectConstructorCall` refuses it — it bails on
`SpreadExpression`, and again on `!controller.isConstructor()` — and the body
compiles to `ScriptBytecodeAdapter.despreadList` plus
`selectConstructorAndTransformArguments`. The meta class then picks the
constructor at run time by reflecting over `getDeclaredConstructors()`. The
static initializer reaches `$INIT` itself through a dynamic call site.
For `enum Colors { RED, GREEN, BLUE }` that is the whole constant-creation
path, even though the only arguments are the compiler-supplied name and
ordinal, both known at compile time.
Where reflection over the enum isn't available the class simply cannot
initialize. In a GraalVM native image built without reachability metadata for
the enum, `getDeclaredConstructors()` returns nothing and class initialization
throws
```
groovy.lang.GroovyRuntimeException: Could not find matching constructor for:
com.example.MyEnum(String, Integer)
```
(note the boxed `Integer` — the ordinal has been through `Object[]`). This
kills the application in a static initializer before any user code runs.
`@CompileStatic` does not help: Groovy already compiles the *call site*
statically (`StaticTypeCheckingVisitor`, GROOVY-10845); it is `$INIT`'s own
body that is necessarily dynamic.
### Change
When **every** constant of an enum is a plain identifier, the arguments are
provably `[name, ordinal]`, and the static initializer now calls the enum's
`(String,int)` constructor directly.
```
static {}; static {};
0: ldc // class Colors 0: new // class
Colors
2: ldc // String RED 3: dup
4: iconst_0 4: ldc // String RED
5: invokestatic Integer.valueOf 6: iconst_0
8: invokedynamic invoke:(Class;String; 7: invokespecial
"<init>":(Ljava/lang/String;I)V
Integer;)Object; 10: putstatic Field
RED:LColors;
13: invokedynamic cast:(Object;)LColors;
18: putstatic Field RED:LColors;
```
The same shape javac emits for a Java enum: 21 bytes and two indy call sites
per constant become 13 bytes and none.
### When the new path applies
Only when all of the following hold:
- every constant is a plain identifier — no arguments, no named arguments,
no class body;
- the enum is not abstract and has no `EnumConstantClassNode` inner classes;
- the enum declares no constructor, or declares one callable with no
user-supplied argument;
- and, checked at bytecode generation once every transform has run, a
`(String,int)` constructor actually exists.
Anything else keeps the existing `$INIT` path.
### How the fallback works
`EnumConstantInit` is a `BytecodeExpression` that holds the original `$INIT`
call. It hands that call to every visitor except `AsmClassGenerator`, and hands
it to `AsmClassGenerator` too when the expected constructor isn't present. So
type checking, scope resolution and AST transforms see exactly the tree they
see today, and a shape that cannot use the direct call degrades to today's
bytecode rather than to a different failure.
A concrete case: `@TupleConstructor(defaults = false) enum E { ONE; String
value }` compiles today and fails at class initialization with `Could not find
matching constructor`. It has no `(String,int)` constructor, so it keeps
`$INIT` and keeps failing in exactly that way. There is a test for it.
`$INIT` is unchanged and still generated for every enum.
### Scope, stated honestly
This is a **compile-time** change. It only helps code compiled by a Groovy
that carries the fix; bytecode already compiled by an earlier Groovy keeps its
`$INIT` path whichever Groovy runs it. I confirmed this by building a GraalVM
native image of an application against a patched Groovy: the framework's own
enums, compiled by an earlier Groovy, still failed with `Could not find
matching constructor` until reachability metadata was restored.
Enums whose constants take arguments (`RED(255, 0, 0)`) are not addressed
and still require reachability metadata in a native image. Fixing those would
mean relaxing `InvocationWriter.makeDirectConstructorCall` to work outside a
constructor, which is a much wider change and deliberately left alone.
One semantic narrowing worth reviewer attention: a plain enum's `<clinit>`
no longer touches the meta class, so anything relying on intercepting an enum
constructor via `ExpandoMetaClass` before class initialization would no longer
see it. I believe this is unreachable in practice — the enum is initialized
once, before any such hook could be installed, and Java enums offer no
equivalent — but it is a real change.
### Tests
- `EnumConstantInitBytecodeTest` (new) — asserts the emitted `<clinit>`
instruction sequence for the direct case, that `$INIT` is still generated with
its usual body, and that constants with arguments / named arguments / a body /
a mix, and the missing-constructor case, all keep `$INIT`.
- `gls.enums.EnumTest` — behaviour coverage for the new path: values,
ordinals, `valueOf`, `next`/`previous`, `MIN_VALUE`/`MAX_VALUE`, ranges,
`EnumSet`, `compareTo`, serialization identity, and enums with an explicit
no-arg or all-defaults constructor.
Also verified by hand across packaged, nested and doubly-nested enums,
`@CompileStatic`, `@TypeChecked`, and a 400-constant enum (correct
`iconst`/`bipush`/`sipush` selection at 5/6/127/128/399).
`./gradlew :test` passes in full: 16,548 tests, 0 failures.
Related to GROOVY-12234.
> Initialize argument-less enum constants with a direct constructor call
> ----------------------------------------------------------------------
>
> Key: GROOVY-12240
> URL: https://issues.apache.org/jira/browse/GROOVY-12240
> Project: Groovy
> Issue Type: Improvement
> Components: Compiler
> Affects Versions: 5.0.8
> Reporter: Scott Murphy Heiberg
> Priority: Minor
>
> {{EnumVisitor}} creates every enum constant through a synthetic helper:
> {code:groovy}
> def $INIT(Object[] para) {
> return this(*para)
> }
> {code}
> {{this(*para)}} is a spread constructor call, so
> {{InvocationWriter.makeDirectConstructorCall}} refuses it — it bails on
> {{SpreadExpression}}, and again on {{!controller.isConstructor()}} — and the
> body compiles to {{ScriptBytecodeAdapter.despreadList}} plus
> {{selectConstructorAndTransformArguments}}. The meta class then picks the
> constructor at run time by reflecting over {{getDeclaredConstructors()}}. The
> static initializer reaches {{$INIT}} itself through a dynamic call site.
> For {{enum Colors { RED, GREEN, BLUE }}} that is the entire constant-creation
> path, even though the only arguments are the compiler-supplied name and
> ordinal, both known at compile time.
> Where reflection over the enum is not available, the class cannot initialize
> at all. In a GraalVM native image built without reachability metadata for the
> enum, {{getDeclaredConstructors()}} returns nothing and class initialization
> throws:
> {noformat}
> groovy.lang.GroovyRuntimeException: Could not find matching constructor for:
> com.example.MyEnum(String, Integer)
> {noformat}
> (note the boxed {{Integer}} — the ordinal has been through {{Object[]}}).
> This kills the application in a static initializer before any user code runs.
> {{@CompileStatic}} does not help. Groovy already compiles the *call site*
> statically (StaticTypeCheckingVisitor, GROOVY-10845); it is {{$INIT}}'s own
> body that is necessarily dynamic.
> h3. Proposal
> When every constant of an enum is a plain identifier, the arguments are
> provably {{[name, ordinal]}}, and the static initializer can call the enum's
> {{(String, int)}} constructor directly:
> {noformat}
> static {}; static {};
> 0: ldc // class Colors 0: new // class Colors
> 2: ldc // String RED 3: dup
> 4: iconst_0 4: ldc // String RED
> 5: invokestatic Integer.valueOf 6: iconst_0
> 8: invokedynamic invoke:(Class;String; 7: invokespecial
> "<init>":(Ljava/lang/String;I)V
> Integer;)Object; 10: putstatic Field
> RED:LColors;
> 13: invokedynamic cast:(Object;)LColors;
> 18: putstatic Field RED:LColors;
> {noformat}
> The same shape javac emits for a Java enum: 21 bytes and two invokedynamic
> call sites per constant become 13 bytes and none.
> Anything else — constants with arguments, named-argument form, anonymous
> constant bodies, or a mixture — keeps the existing {{$INIT}} path, as does
> any enum lacking a {{(String, int)}} constructor (checked at bytecode
> generation, after {{Verifier}} has run). {{$INIT}} itself is unchanged and
> still generated for every enum.
> h3. Scope, stated honestly
> This is a *compile-time* change. It only helps code compiled by a Groovy that
> carries the fix; bytecode already compiled by an earlier Groovy keeps its
> {{$INIT}} path whichever Groovy runs it. Confirmed by building a GraalVM
> native image of an application against a patched Groovy: the framework's own
> enums, compiled by an earlier Groovy, still failed with {{Could not find
> matching constructor}} until reachability metadata was restored.
> Enums whose constants take arguments ({{RED(255, 0, 0)}}) are not addressed
> and still require reachability metadata in a native image. Fixing those would
> mean relaxing {{InvocationWriter.makeDirectConstructorCall}} to work outside
> a constructor, which is a much wider change.
> Related to GROOVY-12234.
> I have a patch with tests ({{./gradlew :test}} passes in full: 16,548 tests,
> 0 failures) and will open a pull request against this issue.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
