[
https://issues.apache.org/jira/browse/GROOVY-12544?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18134081#comment-18134081
]
Paul King commented on GROOVY-12544:
------------------------------------
*Fix:* [PR #3077|https://github.com/apache/groovy/pull/3077]
* When the compiler reads a nested class from its class file, it now keeps the
class's own {{InnerClasses}} entries for itself and its enclosing classes.
Those entries are written into any class that refers to it.
* For class nodes built by reflection, the same entries come from
{{Class.getDeclaringClass()}}. For JDK member classes and Groovy's synthetic
trait helpers, {{Class.getModifiers()}} matches the flags in the class file.
* {{ClassNode.getOuterClass()}} is unchanged and still returns {{null}} for
classes read from the class path. Name resolution and type checking are not
affected; only the generated bytecode changes.
*Results:* a class or script compiled together with the nested classes it uses
now produces the same bytes as when it is compiled against them as pre-compiled
classes. This covers a script body referring to a nested class, a class using a
nested class declared in a script file, two levels of nesting ({{A$B$C}}), and
JDK nested classes such as {{AbstractMap.SimpleEntry}}. Classes declared
alongside a script body in a script file are top-level siblings, so they need
no entry and no special handling.
In the Gradle project from GROOVY-10932
(https://github.com/jdaugherty/groovy-trait-incremental-compile), this change
together with the GROOVY-10932 fix makes the incrementally recompiled
{{Thing.class}} byte-identical to the full build. The downstream
{{:app:compileGroovy}} then stays up to date.
*Performance:* compiling 600 classes against pre-compiled traits shows no
difference outside run-to-run noise.
*Scope:* entries are added at the same places where Groovy already writes them
for nested classes in the same compilation unit, i.e. where a class is referred
to directly. javac additionally lists nested types that appear only in method
signatures, for example {{Map$Entry}} as the return type of {{Map.entry}}. That
would be a separate, larger change.
Since this changes the bytes of most classes that refer to a nested type, it is
intended for master only. An option would be to make it opt-in on 6:
bq. There's a single gating point, and 6.0.x has an established pattern for
incubating switches like this one.
bq. Why it's easy:
bq. - The decompiler changes don't affect output. AsmDecompiler, ClassStub and
DecompiledClassNode are identical on 6.0.x and master, so that part of PR #3077
would cherry-pick as-is. Recording the InnerClasses chain changes nothing about
what gets generated.
bq. - Only one method changes the bytecode. That's
AsmClassGenerator.maybeInnerClassEntry, which is the same on 6.0.x as it was on
master before the PR. Gating the two new branches there leaves the default
output byte-for-byte what 6.0.x produces today.
bq. Two ways to switch it on:
bq. 1. System property, following the GEP-27 switches on 6.0.x
(CompilerConfiguration.CLOSURE_PACKING = "groovy.target.closure.pack", read
with SystemUtil.getBooleanSafe(...)):
bq. } else if
(SystemUtil.getBooleanSafe(CompilerConfiguration.PRECOMPILED_INNER_CLASS_ENTRIES))
{
bq. // the two new branches
bq. }
bq. It's the simplest option, but it's JVM-wide. Gradle runs the Groovy
compiler in a forked worker, so users would set it via
groovyOptions.forkOptions.jvmArgs, and a property set that way may not reliably
invalidate cached build outputs when toggled.
bq. 2. Optimization option, following asmResolving/classLoaderResolving, read
from config.getOptimizationOptions() once per class in visitClass. In Gradle
that's groovyOptions.optimizationOptions['…'] = true. It's set per compile task
and is a declared task input, so toggling it correctly invalidates up-to-date
checks and the build cache. That makes it the better fit for the
incremental-build users this is aimed at.
bq. I'd do the optimization option, possibly with the system property as a
fallback like groovy.target.*. The name is your call; something like
innerClassEntries.
> InnerClasses attribute omits nested classes that are not in the current
> compilation unit
> ----------------------------------------------------------------------------------------
>
> Key: GROOVY-12544
> URL: https://issues.apache.org/jira/browse/GROOVY-12544
> Project: Groovy
> Issue Type: Improvement
> Components: Compiler
> Affects Versions: 4.0.33, 5.1.3, 6.0.0
> Reporter: Paul King
> Priority: Major
> Fix For: 7.x
>
>
> groovyc writes an {{InnerClasses}} entry for a referenced nested class only
> when that class is in the same compilation unit. A nested class read from the
> classpath, whether a pre-compiled Groovy class or a JDK class such as
> {{Map.Entry}}, gets no entry. javac writes an entry for every nested class
> referenced from the class.
> h3. Reproducer
> {code:groovy}
> // A.groovy
> class A { static class B {} }
> // C.groovy
> class C { def m() { new A.B() } }
> {code}
> {noformat}
> groovyc -d together A.groovy C.groovy
> groovyc -d first A.groovy
> groovyc -d second -cp first C.groovy
> {noformat}
> {{together/C.class}} contains:
> {noformat}
> InnerClasses:
> public static #34= #31 of #33; // B=class A$B of class A
> {noformat}
> {{second/C.class}} has no {{InnerClasses}} attribute. The extra constant-pool
> entries in the first case also shift later indices, so instructions can
> change too, e.g. {{ldc}} becomes {{ldc_w}}.
> For comparison, javac compiling
> {code:java}
> class Y { Object m(java.util.Map.Entry e) { return new
> java.util.AbstractMap.SimpleEntry(1, 2); } }
> {code}
> writes entries for both {{AbstractMap$SimpleEntry}} and {{Map$Entry}}. The
> equivalent Groovy class has none.
> h3. Why it matters
> * *Reproducible output:* compiling an unchanged source file gives different
> bytes depending on what else is in the compilation unit. Incremental builds
> recompile a subset of sources against the previous output, so unchanged
> classes come out different.
> * *Downstream recompilation:* in the Gradle project from GROOVY-10932
> (https://github.com/jdaugherty/groovy-trait-incremental-compile), with the
> GROOVY-10932 fix applied, the missing {{InnerClasses}} entry for a trait
> helper is the only remaining difference in the implementer. It is still
> enough to make the downstream project recompile. A control run where the
> class came out byte-identical left the downstream compile up to date. Trait
> helpers are only one case: any class referencing a nested type behaves the
> same way.
> * *Spec:* JVMS §4.7.6 expects every nested class referenced from the constant
> pool to be listed in {{InnerClasses}}.
> Runtime behaviour does not appear to be affected. Reflection on a nested
> class ({{getDeclaringClass}}, {{getSimpleName}}, ...) reads that class's own
> {{InnerClasses}} attribute, not the attribute of the classes that reference
> it.
> h3. Scripts
> Scripts behave the same way. A script body that references a nested class
> gets an {{InnerClasses}} entry only when that class is in the same
> compilation unit. Classes declared alongside the script body in a script file
> are top-level siblings rather than nested classes, so they need no entry and
> do not drift. Classes nested inside those siblings do drift, like any other
> nested class. No special handling for scripts is needed.
> h3. Possible approach
> * *Decompiler:* {{AsmDecompiler}} already reads a class's self-referencing
> {{InnerClasses}} entry to get its modifiers. It could also keep the outer
> class name and simple name in {{ClassStub}}.
> * *Class generator:* {{AsmClassGenerator.maybeInnerClassEntry}} currently
> writes an entry only when {{getOuterClass() != null}}, which holds only for
> {{InnerClassNode}}s. It could also write one for a member class read from the
> classpath, recursing through enclosing classes as {{makeInnerClassEntry}}
> does today (GROOVY-9842). For class nodes built by reflection,
> {{Class.getDeclaringClass()}} provides the same information.
> * *Keep {{getOuterClass()}} unchanged:* it should keep returning {{null}} for
> classpath classes. Name resolution, static type checking and access checks
> rely on it, so the change should stay confined to bytecode generation.
> h3. Compatibility
> This changes the bytes of most class files that reference a nested type,
> including commonly used JDK types such as {{Map.Entry}}. Behaviour is not
> expected to change, but builds comparing outputs across versions will see
> differences. I suggest master only, with a note in the release notes.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
