You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2026/09/07 05:12:43 UTC
[PR] GROOVY-12375: ShortTypeHandling: castToClass should avoid class init [groovy]
paulk-asert opened a new pull request, #2898:
URL: https://github.com/apache/groovy/pull/2898
(no comment)
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12375: ShortTypeHandling: castToClass should avoid class init [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert commented on PR #2898:
URL: https://github.com/apache/groovy/pull/2898#issuecomment-5592964898
Thanks Daniel, changed as per your comments.
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12375: ShortTypeHandling: castToClass should avoid class init [groovy]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #2898:
URL: https://github.com/apache/groovy/pull/2898#issuecomment-5565922058
## ✅ All tests passed ✅
🏷️ Commit: 9ce9a517c0c1a2a523cee76c150a3f367ce4bf7a
▶️ Tests: 117646 executed
⚪️ Checks: 32/32 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12375: ShortTypeHandling: castToClass should avoid class init [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2898:
URL: https://github.com/apache/groovy/pull/2898#issuecomment-5565673810
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2898?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 71.1269%. Comparing base ([`5b75e21`](https://app.codecov.io/gh/apache/groovy/commit/5b75e21feb99a5676f24c3dde932af1ffae9a578?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9ce9a51`](https://app.codecov.io/gh/apache/groovy/commit/9ce9a517c0c1a2a523cee76c150a3f367ce4bf7a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2898 +/- ##
==================================================
+ Coverage 71.1240% 71.1269% +0.0029%
- Complexity 37493 37497 +4
==================================================
Files 1579 1579
Lines 135777 135777
Branches 25174 25174
==================================================
+ Hits 96570 96574 +4
+ Misses 30506 30503 -3
+ Partials 8701 8700 -1
```
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2898?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...groovy/runtime/typehandling/ShortTypeHandling.java](https://app.codecov.io/gh/apache/groovy/pull/2898?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2Ftypehandling%2FShortTypeHandling.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvdHlwZWhhbmRsaW5nL1Nob3J0VHlwZUhhbmRsaW5nLmphdmE=) | `96.8750% <100.0000%> (ø)` | |
... and [6 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2898/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12375: ShortTypeHandling: castToClass should avoid class init [groovy]
Posted by "daniellansun (via GitHub)" <gi...@apache.org>.
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.
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-12375: ShortTypeHandling: castToClass should avoid class init [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert merged PR #2898:
URL: https://github.com/apache/groovy/pull/2898
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org