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.
--
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]
