paulk-asert commented on PR #3085:
URL: https://github.com/apache/groovy/pull/3085#issuecomment-6532734438
Verdict: ready to merge. I found no correctness or metaprogramming blocker.
James has already answered Daniel's asType question in the PR thread: a cast
site never selects asType, while the as operator goes through an ordinary
method call site whose metaclass guard the PR leaves alone. The PR has
blackdrag's approval and green CI.
To test it I built the PR head and its parent and ran probes against both.
The jars and probe scripts are in
/private/tmp/claude-501/-Users-paulk-Projects-groovy/deaa34fd-22fc-4390-b220-52371f39f6b5/scratchpad/p3085/;
I've removed the scratch worktree.
Other ways it could break, and what I found:
1. Is anything else in the cast metaclass-dependent? No. Besides asBoolean,
a cast links one of four handles:
- identity;
- the HashSet/ArrayList constructors;
- closure-to-SAM conversion;
- castToType.
The first three depend only on the value's class. castToType runs fresh
on every call, so any metaclass lookup inside it (such as a constructor through
invokeConstructorOf) never gets baked into the cached handle. The Boolean and
boolean targets are caught by handleBoolean first, which keeps the guard.
2. The metaclass guard was also quietly a null guard. isSameMetaClass(mc,
null) returns false, so a null value forced a relink. With the PR, a cast from
a final static type gets only the switch-point guard; I confirmed in the indy
log that nothing else is added. A null-unsafe handle cached for a non-null
value could therefore be reused for null. In practice that can't happen:
- Identity, SAM and castToType handle null fine.
- The collection-constructor fast path is effectively unreachable,
because isAbstractClassOf(HashSet, Set) tests the subtype relationship the
wrong way round. Those casts fall through to castToType instead. That's an old
oddity, not something this PR introduced.
I warmed a final class Bag extends ArrayList → Set cast past the optimize
threshold, then passed null. It returned null on both builds, and the final
Number → int case still throws GroovyCastException on both.
3. Per-instance asBoolean still relinks. The flag is only set when
handleBoolean selects through the metaclass, and a new selector is created on
every relink, so stale state can't carry over. The PR's alternating
truthy/falsy test covers this.
4. Proxy truthiness is still not fixed, but it matches classic call sites. I
used a Groovy subclass whose getMetaClass() throws, standing in for a scoped
proxy:
| Shape | Before (indy) | PR (indy) | PR
(classic) |
|----------------------------------|---------------|-----------|--------------|
| Report r = p, (Report) p | throws | works | works
|
| boolean b = p, if (p), p ? :, !p | throws | throws | throws
|
| p == p | works | works | works
|
It's worth one sentence on the JIRA so Grails users don't expect if
(scopedProxy) to work outside the scope.
5. Small performance gain. Casting a GroovyObject no longer calls
getMetaClass() on every invocation. Values with per-instance metaclasses no
longer fail the guard and relink the cast site each time.
Backports:
- The PR links 6.0.x and 5.1.x commits, and Daniel agrees backporting is
warranted. I didn't review James's hand-applied 5.1.x version; it's worth
running the new test on that branch before you pick it.
- The PR says the bug reproduces on 4.0.33, and GROOVY_4_0_X has the same
unconditional receiver instanceof GroovyObject guard in setGuards. Grails 7
runs on Groovy 4.0.x. If the scoped-proxy report comes from a Grails 7 app,
4.0.x may be the backport that actually matters, so ask James whether he wants
it there too.
Nits (none blocking):
- The test uses static counters, which is fine as long as tests run
sequentially in one JVM.
- No test uses a real proxy subclass, but Watched covers the same path.
- The existing "call site stays uncached" log line in doCallSiteTargetSet is
misleading: the target is in fact set later.
--
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]
