Skip to content

GROOVY-11970: Provide support for compound assignment operator overlo… #2499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
paulk-asert wants to merge 1 commit into
base: master
Choose a base branch
from
Open

GROOVY-11970: Provide support for compound assignment operator overlo… #2499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/main/java/groovy/transform/OperatorRename.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,28 @@
String or() default Undefined.STRING;
String xor() default Undefined.STRING;
String compareTo() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code +=}. */
String plusAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code -=}. */
String minusAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code *=}. */
String multiplyAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code /=}. */
String divAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code %=}. */
String remainderAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code **=}. */
String powerAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code <<=}. */
String leftShiftAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code >>=}. */
String rightShiftAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code >>>=}. */
String rightShiftUnsignedAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code &=}. */
String andAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code |=}. */
String orAssign() default Undefined.STRING;
/** GEP-15: rename the dedicated compound-assignment method for {@code ^=}. */
String xorAssign() default Undefined.STRING;
}
7 changes: 7 additions & 0 deletions src/main/java/org/codehaus/groovy/classgen/Verifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@
import static org.codehaus.groovy.ast.tools.GenericsUtils.parameterizeType;
import static org.codehaus.groovy.ast.tools.PropertyNodeUtils.adjustPropertyModifiersForMethod;
import static org.codehaus.groovy.transform.sc.StaticCompilationMetadataKeys.STATIC_COMPILE_NODE;
import static org.codehaus.groovy.transform.stc.StaticTypesMarker.COMPOUND_ASSIGN_TARGET;
import static org.codehaus.groovy.transform.stc.StaticTypesMarker.DIRECT_METHOD_CALL_TARGET;

