[
https://issues.apache.org/jira/browse/GROOVY-12490?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18131901#comment-18131901
]
ASF GitHub Bot commented on GROOVY-12490:
-----------------------------------------
codeconsole commented on PR #3024:
URL: https://github.com/apache/groovy/pull/3024#issuecomment-6400630045
Pushed 9d2a811ee9. Replies in the order of the review:
1. Kept, documented and tested in 6190c30aaf. The asymmetry is the
runtime's. With indy, the cast to `Boolean` returns `null` as is
(`CastSelector.handleBoolean`) but calls `asBoolean` through the meta class on
any other value, so inside a category that overrides `asBoolean(Boolean)`,
`$INIT` passes `false` for `true`. A direct `true` would diverge there; a
direct `null` cannot. A `boolean` parameter unboxes without `asBoolean`, so
`true` and `false` go direct there. `Boolean.TRUE` is a property expression,
not a literal, so it keeps `$INIT`.
`testBooleanArgumentsFollowTheCastOfTheRuntime` checks the path for each
combination, and `testBooleanArgumentsUnderACategoryThatOverridesAsBoolean` the
values; allowing `true` for `Boolean` fails both, rejecting `null` fails the
first.
2. Fixed in 083bd6b24f, in both places: `completeEnum` had the same cast, so
a guard in `canInitDirectly` alone would only have moved the
`ClassCastException` (master throws it there too). A non-list initial value is
now the only argument, as `EnumHelper.addEnumConstant` treats it, and then
follows the usual rules. The `testOnlyArgumentOutsideAList*` tests use a parser
plugin that leaves the argument unwrapped; they fail without the change.
3. Changed in 001ced2e4d: a varargs constructor now disables direct init
only where it can win. `calculateParameterDistance` adds at least `1 << 44` to
a varargs call, which outweighs the distance of the matching-arity constructor,
except when the varargs constructor has the call's arity and the last argument
is `null` (no penalty: master picks `E(String, Object...)` over `E(String,
Integer)` for `('two', null)`). That case, and constants that match only as
varargs, keep `$INIT`, with a comment on the `return null`. A 912-enum varargs
matrix (dynamic and `@CompileStatic`, indy and classic) matches master value
for value.
`testVarargsConstructorBesideMatchingConstructorCallsConstructorDirectly`,
`testVarargsConstructorForAnyArgumentsBesideMatchingConstructorCallsConstructorDirectly`,
`testNullLastArgumentForAVarargsConstructorOfTheSameArityUsesInitHelper`,
`EnumTest.testLiteralArgumentsBesideAVarargsConstructor`.
4. Fixed in e50ac46974: the class generator writes the call through
`visit(GroovyCodeVisitor)`, which passes itself to `write(mv, generator)`, and
no field holds it. `visit(MethodVisitor)` still writes a constant without
arguments and throws a `GroovyBugError` instead of an NPE for one with
arguments, since only the generator can load those (emitting the `$INIT` call
needs it too). `testOnlyTheClassGeneratorWritesTheArgumentsOfAConstant`.
5. Fixed in e50ac46974: any other visitor visits `initCall` again. Such
visitors exist: the AST browser and `AstNodeToScriptAdapter` run after the
class generator at the Class Generation phase. They showed
`/*BytecodeExpression*/` at 48a40f5b6b and show `E.$INIT('ONE', 0, 'one')`
again, as on master.
`testOtherVisitorsSeeTheInitCallsThatTheDirectCallsReplace`.
6. Added to the Javadoc of `substitute` in 0719570757: transforms that
rewrite `$INIT` decide the path, and so whether the enum needs constructor
reflection, in both directions
(`testArgumentAddedByATransformToEveryConstantCallsConstructorDirectly`,
`testInitCallReplacedByATransformKeepsTheEnumOnTheEmittedCalls`).
7. Spelled out in the class Javadoc (0719570757) and the PR description:
enums whose constants all have literal arguments, which always went through
`$INIT` before, are now created out of a meta class's sight too. Their opt-out
is one non-literal argument, such as `'one'.toString()` for `'one'`
(`testNonLiteralArgumentKeepsConstantCreationOpenToMetaClassInterception`).
8. `box()` returns at once for a reference type (OperandStack.java:233).
`testClassLiteralArgumentsCallConstructorDirectly` (6997620fe3) pins `LDC
String.class`, `LDC Integer.class` and `GETSTATIC Integer.TYPE`, each followed
directly by `INVOKESPECIAL`.
Test gaps:
- `Boolean`/`null`: item 1 (6190c30aaf).
- `ListExpression` guard: item 2 (083bd6b24f).
- Class literal: item 8 (6997620fe3).
- `Character`/`char`: added in 6997620fe3. Groovy has no char literal, so a
transform adds a `Character` constant. It goes direct to `char` and
`Character`, and keeps `$INIT` for `int`, where the runtime finds no
constructor rather than widening (`testCharacterConstantArguments`,
`EnumTest.testCharacterConstantArgumentsAddedByATransform`).
- Static compiler's `$INIT` form:
`testInitCallsInTheFormOfTheStaticCompilerCallConstructorDirectly` (6997620fe3)
asserts the `MethodCallExpression` form just before class generation and the
direct bytecode;
`testTransformsSeeTheInitCallsOfConstantsThatAreCreatedDirectly` asserts the
`StaticMethodCallExpression` form of a dynamic enum. Disabling either branch of
`initArguments` fails them.
Nits:
- `transformExpression`: no change.
- Named in 9d2a811ee9:
`org.codehaus.groovy.classgen.EnumConstantInit.constantAssignments`, like the
`ClosureWriter` keys.
- `setSourcePosition(initCall)`: still in place.
- By design; stated in the Javadoc of `substitute` (0719570757). With item
5, visitors that run after the class generator see the `$INIT` calls too.
Without the change, 26 tests fail: 24 of the 50 new tests and the 2
rewritten ones. The matrices in the description were re-run on this head with
indy and with classic call sites; the values and exceptions match master apart
from closure identity hashes.
> Enum constants with literal arguments still select their constructor
> reflectively through $INIT
> -----------------------------------------------------------------------------------------------
>
> Key: GROOVY-12490
> URL: https://issues.apache.org/jira/browse/GROOVY-12490
> Project: Groovy
> Issue Type: Improvement
> Components: Compiler
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> GROOVY-12240 made enum constants without arguments call the {{(String,int)}}
> constructor directly. Every constant with arguments still goes through the
> synthetic {{$INIT(Object[])}} helper, whose body is a spread {{this(*para)}}:
> {{MetaClassImpl}} selects the constructor by reflecting over the declared
> constructors. {{@CompileStatic}} does not change that, since {{$INIT}}'s own
> body stays dynamic.
> As a result, an ordinary enum fails to initialize in a GraalVM native image
> built without reflection metadata for it:
> {code:groovy}
> enum AccessModifier {
> PRIVATE('private'), PROTECTED('protected'), PUBLIC('public')
> final String keyword
> AccessModifier(String keyword) { this.keyword = keyword }
> }
> {code}
> As written, the enum fails at the dynamic call to {{$INIT}} itself:
> {noformat}
> groovy.lang.MissingMethodException: No signature of static method: $INIT for
> class: AccessModifier is applicable for argument types: (String, Integer,
> String)
> {noformat}
> With {{@CompileStatic}} on the enum, {{$INIT}} is called with
> {{invokestatic}}, and the constructor selection in its body fails instead:
> {noformat}
> groovy.lang.GroovyRuntimeException: Could not find matching constructor for:
> AccessModifier(String, Integer, String)
> {noformat}
> Proposed fix: call the constructor directly when the compiler can prove what
> the runtime would do: every argument is a literal, exactly one non-synthetic
> constructor has the arity and none could take the arguments as varargs, and
> each literal reaches its parameter unchanged ({{Boolean}} parameters
> excluded). Everything else keeps {{$INIT}}, and the decision stays per enum,
> as in GROOVY-12240.
> PR: https://github.com/apache/groovy/pull/3024
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
