jdaugherty commented on PR #3084: URL: https://github.com/apache/groovy/pull/3084#issuecomment-6634947128
Thanks for measuring this. The table is right, and the sentence about incremental builds was wrong. I have taken the proposal, with one exception (3) where I would like to make a case before changing the code. The branch is rebased on current master; the new commits are the option with its tests, and the notes. **1. Option.** `CompilerConfiguration.DEFAULT_PACKAGE_FIRST` (`defaultPackageFirst`, system property `groovy.resolve.default.package.first`), read in `resolveFromDefaultPackage`. On master it is on unless set to `false`. The key is absent from the default map, like `errorRecovery`, so a map supplied from outside behaves the same as a fresh configuration. The 6.0.x port is the same code with the read inverted: off unless set to `true`, `@since 6.0.1`. **2. Gradle.** I checked Gradle's `ApiGroovyCompiler` ([source](https://github.com/gradle/gradle/blob/master/platforms/jvm/groovy-compiler-worker/src/main/java/org/gradle/api/internal/tasks/compile/ApiGroovyCompiler.java)). It creates the `CompilerConfiguration`, which is where a `-D` from `forkOptions.jvmArgs` would be read, applies `configurationScript`, and then calls `configuration.setOptimizationOptions(spec.getGroovyCompileOptions().getOptimizationOptions())`, which replaces the map. So neither the system property nor a configuration script reaches this option from Gradle; `groovyOptions.optimizationOptions['defaultPackageFirst'] = true` does, and that is what the notes document. **3. Class files only.** I would rather keep the source lookup: - javac does the same. `javac Main.java` with a sibling `Math.java` in the directory fails with `cannot find symbol: method max(int,int), location: class Math`, because the source path defaults to the class path and the sibling is compiled implicitly. The stray-script row is Java's behaviour too. - A class-only probe cannot go through `ClassNodeResolver.resolveName`: a miss is cached as `NO_CLASS` for the unit, so the later `resolveToOuter` would never see the sibling source, and sibling scripts that resolve today would stop resolving. It needs a second, non-caching lookup path. - It would make the result depend on history in a long-lived loader. Script classes compiled earlier are in the `GroovyClassLoader` class cache, and `loadClass(name, false, true)` returns them, so under a `GroovyShell` or GSE whether `Timer` is the sibling would depend on whether something had already compiled `Timer.groovy`. The full lookup is deterministic given the files. - GSE's root probe lives in its `findClassNode` override, so a class-only path would have to bypass subclass overrides as well. The option covers the script-folder case either way. If you still prefer class-only for 7 after this, I will do it, with a separate non-caching probe rather than `resolveName`. **4. Tests.** `Groovy12555` now has the compiled-class case and the sibling-source case, each with the option on and off, plus the default. `CompilerConfigurationTest` covers the system property and the default map. **5. Docs.** `COMPATIBILITY.md` has a Groovy 7 entry and a 6.0.1 entry: who is affected (`groovy foo.groovy` with sibling files, GSE roots), the remedies, the option with the Gradle route, the per-unit cost, and the star-import wording you suggested. `core-program-structure.adoc` has a note under "Default imports" with the current-directory caveat. The PR description and the JIRA are corrected. **6. Backports.** Agreed: 5.1.x dropped. The 6.0.x port is on [`GROOVY-12555-6_0_X`](https://github.com/jdaugherty/groovy/tree/GROOVY-12555-6_0_X) ([diff against `GROOVY_6_0_X`](https://github.com/apache/groovy/compare/GROOVY_6_0_X...jdaugherty:groovy:GROOVY-12555-6_0_X)). -- 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]
