daniellansun commented on PR #2882:
URL: https://github.com/apache/groovy/pull/2882#issuecomment-5555679838
This is a set of notes on the commit, offered with respect — the overall
direction looks right to me.
## Overall impression
The diagnosis and the production-side structure both seem sound to me.
The trampoline encodings name the declaring class, return type, and
parameter types in the constant pool (`CHECKCAST` / `INVOKE*` / `invokeExact`).
A hidden class is still identity-visible to the defining loader, so
`canResolveInvokeTypes` / Step 1 can succeed, the hidden class defines, and the
first invoke throws `NoClassDefFoundError`. That reading of GROOVY-12361
matches the bytecode in `InvokerBytecode`.
A single sticky decline in `tryCreate` also feels like the right place to
hold the invariant. Patching Step 1 alone would not be enough: Step 2 already
refuses a hidden nest host via `HiddenClassDefiner.canAttemptPrivateLookup`,
but Step 3 (classData) and Step 4 (visible artifact) could still emit
uninvokable bytecode. Folding `isHidden()` into `loaderCanResolve` would
similarly miss Step 2/4, which do not walk the method’s types.
So the “code judo” here — four failure modes collapsed into one policy
decline, with the reflective path remaining the fallback — is, in my view, the
correct layer. I would not try to “fix” hidden types by asType-erasing
classData so they can trampoline; that would add a third encoding for a case
that has always been reflective.
The comments in `tryCreate` / `allTypesNameable` are also very helpful: they
explain *why* define succeeds and invoke fails, which is the non-obvious part.
What I would be grateful to see tightened is mainly the tests (and one small
layering point in `tryCreate`). I would not want to hold up the production
idea; I would only suggest we lock the invariant a bit more firmly before
landing.
## Suggestions
### 1. Consider treating the hidden-type check as a policy decline, next to
the existing ones
```99:121:src/main/java/org/apache/groovy/internal/runtime/invoke/InvokerFactory.java
public static DirectInvoker tryCreate(final CachedMethod method) {
if (method == null || !generationAllowed()) {
return null;
}
if (method.isCallerSensitive() ||
Modifier.isAbstract(method.getModifiers())) {
return null;
}
try {
if (!allTypesNameable(method.getCachedMethod())) {
// ...
return null;
}
return defineSteps(method);
} catch (Exception | LinkageError ignored) {
```
Caller-sensitive and abstract are already sticky policy declines *before*
the `try`. Hidden types seem to be the same kind of decision — “please do not
generate” — rather than a define/link failure. Sitting inside `catch (Exception
| LinkageError)` slightly blurs that boundary, and `getCachedMethod()` (which
may `setAccessible`) becomes a side effect of the predicate.
Would it be reasonable to lift the check next to the other declines, and
take `CachedMethod` the way `isPubliclyInvocableFromInvokerFactory` already
does (`getDeclaringClass().getTheClass()`, `getReturnType()`,
`getNativeParameterTypes()`)? That would also match the types `InvokerBytecode`
actually writes, without calling `getCachedMethod()` only to inspect names.
A small related thought: the class javadoc’s define-path list does not yet
mention this precondition. A reader of `tryStep1` still sees “loader can
resolve these types” and could reasonably think a public hidden nestmate is
legal. A sentence next to the four steps might save the next reader a trip
through GROOVY-12361.
I would *not* suggest sprinkling `isHidden()` into `tryStep1` / `tryStep3` /
`canResolveInvokeTypes` in addition to `tryCreate`. That would be the scattered
version of this fix; the single gate is the cleaner model.
### 2. `testHiddenProxyStaysInvocablePastThreshold` may pass without
exercising the bug
```502:518:src/test/groovy/org/apache/groovy/internal/runtime/invoke/InvokerFactoryTest.groovy
@Test
void testHiddenProxyStaysInvocablePastThreshold() {
String old = System.getProperty(InvokerFactory.PROPERTY_THRESHOLD)
System.setProperty(InvokerFactory.PROPERTY_THRESHOLD, '0')
try {
Object proxy = [name: { 'circle' }] as HiddenProxyBase
// ...
3.times { assertEquals('circle', name.invoke(proxy, new
Object[0])) }
} finally {
if (old == null)
System.clearProperty(InvokerFactory.PROPERTY_THRESHOLD)
else System.setProperty(InvokerFactory.PROPERTY_THRESHOLD, old)
}
}
```
This looks like the only test that walks the production hook
(`CachedMethod.invoke` after the threshold), which is exactly the coverage we
want. A few details made me slightly uneasy that it might stay green without
generation ever running:
- Every other property-mutating test in this file, and in
`CachedMethodDirectInvokerTest`, uses
`@ResourceLock(Resources.SYSTEM_PROPERTIES)`. This one does not. Under parallel
execution, a sibling could restore the default threshold while this test is
running. Then `hits > 1000` is never true, generation never runs, and three
reflective invokes would still pass.
- It never asserts `invokerAttempted` / `invoker == null`. The sticky-fail
tests in this same file already do that
(`testCachedMethodStickyFailsWhenAllDefineStepsFail`). Without those fields,
“invoke still works” is not quite proof that the trampoline was refused.
- Save/restore is inlined; `restoreProperty` is already the local helper.
`testTryCreateDeclinesPublicHiddenHost` also splits `cm(ping)` across two
instances, so the `invoke` after `tryCreate` is a fresh `CachedMethod` that
never reaches threshold. That test still proves the factory returns null, which
is valuable; it just does not prove the hook sticky-fails. The e2e test is the
one that has to carry that load.
If you agree, matching `CachedMethodDirectInvokerTest`’s `withThreshold(0L,
…)` shape would make this much harder to go green accidentally: lock, one
`CachedMethod`, invoke past threshold, assert sticky-fail, then invoke again.
### 3. The old Step 3 fall-through test may now be slightly misleading
`testDefineStepsFallsThroughToClassDataForHiddenNonPublicHost` no longer
tests fall-through. `tryCreate` returning null is a good assertion — it is the
regression guard that a Step 1-only patch would miss. The second half then
drives `tryCreateClassData` and asserts that an uninvokable trampoline still
defines.
I may be missing a reason to keep that second half; from the outside it
looks like a dead production path. `testClassDataEncodingInvokesPublicMethod`
already covers classData on types that can actually run. A “definition
succeeds; do not invoke” fixture might document a landmine rather than remove
it.
Would you consider renaming to something like
`testTryCreateDeclinesNonPublicHiddenHost`, keeping `assertNull(tryCreate(…))`,
and dropping the `tryCreateClassData` success assertion?
I realise that once hidden hosts cannot be the Step 3 fixture, the
production `tryCreate → defineSteps → tryStep3` orchestration is only hit via
the reflective `tryStep3` probe. That seems acceptable — I would only suggest
that the test name and comments not claim to cover `defineSteps` fall-through
any more.
## Things I would not chase in this commit
- A shared “nameable type” helper. `CallSiteGenerator` has a similar
constant-pool shape, but it is out of this commit and on the deprecated classic
call-site path. If a second call site appears later, `HiddenClassDefiner` would
be a more natural home than growing `InvokerFactory`.
- Extra tests for hidden parameter/return types without a hidden declaring
class. That shape is almost unreachable from Groovy source. Declaring-class
coverage (public + non-public + map-coerced proxy) is the production case, and
the three-type walk in `allTypesNameable` is still the right defensive check.
## Summary
I think the production structure is the right one: one sticky decline at
`tryCreate`, not a hidden-class special case in each define step. The notes
above are about making that decline sit with the other policy gates, and about
making the e2e test force generation under a property lock and assert
sticky-fail, plus renaming the old Step 3 test so it no longer claims to cover
`defineSteps` fall-through.
Thank you for the careful comments in the factory — they made the failure
mode much easier to follow.
--
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]