/**
Expand Down Expand Up @@ -349,6 +350,12 @@ public void variableNotFinal(Variable var, final Expression bexp) {
if (var instanceof VariableExpression) {
var = ((VariableExpression) var).getAccessedVariable();
}
// GEP-15: a compound-assign that resolved to a *Assign method (e.g. plusAssign)
// mutates the receiver in place rather than reassigning the variable, so the
// final-reassignment check must not fire for it.
if (bexp instanceof BinaryExpression be && be.getNodeMetaData(COMPOUND_ASSIGN_TARGET) != null) {
return;
}
if (var instanceof VariableExpression && isFinal(var.getModifiers())) {
throw new RuntimeParserException("The variable [" + var.getName() + "] is declared final but is reassigned", bexp);
}
Expand Down
69 changes: 56 additions & 13 deletions src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.codehaus.groovy.ast.ClassHelper;
import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.Variable;
import org.codehaus.groovy.ast.expr.ArgumentListExpression;
import org.codehaus.groovy.ast.expr.ArrayExpression;
import org.codehaus.groovy.ast.expr.BinaryExpression;
import org.codehaus.groovy.ast.expr.ClassExpression;
Expand All @@ -35,6 +36,7 @@
import org.codehaus.groovy.ast.expr.PostfixExpression;
import org.codehaus.groovy.ast.expr.PrefixExpression;
import org.codehaus.groovy.ast.expr.PropertyExpression;
import org.codehaus.groovy.ast.expr.StaticMethodCallExpression;
import org.codehaus.groovy.ast.expr.TernaryExpression;
import org.codehaus.groovy.ast.expr.TupleExpression;
import org.codehaus.groovy.ast.expr.VariableExpression;
Expand Down Expand Up @@ -205,15 +207,15 @@ public void eval(final BinaryExpression expression) {
break;

case BITWISE_AND_EQUAL:
evaluateBinaryExpressionWithAssignment("and", expression);
evaluateCompoundAssign("andAssign", "and", expression);
break;

case BITWISE_OR:
evaluateBinaryExpression("or", expression);
break;

case BITWISE_OR_EQUAL:
evaluateBinaryExpressionWithAssignment("or", expression);
evaluateCompoundAssign("orAssign", "or", expression);
break;

case BITWISE_XOR:
Expand All @@ -225,31 +227,31 @@ public void eval(final BinaryExpression expression) {
break;

case BITWISE_XOR_EQUAL:
evaluateBinaryExpressionWithAssignment("xor", expression);
evaluateCompoundAssign("xorAssign", "xor", expression);
break;

case PLUS:
evaluateBinaryExpression("plus", expression);
break;

case PLUS_EQUAL:
evaluateBinaryExpressionWithAssignment("plus", expression);
evaluateCompoundAssign("plusAssign", "plus", expression);
break;

case MINUS:
evaluateBinaryExpression("minus", expression);
break;

case MINUS_EQUAL:
evaluateBinaryExpressionWithAssignment("minus", expression);
evaluateCompoundAssign("minusAssign", "minus", expression);
break;

case MULTIPLY:
evaluateBinaryExpression("multiply", expression);
break;

case MULTIPLY_EQUAL:
evaluateBinaryExpressionWithAssignment("multiply", expression);
evaluateCompoundAssign("multiplyAssign", "multiply", expression);
break;

case DIVIDE:
Expand All @@ -259,14 +261,15 @@ public void eval(final BinaryExpression expression) {
case DIVIDE_EQUAL:
//SPG don't use divide since BigInteger implements directly
//and we want to dispatch through DefaultGroovyMethods to get a BigDecimal result
evaluateBinaryExpressionWithAssignment("div", expression);
evaluateCompoundAssign("divAssign", "div", expression);
break;

case INTDIV:
evaluateBinaryExpression("intdiv", expression);
break;

case INTDIV_EQUAL:
// GEP-15 explicitly excludes \= (no intdivAssign convention)
evaluateBinaryExpressionWithAssignment("intdiv", expression);
break;

Expand All @@ -275,23 +278,25 @@ public void eval(final BinaryExpression expression) {
break;

case MOD_EQUAL:
evaluateBinaryExpressionWithAssignment("mod", expression);
// GEP-15 maps both MOD_EQUAL and REMAINDER_EQUAL to remainderAssign for consistency
// with getOperationName collapse, even though current parser only emits REMAINDER_EQUAL.
evaluateCompoundAssign("remainderAssign", "mod", expression);
break;

case REMAINDER:
evaluateBinaryExpression("remainder", expression);
break;

case REMAINDER_EQUAL:
evaluateBinaryExpressionWithAssignment("remainder", expression);
evaluateCompoundAssign("remainderAssign", "remainder", expression);
break;

case POWER:
evaluateBinaryExpression("power", expression);
break;

case POWER_EQUAL:
evaluateBinaryExpressionWithAssignment("power", expression);
evaluateCompoundAssign("powerAssign", "power", expression);
break;

case ELVIS_EQUAL:
Expand All @@ -303,23 +308,23 @@ public void eval(final BinaryExpression expression) {
break;

case LEFT_SHIFT_EQUAL:
evaluateBinaryExpressionWithAssignment("leftShift", expression);
evaluateCompoundAssign("leftShiftAssign", "leftShift", expression);
break;

case RIGHT_SHIFT:
evaluateBinaryExpression("rightShift", expression);
break;

case RIGHT_SHIFT_EQUAL:
evaluateBinaryExpressionWithAssignment("rightShift", expression);
evaluateCompoundAssign("rightShiftAssign", "rightShift", expression);
break;

case RIGHT_SHIFT_UNSIGNED:
evaluateBinaryExpression("rightShiftUnsigned", expression);
break;

case RIGHT_SHIFT_UNSIGNED_EQUAL:
evaluateBinaryExpressionWithAssignment("rightShiftUnsigned", expression);
evaluateCompoundAssign("rightShiftUnsignedAssign", "rightShiftUnsigned", expression);
break;

case KEYWORD_INSTANCEOF:
Expand Down Expand Up @@ -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)
})
Comment on lines +772 to +790
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
);
helperCall.setSourcePosition(expression);
helperCall.visit(controller.getAcg());

controller.getOperandStack().dup();
controller.getCompileStack().pushLHS(true);
leftExpression.visit(controller.getAcg());
controller.getCompileStack().popLHS();
}

private void evaluateInstanceof(final BinaryExpression expression) {
CompileStack compileStack = controller.getCompileStack();
OperandStack operandStack = controller.getOperandStack();
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionMultiTypeDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Comment thread
paulk-asert marked this conversation as resolved.
private boolean doAssignmentToLocalVariable(final String method, final BinaryExpression binExp) {
Expression left = binExp.getLeftExpression();
if (left instanceof VariableExpression ve) {
Expand Down
48 changes: 46 additions & 2 deletions ...a/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
import static org.codehaus.groovy.transform.sc.StaticCompilationVisitor.ARRAYLIST_CONSTRUCTOR;
import static org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.isAssignment;
import static org.codehaus.groovy.transform.stc.StaticTypeCheckingVisitor.inferLoopElementType;
import static org.codehaus.groovy.transform.stc.StaticTypesMarker.COMPOUND_ASSIGN_TARGET;
import static org.codehaus.groovy.transform.stc.StaticTypesMarker.DIRECT_METHOD_CALL_TARGET;
import static org.codehaus.groovy.transform.stc.StaticTypesMarker.INFERRED_TYPE;
import static org.objectweb.asm.Opcodes.ACC_PUBLIC;
Expand Down Expand Up @@ -136,6 +137,28 @@ private static void visitInsnByType(final ClassNode top, final MethodVisitor mv,

@Override
protected void evaluateBinaryExpressionWithAssignment(final String method, final BinaryExpression expression) {
if (tryStaticCompoundAssignPaths(method, expression)) return;
super.evaluateBinaryExpressionWithAssignment(method, expression);
}

@Override
protected void evaluateCompoundAssign(final String assignName, final String baseName, final BinaryExpression expression) {
if (tryStaticCompoundAssignPaths(baseName, expression)) return;
super.evaluateCompoundAssign(assignName, baseName, expression);
}

/**
* GEP-15 + legacy setter fast-path. Returns true when codegen has been emitted
* (no further dispatch required).
*/
private boolean tryStaticCompoundAssignPaths(final String baseName, final BinaryExpression expression) {
MethodNode assignTarget = expression.getNodeMetaData(COMPOUND_ASSIGN_TARGET);
if (assignTarget != null) {
// GEP-15: receiver.<assignMethod>(arg); receiver remains the expression value.
// The setter (for property LHS) is intentionally skipped.
emitCompoundAssignCall(assignTarget, expression);
return true;
}
Expression leftExpression = expression.getLeftExpression();
if (leftExpression instanceof PropertyExpression pexp
&& !(leftExpression instanceof AttributeExpression)) {
Expand All @@ -161,10 +184,31 @@ protected void evaluateBinaryExpressionWithAssignment(final String method, final
pexp.isSpreadSafe(),
pexp.isImplicitThis(),
true)) { // TODO: GROOVY-11843
return;
return true;
}
}
super.evaluateBinaryExpressionWithAssignment(method, expression);
return false;
}

