[
https://issues.apache.org/jira/browse/GROOVY-11970?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18076830#comment-18076830
]
ASF GitHub Bot commented on GROOVY-11970:
-----------------------------------------
Copilot commented on code in PR #2499:
URL: https://github.com/apache/groovy/pull/2499#discussion_r3154088844
##########
src/main/java/org/codehaus/groovy/transform/OperatorRenameASTTransformation.java:
##########
@@ -122,9 +137,20 @@ public Expression transform(Expression expr) {
if (expr == null) return null;
if (expr instanceof BinaryExpression be) {
int type = be.getOperation().getType();
+ boolean isEqualOperator = removeAssignment(type) != type;
+ // GEP-15: a *Assign rename takes precedence over the base rename
for compound-assign tokens
+ if (isEqualOperator) {
+ String assignOldName = getAssignOperationName(type);
+ if (assignOldName != null &&
assignNameTable.containsKey(assignOldName)) {
+ Expression left = transform(be.getLeftExpression());
+ Expression right = transform(be.getRightExpression());
+ Expression result = callX(left,
assignNameTable.get(assignOldName), right);
+ result.setSourcePosition(be);
+ return result;
+ }
Review Comment:
The `plusAssign` (and other `*Assign`) rename path rewrites `x op= y` to a
plain method call and returns that call expression. This changes
compound-assignment semantics in expression context: `(x += y)` should evaluate
to the (mutated) `x`, not the return value of `x.<assignMethod>(y)` (often
`void`/`null`). The transform should preserve the original expression value
contract while still skipping reassignment/setter.
##########
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java:
##########
@@ -524,6 +524,31 @@ static String getOperationName(final int op) {
};
}
+ /**
+ * 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;
+ };
Review 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.
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionMultiTypeDispatcher.java:
##########
@@ -332,6 +332,15 @@ protected void
evaluateBinaryExpressionWithAssignment(final String method, final
super.evaluateBinaryExpressionWithAssignment(method, binExp);
}
+ @Override
+ protected void evaluateCompoundAssign(final String assignName, final
String baseName, final BinaryExpression binExp) {
+ // GEP-15: keep the primitive-int/long/float/double fast paths for int
i += j etc.;
+ // they cannot have a *Assign override anyway since primitives are
excluded.
+ if (doAssignmentToArray(binExp)) return;
+ if (doAssignmentToLocalVariable(baseName, binExp)) return;
+ super.evaluateCompoundAssign(assignName, baseName, binExp);
+ }
+
Review Comment:
`evaluateCompoundAssign` currently reuses
`doAssignmentToLocalVariable(baseName, ...)`, which emits bytecode for `x =
x.<baseOp>(y)` and bypasses the new `compoundAssign` dispatcher. For
non-primitive locals this will ignore a receiver's `*Assign` overload (e.g.
`plusAssign`) in dynamic mode. Limit the local-variable fast path to true
primitive cases (or delegate to `super.evaluateCompoundAssign(...)` for
non-primitive LHS) so `*Assign` is still discoverable.
```suggestion
Expression left = binExp.getLeftExpression();
if (left instanceof VariableExpression) {
ClassNode leftType =
controller.getTypeChooser().resolveType(left, controller.getClassNode());
if (isPrimitiveType(leftType) &&
doAssignmentToLocalVariable(baseName, binExp)) return;
}
super.evaluateCompoundAssign(assignName, baseName, binExp);
}
private static boolean isPrimitiveType(final ClassNode type) {
return type == boolean_TYPE
|| type == byte_TYPE
|| type == char_TYPE
|| type == double_TYPE
|| type == float_TYPE
|| type == int_TYPE
|| type == long_TYPE
|| type == short_TYPE;
}
```
##########
src/test/groovy/groovy/operator/CompoundAssignmentTest.groovy:
##########
@@ -0,0 +1,301 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.operator
+
+import groovy.test.GroovyTestCase
+
+/**
+ * GEP-15: tests for dedicated compound-assignment operator overloads
+ * (plusAssign, minusAssign, ...).
+ *
+ * Test scripts use a uniquely-named entry method (not run()) so they do not
+ * collide with the script class's auto-generated run() method.
+ */
+final class CompoundAssignmentTest extends GroovyTestCase {
+
+ void testStaticPlusAssignChosenOverPlus() {
+ assertScript '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ class Acc {
+ int total = 0
+ int plusCalls = 0
+ int plusAssignCalls = 0
+ void plusAssign(int n) { total += n; plusAssignCalls++ }
+ Acc plus(int n) { plusCalls++; def a = new Acc(); a.total =
total + n; a }
+ }
+
+ @CompileStatic
+ void exercise() {
+ def a = new Acc()
+ a += 5
+ assert a.total == 5
+ assert a.plusAssignCalls == 1
+ assert a.plusCalls == 0
+ def b = a + 10
+ assert a.plusCalls == 1
+ assert b.total == 15
+ }
+ exercise()
+ '''
+ }
+
+ void testStaticPlusFallbackWhenNoPlusAssign() {
+ assertScript '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ class V {
+ int n = 0
+ V plus(int x) { def v = new V(); v.n = n + x; v }
+ }
+
+ @CompileStatic
+ void exercise() {
+ def v = new V()
+ v += 7
+ assert v.n == 7
+ }
+ exercise()
+ '''
+ }
+
+ void testStaticExpressionValueIsReceiver() {
+ assertScript '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ class C {
+ int n = 0
+ void plusAssign(int x) { n += x }
+ }
+
+ @CompileStatic
+ void exercise() {
+ def c = new C()
+ def r = (c += 4)
+ assert c.n == 4
+ assert r.is(c)
+ }
+ exercise()
+ '''
+ }
+
+ void testDynamicPlusAssignChosenOverPlus() {
+ assertScript '''
+ class Acc {
+ int total = 0
+ int plusCalls = 0
+ int plusAssignCalls = 0
+ void plusAssign(int n) { total += n; plusAssignCalls++ }
+ Acc plus(int n) { plusCalls++; def a = new Acc(); a.total =
total + n; a }
+ }
+
+ def a = new Acc()
+ a += 5
+ assert a.total == 5
+ assert a.plusAssignCalls == 1
+ assert a.plusCalls == 0
+ '''
+ }
+
+ void testDynamicPlusFallbackWhenNoPlusAssign() {
+ assertScript '''
+ class V {
+ int n = 0
+ V plus(int x) { def v = new V(); v.n = n + x; v }
+ }
+
+ def v = new V()
+ v += 7
+ assert v.n == 7
+ '''
+ }
+
+ void testStaticPropertyCompoundAssignSkipsSetter() {
+ assertScript '''
+ import groovy.transform.CompileStatic
+
+ class Counter {
+ int n = 0
+ void plusAssign(int x) { n += x }
+ }
+
+ @CompileStatic
+ class Holder {
+ Counter counter = new Counter()
+ int setCounterCalls = 0
+ void setCounter(Counter c) { setCounterCalls++; this.@counter
= c }
+ }
+
+ @CompileStatic
+ void exercise() {
+ def h = new Holder()
+ h.counter += 3
+ assert h.counter.n == 3
+ assert h.setCounterCalls == 0
+ }
+ exercise()
+ '''
+ }
+
+ void testStaticPrimitiveFastPathUnaffected() {
+ assertScript '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ int sum(int n) {
+ int total = 0
+ for (i in 0..n) total += i
+ total
+ }
+
+ assert sum(10) == 55
+ '''
+ }
+
+ void testStaticFinalLocalWithPlusAssign() {
+ assertScript '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ class Counter {
+ int n = 0
+ void plusAssign(int x) { n += x }
+ }
+
+ @CompileStatic
+ void exercise() {
+ final Counter c = new Counter()
+ c += 5
+ assert c.n == 5
+ }
+ exercise()
+ '''
+ }
+
+ void testStaticFinalFieldWithPlusAssign() {
+ assertScript '''
+ import groovy.transform.CompileStatic
+
+ @CompileStatic
+ class Counter {
+ int n = 0
+ void plusAssign(int x) { n += x }
+ }
+
+ @CompileStatic
+ class Holder {
+ final Counter c = new Counter()
+ }
+
+ @CompileStatic
+ void exercise() {
+ def h = new Holder()
+ h.c += 5
+ assert h.c.n == 5
+ }
+ exercise()
+ '''
+ }
+
+ void testOperatorRenameAssignVariant() {
+ assertScript '''
+ import groovy.transform.OperatorRename
+
+ class Bag {
+ int total = 0
+ void addInPlace(int n) { total += n }
+ int add(int n) { total + n }
+ }
+
+ @OperatorRename(plus="add", plusAssign="addInPlace")
+ def exercise() {
+ def b = new Bag()
+ b += 5
+ assert b.total == 5
+ def s = b + 3
+ assert s == 8
+ assert b.total == 5
+ }
+ exercise()
+ '''
+ }
Review Comment:
There are tests for expression-value semantics of `+=` in general, but none
covering the `@OperatorRename(… plusAssign=…)` path. Adding a test like `def r
= (b += 1); assert r.is(b)` (or equivalent) would help catch regressions where
the rename transform accidentally returns the `*Assign` method's return value
instead of the mutated receiver.
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java:
##########
@@ -755,6 +760,44 @@ protected void
evaluateBinaryExpressionWithAssignment(final String method, final
controller.getCompileStack().popLHS();
}
+ /**
+ * GEP-15: dynamic-mode compound-assign codegen. Routes through
+ * {@link ScriptBytecodeAdapter#compoundAssign(Object, Object, String,
String)}
+ * which dispatches to {@code assignName} when the receiver responds to it,
+ * and falls back to {@code baseName} otherwise. The caller stores the
helper's
+ * return value into the LHS — for the in-place branch this is a no-op
store
+ * of the receiver back to itself; for the fallback branch it is the usual
+ * "x = x.op(y)" assignment.
+ */
+ 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)
+ })
Review 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.
##########
src/main/java/org/codehaus/groovy/transform/OperatorRenameASTTransformation.java:
##########
@@ -140,6 +166,24 @@ public Expression transform(Expression expr) {
return expr.transformExpression(this);
}
+ 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;
+ };
+ }
Review 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.
##########
src/spec/doc/core-operators.adoc:
##########
@@ -1007,3 +1007,37 @@ Here is a complete list of the operators and their
corresponding methods:
| `~a`
| a.bitwiseNegate()
|====
+
+[[Compound-Assignment-Overloading]]
+=== Compound assignment operator overloading
+
+Compound assignment operators (`+=`, `-=`, `\*=`, ...) normally desugar to `x
= x.op(y)`,
+which creates a new value and reassigns the variable. Since Groovy 5.0
(GEP-15), classes can
+define dedicated *Assign methods to override compound-assign independently of
the base operator.
+When a `*Assign` method exists on the receiver type it is called directly: the
receiver is mutated
+in place, no reassignment occurs (so `final` fields and variables can use
compound assignment),
+and for property LHS the setter is *not* invoked.
Review Comment:
The text implies `final` locals/fields can use compound assignment in
general, but the compiler-side final-reassignment suppression currently keys
off `StaticTypesMarker.COMPOUND_ASSIGN_TARGET`, which is only set when the
static type checker resolves a `*Assign` target. If dynamic compilation (no
STC) is still expected to reject `final x += y`, this section should clarify
that the `final` relaxation applies when the compiler can resolve a `*Assign`
method (e.g. `@CompileStatic`). Otherwise, consider extending the
final-variable analysis to cover dynamic compilation when the receiver type is
known.
```suggestion
When a `*Assign` method exists on the receiver type and the compiler can
resolve it directly
(for example, under `@CompileStatic`), it is called directly: the receiver
is mutated in place,
no reassignment occurs (so `final` fields and variables can use compound
assignment in that
case), and for property LHS the setter is *not* invoked.
```
> Provide support for compound assignment operator overloading (GEP-15)
> ---------------------------------------------------------------------
>
> Key: GROOVY-11970
> URL: https://issues.apache.org/jira/browse/GROOVY-11970
> Project: Groovy
> Issue Type: New Feature
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
