paulk-asert commented on PR #3084:
URL: https://github.com/apache/groovy/pull/3084#issuecomment-6806281191
Thanks James — your points on keeping the source lookup are fair,
particularly javac parity. I'm happy for that to be the default on master,
provided there's an opt-out just for the source part. I ran a few more checks
against the current tip (`b96fa00c30`), comparing the option on and off:
| Scenario
| Option off |
Option on
|
|
--------------------------------------------------------------------------------------------------------------------
| ---------------------------- |
--------------------------------------------------------------------------------------------
|
| `GroovyShell` evaluates `new Timer()`, then parses a file `Timer.groovy`,
then evaluates `new Timer()` again | `java.util.Timer` both times |
`java.util.Timer`, then `Timer`
|
| GSE runs `a.groovy` (`new Date()`); `Date.groovy` is then added to the
root; `a.groovy` and a new `b.groovy` are run | all `java.util.Date` |
cached `a` gives `java.util.Date`, `b` gives `Date`, a fresh engine running `a`
gives `Date` |
| …then `Date.groovy` is deleted and `b.groovy` is run again
| `java.util.Date` |
still `Date`
|
| GSE root lookups for one script that uses 18 common JDK types
| 103 | 139
(two more per type, every compile)
|
| `GroovyShell` whose parent class loader has a default-package
`Random.groovy` resource | `java.util.Random`
| `Random`
|
Two things follow from this:
- **The full lookup also depends on what the loader has compiled before**
(first row). `findByClassLoading` returns classes cached in the loader, so this
happens with or without sources. I don't think it separates the two options, so
I'd drop it as an argument for keeping sources. It's still worth a sentence in
the notes, since groovysh, the console, and reused `GroovyShell`/GSE instances
are affected.
- **GSE is where the sources part hurts most.** A script records a
dependency only on what it actually resolved to. A script that resolved `Date`
to `java.util.Date` therefore never recompiles when `Date.groovy` appears in a
root. A single engine ends up giving both answers, and a deleted source can
stay in use. Groovlet or script names like `Calendar`, `Locale`, `Duration`,
`Period`, `Year`, `Month`, `Clock`, `Instant`, `Proxy` or `Script` all fall
into this.
### Proposal
**1. A second option for the source part.** For example
`DEFAULT_PACKAGE_SOURCES` (`defaultPackageSources`, system property
`groovy.resolve.default.package.sources`). It only applies when
`defaultPackageFirst` is on, and is on unless set to `false`:
| `defaultPackageFirst` | `defaultPackageSources` | Lookup before the
default imports |
| --------------------- | ----------------------- |
---------------------------------------------------------------------------------------
|
| `false` | (ignored) | none, the old order
|
| on | on (default) | compiled classes and
sources, as now |
| on | `false` | compiled `.class` files
only; sources still resolve after the default imports, as today |
For the `false` case, please don't go through `resolveName` or
`loader.loadClass`:
- Use `loader.getResource(name + ".class")`, then `new
DecompiledClassNode(AsmDecompiler.parseClass(url), new
AsmReferenceResolver(resolver, compilationUnit))`, with the same name-mismatch
check as `readClassFile`.
- On a hit, `classNodeResolver.cacheClass(name, node)`; on a miss, cache
nothing, so the later `resolveToOuter` still finds sibling sources.
Keeping clear of `loadClass` also keeps the loader's cache, and so its
history, out of it. It leaves GSE's `findClassNode` override, and therefore its
roots, alone.
**2. GSE default (optional, your call).** Consider having
`GroovyScriptEngine` default `defaultPackageSources` to `false` unless the
caller sets it. That avoids the mixed and stale results above without changing
`groovyc` or the `groovy` command. If you'd rather keep one default everywhere,
please document the GSE behaviour in the notes instead.
**3. Gradle isn't affected by the source opt-out.** In an incremental build
the classes Gradle doesn't recompile are `.class` files in the output
directory, so the class-only lookup finds them. Source directories aren't on
the compile class path. The stashed-class loaders in gradle/gradle#39622 don't
change this: `CompileClasspathGroovyClassLoader` hides stashed classes from
resolution, and those classes are in the compilation unit anyway. Either
setting therefore gives the same full-vs-incremental result in Gradle.
**4. Tests.**
- Extend `Groovy12555` with the sources-off case: a compiled sibling still
shadows the default import, and a source sibling no longer does.
- Add a GSE case: a root source shadowing a default import with the default
settings, and not with sources off. Assert whichever GSE default you pick.
**5. Notes.** Update `COMPATIBILITY.md` (the Groovy 7 and 6.0.1 entries) and
`core-program-structure.adoc` for the new option, with the Gradle route
`groovyOptions.optimizationOptions['defaultPackageSources']`. Also cover GSE's
mixed old and new results and stale results until the engine is recreated, the
two extra root lookups per type under GSE, and the loader-history point above.
The 6.0.x port would carry both options, both off by default there.
**6. Housekeeping.**
- Daniel's approval was for `14288a89`. The later commit, which applies the
lookup to qualified nested names (`testModuleImports` → `testDefaultImports`),
needs a quick re-review.
- The branch now conflicts with master in `COMPATIBILITY.md` and in the
system-property table in `CompilerConfiguration`'s Javadoc. Both are
straightforward: keep both sides.
--
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]
