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