[
https://issues.apache.org/jira/browse/GROOVY-12751?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18141550#comment-18141550
]
ASF GitHub Bot commented on GROOVY-12751:
-----------------------------------------
paulk-asert commented on PR #3284:
URL: https://github.com/apache/groovy/pull/3284#issuecomment-6752330880
PR #3284 is solid overall, but one gap matters before merge: Java sources in
a later compilation can't see classes an earlier compilation published.
All 12 of the PR's `MemClassOutputPublicationTest` tests pass on the PR head
(c61a950f18). I did not run the full suite.
**1. Published classes are invisible to javac (confirmed, should fix or
document before merge).** When javac builds its classpath
(`JavacJavaCompiler.makeParameters`), it skips the in-memory URL.
`InMemoryJavaFileManager` passes classpath lookups straight to the standard
file manager, so nothing gives javac the published bytes. Groovy code in the
later unit resolves them fine, through the class loader.
I confirmed this with a scratch test:
- Unit 1 compiles `p/Lib.java` and `p/G.groovy`, then gets published.
- Unit 2 compiles `q/Use.java`, which calls `p.Lib.n() + p.G.g()`.
- javac fails with `error: package p does not exist`.
The PR's tests miss this because every second-unit Java source is either
standalone or a replacement for an earlier class. Since the whole point of
GROOVY-12751 is that a later compilation can use the earlier one's output, this
should either work or be stated plainly as a limitation. Making it work means
the file manager would list and serve published classes for `CLASS_PATH`.
**2. `AsmDecompiler` now re-parses class files for many users who never
enable the option.** `cacheableScheme` caches only `file`, `jar` and `jrt`
URLs. Before this PR, URLs of every scheme were cached. So class files on OSGi
(`bundleresource:`), JBoss (`vfs:`) and WebSphere (`wsjar:`) classpaths are now
parsed again on every lookup. Only the in-memory protocol needs to skip the
cache, so the check should exclude `x-groovy-mem` rather than list the three
cacheable schemes.
**3. The Console and groovysh changes are outside this PR's scope.** Neither
tool ever turns on `memClassOutput`. The groovysh `PackageHelper` change is
also a real behaviour change: it now silently skips any non-file, non-jar URL
where it used to throw `ClassNotFoundException`. That's probably better, but
it's unrelated and arguably its own commit. The `ModuleImportHelper` change is
a genuine fix: `jar:` URLs used to escape as an uncaught
`FileSystemNotFoundException`.
**4. Minor points:**
- **Exception cost on misses:** the in-memory URL is last in lookup order,
and the JDK's handling for unknown protocols turns every miss there into a
thrown `FileNotFoundException`. Groovy's class resolver does a lot of
speculative lookups, so a stackless exception in `Connection.connect` would be
cheaper.
- **Shadowing warning is partial:** `warnIfShadowed` runs straight after
javac, so it only checks javac's classes, not Groovy's own output.
- **Option checked inconsistently:** `memStubEnabled` is read once in the
constructor, but `isMemClassOutput()` is checked on every compile. The docs say
you can turn the option off between compiles, but changing `memStub` that way
has no effect.
- **New public API:** `CompilationUnit.writeClassFile` is new protected API
(`@since 7.0.0`). That's fine, but it's an extension point people will be able
to override.
Things I checked that hold up:
- Saving the compiled classes only after the output phase completes, and
discarding them on failure (`getPhase() > Phases.OUTPUT`).
- Class files javac produces from Groovy stubs are kept out of the in-memory
output.
- `-proc:none` is added only in the new mode, so the default build is
unchanged.
- The in-memory URL never triggers DNS lookups in `equals`/`hashCode`.
- The resource-path checks reject `..` and backslashes.
> In-memory javac class output across compilation units (follow-up to
> GROOVY-12711)
> ---------------------------------------------------------------------------------
>
> Key: GROOVY-12751
> URL: https://issues.apache.org/jira/browse/GROOVY-12751
> Project: Groovy
> Issue Type: Improvement
> Components: Compiler
> Reporter: Daniel Sun
> Priority: Major
>
> h3. Background
> GROOVY-12711 ([PR #3242|https://github.com/apache/groovy/pull/3242]) adds the
> joint-compilation option {{memClassOutput}}
> ({{CompilerConfiguration.MEM_CLASS_OUTPUT}}). Together with {{memStub}}, it
> keeps javac's {{CLASS_OUTPUT}} in memory, so a {{JavaAwareCompilationUnit}}
> needs neither a stub directory nor a target directory on disk.
> In that change, the in-memory store is owned by a single compilation unit. An
> earlier revision also kept a map from {{GroovyClassLoader}} to store, so that
> a second compilation unit created on the same loader could see the classes
> published by the first. That map was removed during review: the only scenario
> it served was a test that deliberately built two units on one loader, and
> none of the normal entry points ({{groovyc}}, the Ant task,
> {{GroovyClassLoader#parseClass}}, groovy-lsp) actually reaches it.
> The underlying question is still a fair one, though, and deserves to be
> looked at on its own merits: when a host keeps a long-lived class loader and
> compiles more than once, how should in-memory class output from earlier
> compilations be made visible to later ones?
> h3. Goal
> Define, and if justified implement, a publication rule for in-memory class
> output that spans compilation units, driven by how real hosts use Groovy's
> existing APIs rather than by a hand-built test scenario.
> Concretely, the outcome should answer:
> * Which loader is long-lived, and which loader defines the classes of each
> compilation?
> * Where must the in-memory bytes live so that the loader actually searched by
> {{getResource}} and {{loadClass}} can find them?
> * How do overlapping class names behave when a later compilation redefines a
> class that an earlier one already produced?
> h3. Facts any design has to respect
> * {{URLClassLoader}} cannot remove a URL, and {{getResource}} returns the
> first match in parent-first, insertion order. Appending one more in-memory
> URL per compilation to a long-lived loader therefore lets the *oldest* bytes
> win for overlapping names. That cannot serve as an update mechanism.
> * A class that a loader has already defined cannot be replaced. Recompiling a
> loaded class always requires a new defining loader.
> * {{GroovyClassLoader#parseClass}} already separates the two roles: the outer
> loader is long-lived, while each parse defines its classes on a fresh,
> short-lived {{InnerLoader}} whose {{getResource}} and {{addURL}} delegate to
> the outer loader. Joint compilation does not use this split today. A map
> keyed on the compilation unit's loader does not follow it either.
> * groovy-lsp reuses one loader across compilation units while the compiler
> settings are unchanged, and its recompiles usually overlap the previous ones.
> It sets {{memStub}} but still writes javac output to a temporary directory.
> If it ever adopts {{memClassOutput}}, overlapping names will be the common
> case, not the exception.
> * The Ant task's incremental mode keeps earlier classes visible by putting
> the destination directory on the classpath ({{includeDestClasses}}). An
> in-memory equivalent has to state whether it means parent delegation to
> earlier output, or a fresh compilation of the union of sources.
> h3. Proposed approach
> # Pick one or more concrete hosts (for example an embedder built on
> {{parseClass}}, or groovy-lsp) and describe its loader lifetime through
> existing, public API only.
> # Follow the {{parseClass}} shape: one long-lived parent that holds
> previously published output only when the host asks for it, plus a new child
> loader per compilation that defines that compilation's classes and is
> searched first.
> # Put the in-memory bytes on the loader that the child actually searches,
> rather than appending a sticky URL to a shared loader.
> # Specify what happens for overlapping names and for classes that are already
> defined, and do not suggest that {{loadClass}} on a loader that already
> defined a class will ever see new bytes.
> # Cover it with tests that go through the normal API
> ({{CompilerConfiguration}}, {{JavaAwareCompilationUnit}},
> {{GroovyClassLoader}}), not with two units manually attached to one loader.
> h3. Out of scope
> * Enabling {{memClassOutput}} inside {{parseClass}} or groovy-lsp. Those are
> potential consumers of whatever rule this issue settles, and can be changed
> separately.
> * Any change to the single-unit behaviour delivered by GROOVY-12711.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
