[
https://issues.apache.org/jira/browse/GROOVY-12751?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18141681#comment-18141681
]
ASF GitHub Bot commented on GROOVY-12751:
-----------------------------------------
daniellansun commented on PR #3284:
URL: https://github.com/apache/groovy/pull/3284#issuecomment-6756159830
Thanks for the thorough review, @paulk-asert. All of it is addressed on the
current head `8163138f0e4d`.
**1. Published classes invisible to javac.** Fixed.
`InMemoryJavaFileManager` now lists and serves the in-memory classes for
`CLASS_PATH` only: this compilation's stage, then its committed classes, then
classes an earlier compilation published (the same precedence the class loader
uses). They are appended after the delegate's listing, and a binary name the
regular classpath already provides is not added again, so a real classpath
entry still wins. `getJavaFileForInput`, `getFileForInput`, `contains` and
`hasLocation` follow the same rule. Platform and module locations are
untouched, and invalid resource names (empty, `..`, backslash, non-class) are
not served. Your `p/Lib.java` + `p/G.groovy` → `q/Use.java` scenario is now a
test in `MemClassOutputPublicationTest`.
**2. `AsmDecompiler` caching.** Fixed as suggested: `cacheableScheme` now
excludes only `x-groovy-mem`, so `bundleresource:`, `vfs:`, `wsjar:` and every
other scheme are cached as before.
**3. Console/groovysh changes.** These are not part of the GROOVY-12751
commit; they come from the GROOVY-12711 parent (`d413c518`, PR #3242), which
this PR is stacked on. Reverting them here would undo part of #3242 once both
land, so I've left them there. I agree the `ModuleImportHelper` change is the
genuine fix (a `jar:` or in-memory URL no longer escapes as
`FileSystemNotFoundException`); if you'd prefer the `PackageHelper` behaviour
change split out of #3242 into its own commit, I'm happy to do that.
**4. Minor points.**
- *Exception cost on misses:* a miss in `Connection.connect` /
`resourcePath` now throws a stackless `FileNotFoundException` subclass, so
speculative lookups no longer pay for stack capture.
- *Partial shadowing warning:* `writeClassFile` now also runs the shadow
check for each Groovy-generated class as it is stored, in addition to the javac
output check.
- *`memStub` read inconsistently:* `memStub` is now read at the start of
each `JavaAwareCompilationUnit.compile`, consistent with `isMemClassOutput()`,
so toggling it between compiles takes effect.
- *`writeClassFile` as protected API:* kept as an intentional extension
point; the javadoc now states that an override which keeps the bytes itself
should not call through, otherwise the class file is also written to the target
directory.
Verification: `./gradlew lintGate` and the full `./gradlew test` pass
locally, and CI on `8163138f0e4d` is green across the LTS matrix (Linux
17/21/25, Windows, macOS), lint and CodeQL.
> 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)
