GROOVY-11970: Provide support for compound assignment operator overlo…#2499
GROOVY-11970: Provide support for compound assignment operator overlo…#2499paulk-asert wants to merge 1 commit intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds GEP-15 support for dedicated compound-assignment operator overloads (e.g. plusAssign) across static type checking, bytecode generation, @OperatorRename, documentation, and tests.
Changes:
- Introduces
*Assignmethod resolution in the static type checker and propagates the resolved target to static codegen. - Adds dynamic-mode compound-assign dispatch via
ScriptBytecodeAdapter.compoundAssignand updates bytecode generation to use it. - Documents the feature in the operator spec and adds a focused test suite for compound assignment behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/groovy/groovy/operator/CompoundAssignmentTest.groovy | New tests covering static/dynamic *Assign selection, fallback behavior, final handling (static), setter skipping (static), and @OperatorRename scenarios. |
| src/spec/doc/core-operators.adoc | Documents compound assignment operator overloading and method mapping table. |
| src/main/java/org/codehaus/groovy/transform/stc/StaticTypesMarker.java | Adds metadata key to carry resolved *Assign target from STC into codegen. |
| src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java | Resolves *Assign methods for compound-assign tokens and records the chosen target; skips setter-info when in-place mutation applies. |
| src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java | Adds token→*Assign method-name mapping helper. |
| src/main/java/org/codehaus/groovy/transform/OperatorRenameASTTransformation.java | Extends @OperatorRename to support *Assign renames with precedence over base operator renames. |
| src/main/java/org/codehaus/groovy/syntax/Types.java | Treats REMAINDER_EQUAL as an assignment operator token. |
| src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java | Adds dynamic dispatcher helper for *Assign vs fallback operator call. |
| src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java | Emits in-place *Assign calls during static compilation when STC resolved a target; maintains legacy property-set fast-path. |
| src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionMultiTypeDispatcher.java | Overrides compound-assign emission to retain primitive/array fast paths. |
| src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java | Routes compound-assign tokens through new evaluateCompoundAssign helper-based path. |
| src/main/java/org/codehaus/groovy/classgen/Verifier.java | Skips final-variable reassignment checks for compound assigns marked as in-place via STC metadata. |
| src/main/java/groovy/transform/OperatorRename.java | Adds annotation members for *Assign renames. |
| protected void evaluateCompoundAssign(final String assignName, final String baseName, final BinaryExpression expression) { | ||
| Expression leftExpression = expression.getLeftExpression(); | ||
| if (leftExpression instanceof BinaryExpression bexp | ||
| && bexp.getOperation().getType() == LEFT_SQUARE_BRACKET) { | ||
| // Subscript LHS (e.g. a[i] += b) is intentionally out of scope for GEP-15; | ||
| // keep the legacy getAt/putAt-based path. | ||
| evaluateArrayAssignmentWithOperator(baseName, expression, bexp); | ||
| return; | ||
| } | ||
|
|
||
| StaticMethodCallExpression helperCall = new StaticMethodCallExpression( | ||
| ClassHelper.make(ScriptBytecodeAdapter.class), | ||
| "compoundAssign", | ||
| new ArgumentListExpression(new Expression[]{ | ||
| leftExpression, | ||
| expression.getRightExpression(), | ||
| new ConstantExpression(assignName), | ||
| new ConstantExpression(baseName) | ||
| }) |
There was a problem hiding this comment.
evaluateCompoundAssign doesn't currently account for null-safe compound assignments (expression.isSafe()), because ScriptBytecodeAdapter.compoundAssign will fall back to invoking the base operator even when the receiver is null. This breaks safe-navigation semantics (the operation should short-circuit without invoking plus/etc when safe and the LHS resolves to null). Consider either handling expression.isSafe() here by keeping the legacy safe path, or extending the helper to treat safe && receiver==null as a short-circuit.
| /** | ||
| * GEP-15: returns the dedicated compound-assignment method name for a compound-assign | ||
| * operator token (e.g. {@code PLUS_EQUAL} -> {@code "plusAssign"}), or {@code null} if | ||
| * the token is not one of the twelve operators in scope. {@code INTDIV_EQUAL}, | ||
| * {@code MOD_EQUAL}, {@code ELVIS_EQUAL}, {@code LOGICAL_OR_EQUAL} and | ||
| * {@code LOGICAL_AND_EQUAL} are intentionally excluded. | ||
| */ | ||
| public static String getAssignOperationName(final int op) { | ||
| return switch (op) { | ||
| case PLUS_EQUAL -> "plusAssign"; | ||
| case MINUS_EQUAL -> "minusAssign"; | ||
| case MULTIPLY_EQUAL -> "multiplyAssign"; | ||
| case DIVIDE_EQUAL -> "divAssign"; | ||
| case REMAINDER_EQUAL -> "remainderAssign"; | ||
| case POWER_EQUAL -> "powerAssign"; | ||
| case LEFT_SHIFT_EQUAL -> "leftShiftAssign"; | ||
| case RIGHT_SHIFT_EQUAL -> "rightShiftAssign"; | ||
| case RIGHT_SHIFT_UNSIGNED_EQUAL -> "rightShiftUnsignedAssign"; | ||
| case BITWISE_AND_EQUAL -> "andAssign"; | ||
| case BITWISE_OR_EQUAL -> "orAssign"; | ||
| case BITWISE_XOR_EQUAL -> "xorAssign"; | ||
| default -> null; | ||
| }; |
There was a problem hiding this comment.
getAssignOperationName only maps REMAINDER_EQUAL, but Groovy still defines/uses MOD_EQUAL (%=) as a distinct token in some parser paths. Since codegen now routes MOD_EQUAL through remainderAssign (see BinaryExpressionHelper), the STC helper should likely map MOD_EQUAL to "remainderAssign" as well; otherwise statically compiled %= may miss remainderAssign depending on which token the frontend produces.
| static String getAssignOperationName(final int op) { | ||
| return switch (op) { | ||
| case PLUS_EQUAL -> "plusAssign"; | ||
| case MINUS_EQUAL -> "minusAssign"; | ||
| case MULTIPLY_EQUAL -> "multiplyAssign"; | ||
| case DIVIDE_EQUAL -> "divAssign"; | ||
| case REMAINDER_EQUAL -> "remainderAssign"; | ||
| case POWER_EQUAL -> "powerAssign"; | ||
| case LEFT_SHIFT_EQUAL -> "leftShiftAssign"; | ||
| case RIGHT_SHIFT_EQUAL -> "rightShiftAssign"; | ||
| case RIGHT_SHIFT_UNSIGNED_EQUAL -> "rightShiftUnsignedAssign"; | ||
| case BITWISE_AND_EQUAL -> "andAssign"; | ||
| case BITWISE_OR_EQUAL -> "orAssign"; | ||
| case BITWISE_XOR_EQUAL -> "xorAssign"; | ||
| default -> null; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Same token-mapping issue exists here: getAssignOperationName doesn't include MOD_EQUAL, so @OperatorRename(… plusAssign=…) precedence logic may not trigger for %= depending on which token (MOD_EQUAL vs REMAINDER_EQUAL) the parser emits. Consider mapping both tokens to remainderAssign for consistency with codegen and the user-facing operator table.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2499 +/- ##
==================================================
+ Coverage 67.1374% 67.1516% +0.0142%
- Complexity 31622 31659 +37
==================================================
Files 1451 1451
Lines 122565 122694 +129
Branches 22007 22020 +13
==================================================
+ Hits 82287 82391 +104
- Misses 33199 33222 +23
- Partials 7079 7081 +2
🚀 New features to boost your workflow:
|
paulk-asert
force-pushed
the
groovy11970
branch
from
d228b36 to
a49310e
Compare
| return receiver; | ||
| } | ||
| } | ||
| return InvokerHelper.invokeMethod(receiver, baseName, arg); |
There was a problem hiding this comment.
SCB already has methods for invocation, why not use those?
| public static Object invokeRenamedCompoundAssign(final Object receiver, final Object arg, | ||
| final String name) throws Throwable { | ||
| InvokerHelper.invokeMethod(receiver, name, arg); | ||
| return receiver; |
There was a problem hiding this comment.
why not use a normal method call for this then? I don't see why we need an extra method in SCB for this
…ading (GEP-15)