private void emitCompoundAssignCall(final MethodNode target, final BinaryExpression expression) {
OperandStack operandStack = controller.getOperandStack();
Expression leftExpression = expression.getLeftExpression();
Expression rightExpression = expression.getRightExpression();

leftExpression.visit(controller.getAcg());
ClassNode receiverType = operandStack.getTopOperand();
int slot = controller.getCompileStack().defineTemporaryVariable("$gep15recv", receiverType, true);

VariableSlotLoader callReceiver = new VariableSlotLoader(receiverType, slot, operandStack);
MethodCallExpression call = callX(callReceiver, target.getName(), rightExpression);
call.setMethodTarget(target);
call.setImplicitThis(false);
call.setSourcePosition(expression);
call.visit(controller.getAcg());
operandStack.pop(); // discard the *Assign return value

new VariableSlotLoader(receiverType, slot, operandStack).visit(controller.getAcg());
controller.getCompileStack().removeVar(slot);
}

@Override
Expand Down
30 changes: 30 additions & 0 deletions src/main/java/org/codehaus/groovy/runtime/ScriptBytecodeAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,4 +998,34 @@ public static Object bitwiseNegate(final Object value) throws Throwable {
throw unwrap(gre);
}
}

/**
* GEP-15: dispatcher for compound-assignment operators in dynamic Groovy.
* If {@code receiver} responds to {@code assignName} with the supplied argument,
* invoke it and return {@code receiver} (so the caller's STORE assigns the same
* reference back, leaving the in-place mutation visible). Otherwise fall back to
* {@code baseName} and return its result for the caller to assign.
*/
public static Object compoundAssign(final Object receiver, final Object arg,
final String assignName, final String baseName) throws Throwable {
if (receiver != null) {
MetaClass mc = InvokerHelper.getMetaClass(receiver);
if (!mc.respondsTo(receiver, assignName, new Object[]{arg}).isEmpty()) {
mc.invokeMethod(receiver, assignName, arg);
return receiver;
}
}
return InvokerHelper.invokeMethod(receiver, baseName, arg);
Copy link
Copy Markdown
Contributor

@blackdrag blackdrag Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SCB already has methods for invocation, why not use those?

}

/**
* GEP-15: helper for {@code @OperatorRename(plusAssign="...")} expression rewrites.
* The user's renamed name is authoritative (no fallback), and the expression value of
* {@code x op= y} is the (mutated) {@code x}, not the called method's return value.
*/
public static Object invokeRenamedCompoundAssign(final Object receiver, final Object arg,
final String name) throws Throwable {
InvokerHelper.invokeMethod(receiver, name, arg);
return receiver;
Copy link
Copy Markdown
Contributor

@blackdrag blackdrag Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use a normal method call for this then? I don't see why we need an extra method in SCB for this

}
}
3 changes: 2 additions & 1 deletion src/main/java/org/codehaus/groovy/syntax/Types.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,8 @@ public static boolean ofType(int specific, int general) {
case ASSIGNMENT_OPERATOR:
return specific == EQUAL || (specific >= PLUS_EQUAL && specific <= ELVIS_EQUAL) || (specific >= LOGICAL_OR_EQUAL && specific <= LOGICAL_AND_EQUAL)
|| (specific >= LEFT_SHIFT_EQUAL && specific <= RIGHT_SHIFT_UNSIGNED_EQUAL)
|| (specific >= BITWISE_OR_EQUAL && specific <= BITWISE_XOR_EQUAL);
|| (specific >= BITWISE_OR_EQUAL && specific <= BITWISE_XOR_EQUAL)
|| specific == REMAINDER_EQUAL;

case COMPARISON_OPERATOR:
return specific >= COMPARE_NOT_EQUAL && specific <= COMPARE_TO;
Expand Down
Loading
Loading