paulk-asert commented on PR #2645:
URL: https://github.com/apache/groovy/pull/2645#issuecomment-5086232622
Thanks @daniellansun and @blackdrag for the thorough reviews — nearly
everything has been actioned, and the preemption design has been reworked
substantially in response. Details below; the branch is updated.
## Preemption redesign: intent + containment + consent
The `classTagPreemptionTargets` name allowlist is gone. Daniel was right
that a config-level name list was the wrong layer (names aren't unique, and a
library couldn't express intent alone), and Jochen's mailing-list concern was
that preemption changes the meaning of existing source at all. The replacement
splits the decision across three parties, each owning the part that's theirs:
1. **Intent (API author)** — `@ClassTag` gains `preempt() default false`.
Only an overload explicitly marked `@ClassTag(preempt=true)` can ever displace
a token-less incumbent; unmarked tags are strictly additive. A library now
self-declares — no consumer configuration needed. DGM's checked `withDefault`
overloads declare it; `asChecked` does not (nothing to preempt).
2. **Containment (structural invariant, not configurable)** — a tagged
overload may only preempt a token-less overload declared by the *same owner*:
for an extension-method incumbent, tagged overloads of the same extension
class; for an instance-method incumbent, the receiver's own class hierarchy.
This closes what I think is the most important hole in author-declared
preemption: without it, any jar on the compile classpath could ship e.g.
`collect(Iterable, @ClassTag(preempt=true) Class, Closure)` and silently
capture every statically-compiled `collect{}` call on the next rebuild. With
the same-owner rule, a library can upgrade callers of *its own* lenient API but
structurally cannot capture calls it doesn't own — the residual exposure
reduces to "a library can change the behaviour of calls to itself", which it
could already do by editing its method bodies.
3. **Consent (consumer)** —
`CompilerConfiguration.classTagPreemptionDisabled`, a veto set using the same
selector forms as `groovy.extension.disable`: a bare name (`withDefault`), an
exact signature in self-first static form (`withDefault(Map,Class,Closure)`),
or `*` to disable all preemption. Seeded from
`-Dgroovy.classtag.preemption.disable`, so no config script is needed for the
common cases. Default empty = honour declared intent.
For Jochen specifically: `*` (or simply never writing `preempt=true`) is now
a first-class "no preemption ever" mode, and out of the box the only method
that preempts anything is `withDefault` — whose lenient variant is the
GROOVY-11807 bug this exists to route around.
## Daniel's review items
- **#1 inject → re-select → rollback**: reworked as suggested. Matching is
now a pure computation (`ClassTagSupport.matchAdditive`/`matchPreemption`);
tagged argument types are computed by splicing token types into the args array,
and the AST is rewritten only after the retried selection binds. The rollback
path and the duplicated receiver loop are gone.
- **#3 visitor growth + orphaned Javadoc**: all ClassTag logic extracted to
a new package-private `ClassTagSupport` (~25-line call site remains in the
visitor, net −230 lines); `findMethodsWithGenerated`'s Javadoc is reunited with
its method.
- **#4a arity-only matching**: subsumed by the pure-match refactor.
- **#4b silent ambiguity**: still fail-soft by design (degrading to the call
as written must not become an error), but I agree the additive-case "no
matching method" error deserves a hint that injection was attempted and
abandoned — follow-up.
- **#4c receiver binding**: fixed. Additive matching walks receivers in
delegate/owner priority order and constrains the retry to the matched receiver;
preemptive matching reifies from the receiver the incumbent bound on, so an
upgrade can change overload but never binding target.
- **#4d docs**: annotation Javadoc no longer says "trailing" (tokens fill
declared positions, anywhere in the list), and preemption + the veto config are
now documented on the annotation itself.
- **#4e non-`Class` parameter**: now a compile-time error on source methods,
parallel to the override-typo check.
## Jochen's test questions
1. *token-less overload alongside the tagged one* — covered by
`testUserApiWithoutDeclaredIntentStaysLenient` /
`testUserApiDeclaresPreemptionIntent` (and now also
`testUserApiIntentVetoedByConsumer`).
2. *switched `Class<V>`/`Class<K>` positions* —
`testTokensReorderedByTypeVariableNotPosition` covers the swap; new
`testTokensInterleavedWithSuppliedArguments` additionally covers a token
declared *after* a supplied argument.
3. *method-declared type parameters* — this was a genuinely good catch:
chasing it surfaced that a method-declared type variable shadowing a
class-level one (per Java scoping) could have reified the *wrong* class. Now:
on instance methods compiled from source, tagging a method-declared variable is
a compile-time error; a matching-side guard makes precompiled libraries degrade
to no-injection rather than mis-inject; static self-style methods (the
extension-authoring pattern) are exempt since there the method-level variable
*is* the mechanism. Tests: `testMethodTypeParameterIsRejectedAtCompileTime`,
`testShadowedTypeParameterIsRejectedAtCompileTime`,
`testStaticSelfStyleMethodTypeParameterAccepted`.
## Status / open items
`ClassTagStaticTest` 32/32, `CompilerConfigurationTest` 15/15, full
`groovy.transform.stc.*` (2012 tests) and `classgen.asm.sc.*` (2295 tests)
sweeps green, `Groovy11807` regression green.
Two known follow-ups: (a) the preemption check now runs without the old O(1)
name pre-gate — it's bounded (cached DGM lookup filtered by owner /
receiver-hierarchy scan), but I'd like a compile-time benchmark on a large
`@CompileStatic` codebase before merge, with an `ExtensionMethodCache`-level
"names with preempt intent" cache as the optimization if needed; (b) the
cross-module containment ("an extension module cannot capture DGM's methods")
is implemented but wants an m12n-style fixture-jar integration test.
Comments on the revised model welcome — particularly whether `preempt=true`
+ same-owner containment + selector veto addresses the "why is one method
better" and "wrong layer" concerns, respectively.
--
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]
