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