paulk-asert commented on PR #2893: URL: https://github.com/apache/groovy/pull/2893#issuecomment-5557100913
Thanks @daniellansun for the careful notes — all three suggestions taken in the follow-up commit (1a575020a4), plus Copilot's lock point which was the same as your first bullet under (2). **Production** - The hidden-type check now sits above the `try` beside the caller-sensitive / abstract declines, so it reads as a policy decline rather than a define failure. - `allTypesNameable` takes the `CachedMethod` and reads the declaring class, return type and native parameter types the same way `isPubliclyInvocableFromInvokerFactory` does, so it no longer forces accessibility on the underlying `Method`. (Either form yields the same classes as `InvokerBytecode` writes; this one just avoids the side effect.) - The class javadoc gains a paragraph after the four define-path steps stating the hidden-type precondition, so a reader of `tryStep1` no longer has to go via GROOVY-12361. **Tests** - `testHiddenProxyStaysInvocablePastThreshold` now takes `@ResourceLock(Resources.SYSTEM_PROPERTIES)`, uses `restoreProperty`, drives a fresh `CachedMethod` past the threshold and asserts `invokerAttempted == true` / `invoker == null` before re-invoking, mirroring `testCachedMethodStickyFailsWhenAllDefineStepsFail`. For the record I also confirmed it is not vacuous: with the gate removed it fails with the original `NoClassDefFoundError` on the `_groovyProxy` hidden class, and the two factory-level tests fail on the null assertion. - The old Step 3 fall-through test is renamed `testTryCreateDeclinesNonPublicHiddenHost`, keeps `assertNull(tryCreate(...))`, adds an `allTypesNameable` check, and drops the direct `tryCreateClassData` definition of an uninvokable trampoline. You were right that the "remaining classData tests rely on it" comment was inaccurate — they all use ordinary subjects. One small note on the lock: JUnit parallel execution is not enabled in this build, so the race was theoretical, but consistency with the rest of the file is reason enough. Not chased, as you suggested: a shared nameable-type helper with `CallSiteGenerator`, and extra coverage for hidden return/parameter types without a hidden declaring class (the three uncovered lines Codecov flags). The TestLens failure is the known-flaky `ChannelSelectTest` timer test, unrelated to this change. `InvokerFactoryTest` (62) and `CachedMethodDirectInvokerTest` (33) are green locally. -- 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]
