daniellansun commented on PR #2645: URL: https://github.com/apache/groovy/pull/2645#issuecomment-5083464789
# Request changes before merge: ## 1. Method selection: inject → re-select → rollback In `visitMethodCallExpression`, the implementation mutates `argumentList` in place, re-runs a near-duplicate receiver/`findMethod` loop, then clears and restores the original arguments on failure. That couples AST surgery to selection success via an implicit rollback contract and duplicates control flow already present in the same method. **Suggestion:** Treat `@ClassTag` matching as a pure computation (no AST mutation). Reuse a single `selectOnReceivers(...)` for both plain and expanded argument types. Commit the argument rewrite only after tagged selection succeeds. That removes the rollback path and the duplicated loop. --- ## 2. Preemption policy via method-name config is the wrong layer `CompilerConfiguration.classTagPreemptionTargets` gates preemption by bare method name (default `"withDefault"`). Method names are not unique, so any type’s `withDefault` shares the policy. User APIs that want preemption need both parameter annotations and a compiler config change—the library cannot express intent alone. **Suggestion:** Co-locate preemption with the API (e.g. an attribute on `@ClassTag`, or a method-level marker). Keep configuration, if needed, as a global kill-switch rather than a name allowlist. --- ## 3. ClassTag logic should not grow `StaticTypeCheckingVisitor` Roughly 250 lines of cohesive feature logic were added to a visitor already past 7k lines. The package already factors helpers into types such as `StaticTypeCheckingSupport`. **Suggestion:** Extract matching/resolution into a dedicated helper (or Support methods) and leave a thin call site in the visitor. **Also:** The block was inserted between `findMethodsWithGenerated`’s Javadoc and the method body, which orphans that documentation. Please restore adjacency. --- ## 4. Selection model and contract gaps - Arity-only matching with deferred applicability forces the mutate/rollback design; fold reifiability and remaining-parameter applicability into one pass so a failed match never touches the AST. - When equal-token candidates disagree on erased types, injection is abandoned silently—consider a diagnostic or a clearer failure policy for API authors. - Tokens may be resolved against one receiver while selection binds on another (multi-receiver / delegate-owner cases). Bind `(receiver, match)` together. - `@ClassTag` documentation emphasises trailing `Class` arguments, but any parameter position is supported; preemption is undocumented on the annotation. Align the docs with behaviour. - On source methods, `@ClassTag` on a non-`Class` parameter should error rather than silently disable injection (same class of bug as the override typo check). -- 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]
