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