[
https://issues.apache.org/jira/browse/GROOVY-12369?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18111997#comment-18111997
]
ASF GitHub Bot commented on GROOVY-12369:
-----------------------------------------
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.
> Harden Grape uninstall
> ----------------------
>
> Key: GROOVY-12369
> URL: https://issues.apache.org/jira/browse/GROOVY-12369
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
