[
https://issues.apache.org/jira/browse/GROOVY-12555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18138058#comment-18138058
]
ASF GitHub Bot commented on GROOVY-12555:
-----------------------------------------
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)).
> A default-package class resolves a precompiled default-package class to a
> default import of the same name
> ---------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12555
> URL: https://issues.apache.org/jira/browse/GROOVY-12555
> Project: Groovy
> Issue Type: Bug
> Components: Compiler
> Affects Versions: 4.0.33, 5.1.3
> Reporter: James Daugherty
> Priority: Major
>
> A class in the default package that refers to another class of the default
> package by its simple name finds that class while both are compiled together.
> When the other class is already compiled, the name resolves to a class of the
> default imports with the same simple name instead, here
> {{groovy.util.Factory}}, and the call fails at run time. In Java, a class of
> the same package, the unnamed one included, takes precedence over on-demand
> imports such as {{java.lang.*}}.
> {code:groovy}
> // Factory.groovy, default package
> class Factory {
> static String hello() { 'hello from the Factory of this project' }
> }
> // Caller.groovy, default package
> class Caller {
> static void main(String[] args) {
> println Factory.hello() // MissingMethodException for
> groovy.util.Factory when Factory is already compiled
> }
> }
> {code}
> Reproduced with 4.0.33, 5.1.3 and master. A reproducer with a groovyc script
> and an incremental Gradle build:
> https://github.com/jdaugherty/groovy-default-package-resolution
> *Cause*
> {{ResolveVisitor.resolve}} tries, in order, the classes of the module, of the
> compilation unit, of the default imports, and then the class path. For a
> module of a named package, the module step also looks up {{package.Name}} on
> the class path, so a compiled class of the same package comes first. For the
> default package there is no such step, so the default imports come before a
> compiled class of the default package.
> *Proposed fix*
> For a class of the default package, look up the simple name on the class path
> after the compilation unit and before the default imports, as is done for a
> named package.
> *Compatibility*
> For a compilation unit in the default package, a simple type name that is not
> declared or imported in the unit and not in the same {{CompilationUnit}} is
> now looked up on the class path, and on the source path of the
> {{GroovyClassLoader}}, as a default-package class before the default imports
> ({{java.lang}}, {{java.util}}, {{java.io}}, {{java.net}}, {{java.time}},
> {{groovy.lang}}, {{groovy.util}}, {{BigInteger}}, {{BigDecimal}}). A
> default-package {{List}} or {{Node}} on the class path therefore shadows
> {{java.util.List}} or {{groovy.util.Node}} in default-package code, where the
> default import used to win. This matches Java (JLS ยง6.4.1: a type declared in
> the same package, the unnamed package included, shadows a type imported on
> demand), what Groovy already does when the class is in the same
> {{CompilationUnit}}, and what {{ResolveVisitor.resolveFromModule}} does for a
> named package. The order only changes for an incremental or split build,
> which is the case where the old order produced different classes from a full
> build.
> Found while making incremental builds of a Grails application produce the
> same classes as a full build; see also GROOVY-12550 and GROOVY-12544.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
