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.
```
--
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]
