[
https://issues.apache.org/jira/browse/GROOVY-12488?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18132167#comment-18132167
]
ASF GitHub Bot commented on GROOVY-12488:
-----------------------------------------
paulk-asert commented on PR #3022:
URL: https://github.com/apache/groovy/pull/3022#issuecomment-6408480699
Thanks Scott, this is a thorough piece of work and the core change is right:
under static compilation, X::new now selects its constructor the way `new
X(args)` and every other method reference already do. I read the full compiler
and runtime diff. Three things before merge, then a suggestion on how to carve
it up.
**1. Documentation.** The behaviour changes in the description (overload
chosen by the SAM's static types, Object[] to varargs as one element, NPE for
null to a primitive, visible-constructor selection, the EMC constructor
override bypass) are method-resolution changes, which COMPATIBILITY.md classes
as breaking. They are the right semantics for @CompileStatic, but they need a
"Groovy 7" entry there, plus a sentence in the constructor-reference section of
core-operators.adoc. Same for the new compile error on `new` of a protected
constructor from another package in a subclass: dynamic Groovy allows that, so
it is a real Groovy/Java difference that SC now closes. Worth a line for
native-image users too: the fallback cases (enum, protected constructor of
another package, ambiguous overload, anything a GroovyClassLoader defines at
run time) still need reflection metadata.
**2. Scope.** The constructor-reference fix needs the run-time-package guard
for constructors, since without it a direct call would regress a GroovyShell
script referencing a package-private constructor from an earlier parse.
Everything beyond that is fixing pre-existing bugs, some unrelated to
constructor references. I'd like those under their own JIRAs even if the
commits stay here, and the BroadcastChannel, test de-flaking and LogTest
changes moved to a separate PR. Suggested grouping:
A. Constructor references, Groovy 7 and the 6.0.1 backport Grails wants:
d5395be, 9f9124f, 8fba262, f60c6e4 (the ACC_FINAL fix is JVMS 4.6 and
hits every helper method in a @CompileStatic interface, so it belongs here),
f35e725, 4169695, a358ba1, f85f7e2, 50a8e6e, 51893ae, the constructor slice of
0b89f6c (GroovyClassLoader marker, isInSameRuntimePackage,
StaticInvocationWriter.isDirectlyAccessible and its use in the method-reference
writer), and the constructor slice of 932d9e6. c854974 is superseded by
GROOVY-12454 from master: its production change is gone from the net diff and
only testFunctionCN25 remains, so keep the test and drop the rest.
B. The same run-time-package rule applied to `new`, methods, fields and
method references, so 7.0 doesn't ship a rule that applies only to
constructors. One new JIRA:
19f8427, c4b5728, 41c24c6, 1865f19, 3bf8589, the remainder of 0b89f6c and
932d9e6, a4ef7fb, plus the protected-member diagnostics fe92a5f and 8c338c3 and
the Selector change 94895f3 (all three turn VerifyError or GroovyBugError into
a compile error or the meta-class path, so no working code is affected).
C. Extras, each its own JIRA and PR:
0e55d32 (package-private classes of another class loader: this widens
inferred types in StaticTypesTypeChooser and follows through the call-site
writer, for-each, spread and method-pointer paths, which is the broadest
codegen change in the PR), b4b1b63 (MetaClassImpl super dispatch; this affects
all dynamic code in any split-package setup, not just scripts), a270edd
(BroadcastChannel publisher after close; a shipped 6.0.0 API, and it conflicts
on GROOVY_6_0_X), bce20df and 99ad290 (sleep-to-await test changes), 35346fd
(LogTest). The merge commit d64a720 goes away on squash.
**3. Backport.** Group A is contained and well tested; I'd take it to 6.0.1
after a heads-up on the dev list, since it changes overload selection in a
point release. Groups B and C should soak on master first.
Two smaller notes: the protected-constructor error is raised from
StaticInvocationWriter during class generation rather than by the type checker,
so @TypeChecked code is silent and works dynamically; tolerable, but worth a
comment. And GroovyClassLoader marks every parseClass compilation as defined in
memory regardless of the collector, so a subclass that defines classes into the
loader itself will take the meta-class path where direct access would work; not
a failure, but a sentence in the javadoc would help the next reader.
> SC: constructor reference calls the constructor through the meta-class
> instead of directly
> ------------------------------------------------------------------------------------------
>
> Key: GROOVY-12488
> URL: https://issues.apache.org/jira/browse/GROOVY-12488
> Project: Groovy
> Issue Type: Improvement
> Components: Static compilation
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> Under {{@CompileStatic}}, a constructor reference such as {{Made::new}} is
> implemented by a synthetic method {{ctorRef$<method>$<n>}} on the enclosing
> class, which the LambdaMetafactory call site targets. That method is not
> statically compiled:
> {noformat}
> private static final Made ctorRef$ctorRef$0();
> 0: ldc class Made
> 2: invokedynamic init:(Ljava/lang/Class;)Ljava/lang/Object;
> 7: invokedynamic cast:(Ljava/lang/Object;)LMade;
> 12: areturn
> {noformat}
> So every call selects the constructor through the meta-class at run time,
> while other method references ({{String::length}}, {{Math::abs}}) link their
> target directly. In a GraalVM native image the constructor also needs
> reflection metadata although the code is statically compiled.
> The cause is
> {{StaticTypesMethodReferenceExpressionWriter.addSyntheticMethodForConstructorReference}}:
> it builds the constructor call without a target constructor and sets
> {{STATIC_COMPILE_NODE}} to false (there is a TODO for it).
> Proposed fix: select the constructor at compile time among the constructors
> visible from the enclosing class, matched against the functional interface's
> parameter types as for a constructor call expression, and call it directly.
> Keep the run-time selection when no single constructor matches
> (GROOVY-10971), for a protected constructor in another package, and for a
> protected or package-private constructor of a class compiled in another
> compilation unit, which may come from another class loader. As with other
> method references and with Java, the constructor is then chosen from the
> declared parameter types rather than the run-time argument types.
> PR: https://github.com/apache/groovy/pull/3022
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
