[
https://issues.apache.org/jira/browse/GROOVY-12555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18136955#comment-18136955
]
ASF GitHub Bot commented on GROOVY-12555:
-----------------------------------------
paulk-asert commented on PR #3084:
URL: https://github.com/apache/groovy/pull/3084#issuecomment-6594231862
Thanks for this — I agree with the direction: a default-package class should
resolve the same way whether it comes from the same compilation unit or from a
previous (incremental) compile, and JLS §6.4.1 is the right reference point.
I've been trying to size the blast radius, and it's wider than the
Compatibility note suggests, so I'd like to propose a slightly different
landing.
### What I measured
I overlaid this PR's `ResolveVisitor` on a current master jar and ran each
case both ways via the real launcher (`GroovyStarter`/`GroovyMain`, default
class path `.`):
| Scenario | master | this PR |
|---|---|---|
| `Node.groovy`, `Timer.groovy`, `Date.groovy` (classes) next to
`main.groovy` | `groovy.util.Node`, `java.util.Timer`, `java.util.Date` | the
sibling classes |
| a stray script `Math.groovy` next to a script doing `Math.max(1, 2)` | `2`
| `MissingMethodException: No signature of static method: max for class: Math` |
| a half-written `Timer.groovy` (syntax error) next to a script doing `new
Timer()` | runs | compile error in `Timer.groovy`, a file that was never
compiled before |
| a sibling `Process.groovy` script; `"ls".execute() instanceof Process` |
`true` | `false`, silently |
| `GroovyScriptEngine` root containing `Date.groovy`; another script does
`new Date()` | `java.util.Date` | the root's `Date` |
| compiled `Factory.class` on the class path (the case in this PR) |
`groovy.util.Factory` | `Factory` ✔ |
So the sentence *"The order only changes for an incremental or split build"*
isn't accurate. Two things make the exposure broader:
1. When no classpath is given, the `groovy` command uses the current
directory, the same as `java` (`startGroovy`: `CP=.`; see GROOVY-12374 and the
note in `tools-groovy.adoc`). On 5.1.x, `.` is appended even to an explicit
`-cp`/`CLASSPATH`. So in a folder of scripts, any file named like a
default-import type now shadows it. That includes `java.lang` names (`Math`,
`System`, `Process`, `Thread`, `Record`) as well as `java.util` / `groovy.util`
ones (`Timer`, `Random`, `Node`, `Proxy`, `Eval`, `Script`, `Range`, …).
2. `resolveToOuter` ends in `ClassNodeResolver.tryAsScript`, so `.groovy`
**sources** on the loader's source path are found, added to the compile
(`Interrupt`/restart), and win. The same applies to `GroovyScriptEngine` roots,
and therefore Groovlets.
There's also a cost for default-package scripts, which are the hot path for
`GroovyShell`, `Eval` and GSE. I timed 1000 fresh `GroovyShell.parse` calls of
a script using about 20 JDK types, with 60 jars on the class path:
| Variant | Time per parse |
|---|---|
| master | ~5.0 ms |
| this PR | ~6.1 ms (about +20–25%) |
| master, same script with `package p` | ~5.3 ms |
The "named packages already pay this" point holds in kind, but the
`ClassNodeResolver` cache is per compilation unit, so every fresh shell or
script compile pays it again. Under GSE, each miss also probes every root via
`getResourceConnection`. I think that's acceptable, but it should be stated
rather than described as nothing new.
On star imports: an explicit `import java.util.*` still beats a
default-package class with this change, whereas Java would pick the
same-package type. Groovy has long described star imports as coming after
everything else, so I'm fine with that. I'd just word the Java comparison as
"matches JLS §6.4.1, except that explicit star imports still take precedence,
as elsewhere in Groovy".
### Proposal
**1. Put the behaviour behind a compiler optimization option, using the
existing pattern.**
`CompilerConfiguration` already maps system properties onto optimization
options (`groovy.parallel.parse`, `groovy.parser.error.recovery`, …) via
`handleOptimizationOption(...)`. A new option there, with a matching system
property (name open — something like `groovy.resolve.defaultPackageFirst`),
would be read in `resolveFromDefaultPackage`. The same code would then ship to
both branches with different defaults:
- **master (Groovy 7): on by default.** The flag is an opt-out for anyone
hit by sibling-file shadowing.
- **GROOVY_6_0_X (6.0.1): off by default.** It's opt-in, so 6.0.x
behaviour doesn't change unless someone asks for it.
**2. Make the opt-in reachable from Gradle.** The motivating case is Gradle
incremental compilation, so please document a way to enable it there. Either
`groovyOptions.configurationScript` setting the optimization option, or
`groovyOptions.forkOptions.jvmArgs += '-D…=true'`, whichever actually reaches
the compiler daemon. Otherwise the 6.0.x opt-in won't help the people who
reported this.
**3. Consider restricting the early lookup to compiled `.class` files.** The
incremental-build inconsistency only needs the class-file lookup. If the early
step skipped the `tryAsScript` / source-path branch, sibling `.groovy` files
would keep resolving as they do today. That removes most of the surprising rows
above (stray `Math.groovy`, a broken `Timer.groovy` breaking unrelated scripts,
GSE roots), while compiled classes would shadow as in Java. I'd like this at
least considered for the on-by-default 7 behaviour. Happy to discuss if you
think the source-path case matters for consistency.
**4. Tests.** Parameterize `Groovy12555` over the option: with it on, the
existing full-vs-incremental assertion; with it off, resolution unchanged (the
6.0.x default). If the source-path lookup stays in, please add a case with a
sibling `.groovy` source as well.
**5. Docs and compatibility notes.**
- `COMPATIBILITY.md`, two entries:
- **Groovy 7:** "a default-package class on the class path takes
precedence over default imports". Cover who is affected (`groovy foo.groovy`
with sibling files named like JDK/GDK types, GSE roots), the remedies (rename,
use a package, or use a fully-qualified name), the opt-out, and the per-unit
lookup cost.
- **Groovy 6.0.1:** the new opt-in, with 6.0.x behaviour unchanged unless
it's set.
- `src/spec/doc/core-program-structure.adoc`: a short note in "Default
imports" or "Namespace conflicts" that a type in the same package, the default
package included, shadows a default import. Include the `groovy foo.groovy` /
current-directory caveat.
- The PR description and JIRA: correct the "only incremental or split
builds" sentence.
**6. Backports.** I'd stop at 6.0.x (opt-in). This reproduces on 4.0.33 and
5.1.3, so it's a long-standing inconsistency rather than a regression. Adding a
flag to the 5.1.x maintenance line doesn't seem worth it unless there's a
pressing need, so I'd leave `GROOVY_5_1_X` as is.
Thanks again — the fix itself is small and clear. This is mostly about
landing it so that people with a folder full of scripts aren't surprised.
> 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)
