[
https://issues.apache.org/jira/browse/GROOVY-12375?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18112976#comment-18112976
]
ASF GitHub Bot commented on GROOVY-12375:
-----------------------------------------
daniellansun commented on PR #2898:
URL: https://github.com/apache/groovy/pull/2898#issuecomment-5590815514
## Overview
`Class.forName(name, false, ShortTypeHandling.class.getClassLoader())`
already looks like the right shape. I would not wrap it in a helper, and I
would not turn it into a general-purpose loader with an initialize flag.
The one-argument `Class.forName(name)` is “caller’s class loader,
`initialize=true`”. The caller is `ShortTypeHandling`, so passing
`ShortTypeHandling.class.getClassLoader()` and only flipping `initialize` to
`false` is easy to follow and keeps the old loader behaviour.
`ClassLoader.loadClass` would drop array names such as `"[Ljava.lang.String;"`,
which seems a step backwards. Switching to the thread context class loader
would change *which* classes resolve — a different question, and probably not
one to mix with “do not run `<clinit>`” in the same commit.
None of the touched files crosses a thousand lines on this change
(`ShortTypeHandling.java` ~135, the test ~266, `THREAT_MODEL.md` ~842), and the
production path does not grow extra branches. I do not see a structural split
that would earn its keep.
The three notes below are mostly about how the new contract is recorded and
how the regression is written, not about that one production line.
---
## 1. The “no initialization” claim in the threat model may need a 6.0.0
qualifier
`THREAT_MODEL.md` says it applies to **3.0.x, 4.0.x, 5.0.x, and 6.0.x
alpha**. This change lands on `6.0.0-SNAPSHOT`. On 3/4/5, `'name' as Class`
still runs static initializers.
Section 11a is the “known non-finding” table used by scanners and triage. A
reader on 4.0 or 5.0 who sees “coercion does not run static initializers” might
downgrade a finding that is still true on those lines.
The same document already dates new mitigations (“from 6.0.0” for JSON
`maxNestingDepth` and XML `maxElementDepth`). This row could follow that
pattern:
- On every covered version, the disposition can stay `KNOWN-NON-FINDING` /
downstream boundary — even when initialization still runs, feeding untrusted
data to `as Class` remains an application choice.
- The “resolved without initialization” part could be marked **as of 6.0.0
(GROOVY-12375)**.
That would keep the 6.0 story intact without implying the mitigation already
exists on 3/4/5.
---
## 2. This is a user-visible language change; a 6.0 compatibility note might
help
`castToClass` is `@GroovyABI(since = "2.3.0")` and is how `'name' as Class`
is implemented (`InvocationWriter` calls it directly; the dynamic path goes
through `DefaultTypeTransformation` / `asType`). The old javadoc said the name
was loaded via `Class.forName`, and `Class.forName` is widely understood to
initialize. The likely casualty is the JDBC `Class.forName(driver)` pattern:
`'com.mysql.jdbc.Driver' as Class` will no longer register the driver.
6.0 is a major release, so the change is reasonable. `COMPATIBILITY.md`
still treats “behavioural changes users may have come to rely on” as breaking,
even when `japicmp` cannot see them. The method javadoc is helpful for Java
callers; for the language-level `as Class` operator, a line in the 6.0
compatibility notes or release notes might be safer, along the lines of:
> String-to-`Class` coercion resolves the type; it does not run `<clinit>`.
If you still need that side effect, call `Class.forName(name)` (or
`Class.forName(name, true, loader)`).
The javadoc on the method is already clear. I would not add API or a switch
just to preserve the old side effect.
---
## 3. The regression belongs on the existing test class, and the two-type
split is sound; a few small tidy-ups
Splitting Flag and Probe is necessary: reading a static field on
`StaticInitProbe` would initialize it and hollow out the test. I would keep
that.
Three smaller points:
**Missing `// GROOVY-12375`.** `CONTRIBUTING.md` asks for that comment on
the line immediately above a method added to an existing test class, so `git
grep GROOVY-12375` can find it. Right now the key appears only in the commit
message. The sibling `DefaultTypeTransformationTest` already uses `//
GROOVY-9916`, `// GROOVY-10028`, and so on.
**Top-level Groovy types are public by default.** `StaticInitProbe` and
`StaticInitProbeFlag` become public types on the test classpath, with one-shot
mutable static state. The same package already nests helpers
(`DefaultTypeTransformationTest` uses nested `static class`). Folding Flag and
Probe into `ShortTypeHandlingTest` as static nested types would keep the
package a little quieter.
**Hard-coded FQCN.** Once nested, the binary name becomes
`ShortTypeHandlingTest$StaticInitProbe`. That is what `Class.forName` wants;
`StaticInitProbe.name` tracks the type if it moves, and is a bit more durable
than a string literal.
A possible sketch:
```groovy
// GROOVY-12375
@Test
void testCastToClassDoesNotInitialize() {
def probe = castToClass(StaticInitProbe.name)
assert probe == StaticInitProbe
assert !StaticInitProbeFlag.initialized: 'coercion must not run the
static initializer'
StaticInitProbe.touch()
assert StaticInitProbeFlag.initialized: 'static initializer should
run on first use'
}
static class StaticInitProbeFlag {
static boolean initialized
}
static class StaticInitProbe {
static {
StaticInitProbeFlag.initialized = true
}
static void touch() {}
}
```
Testing `castToClass` directly is enough: that is the method the compiler
emits for `as Class`. I would not add a `GroovyClassLoader` fixture, or a
second assertion on the language operator, unless someone particularly wants
the extra documentation.
The static flag is still one-shot (a retry in the same JVM would fail). That
seems acceptable for this suite; I would not add reset or locking around a
one-way `<clinit>`.
---
## What I would leave as-is
- No shared “load without initializing” helper. Core already uses
three-argument `forName` in place (`IOGroovyMethods.resolveClass`,
`JacksonHelper`); one more call site does not seem to justify a new abstraction.
- I would not switch to the TCCL in this commit. That changes which classes
can be resolved, which is a different problem from skipping `<clinit>`.
- I would not add an `initialize` parameter or mode flag to `castToClass`.
Its job is coercion to `Class`, not a re-export of the full `Class.forName` API.
- File size and control flow look fine. The production implementation
already looks like the shape I would keep.
---
## Summary
The Java change is the one-liner I would have hoped to see. Before merge, it
might be worth:
1. Qualifying the §11a row (as of 6.0.0 / GROOVY-12375);
2. Marking the regression with `// GROOVY-12375` and nesting the probe types.
The 6.0 compatibility note can live in the same commit or wait for the
release-notes pass; it probably should not live only in the method javadoc.
Offered with thanks for the work — happy to be corrected if I have misread
any of this.
> ShortTypeHandling: castToClass should avoid class init
> ------------------------------------------------------
>
> Key: GROOVY-12375
> URL: https://issues.apache.org/jira/browse/GROOVY-12375
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
