Skip to content

Commit

Permalink
GROOVY-11288: do not load assignment expression value for expr statement
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Oct 9, 2024
1 parent 379ad88 commit cdb69c9
Show file tree
Hide file tree
Showing 8 changed files with 370 additions and 319 deletions.
13 changes: 9 additions & 4 deletions src/main/java/org/codehaus/groovy/ast/expr/DeclarationExpression.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public String getText() {
if (v.isDynamicTyped()) {
text.append("def");
} else {
text.append(formatTypeName(v.getType()));
text.append(formatTypeName(v.getOriginType()));
}
text.append(' ').append(v.getText());
} else {
Expand All @@ -149,16 +149,21 @@ public String getText() {
if (e instanceof VariableExpression) {
VariableExpression v = (VariableExpression) e;
if (!v.isDynamicTyped()) {
text.append(formatTypeName(v.getType())).append(' ');
text.append(formatTypeName(v.getOriginType())).append(' ');
}
}
text.append(e.getText()).append(", ");
}
text.setLength(text.length() - 2);
text.append(')');
}
text.append(' ').append(getOperation().getText());
text.append(' ').append(getRightExpression().getText());

if (getRightExpression() instanceof EmptyExpression) {
text.append(';');
} else {
text.append(' ').append(getOperation().getText());
text.append(' ').append(getRightExpression().getText());
}

return text.toString();
}
Expand Down
91 changes: 48 additions & 43 deletions src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,16 @@
import static org.codehaus.groovy.syntax.Types.RIGHT_SHIFT_UNSIGNED_EQUAL;
import static org.objectweb.asm.Opcodes.ACONST_NULL;
import static org.objectweb.asm.Opcodes.ALOAD;
import static org.objectweb.asm.Opcodes.DCONST_0;
import static org.objectweb.asm.Opcodes.DUP;
import static org.objectweb.asm.Opcodes.FCONST_0;
import static org.objectweb.asm.Opcodes.GOTO;
import static org.objectweb.asm.Opcodes.ICONST_0;
import static org.objectweb.asm.Opcodes.IFEQ;
import static org.objectweb.asm.Opcodes.IFNE;
import static org.objectweb.asm.Opcodes.IF_ACMPEQ;
import static org.objectweb.asm.Opcodes.INSTANCEOF;
import static org.objectweb.asm.Opcodes.LCONST_0;
import static org.objectweb.asm.Opcodes.POP;

public class BinaryExpressionHelper {
Expand Down Expand Up @@ -401,40 +405,41 @@ public void evaluateEqual(final BinaryExpression expression, final boolean defin
Expression rightExpression = expression.getRightExpression();
boolean singleAssignment = !(leftExpression instanceof TupleExpression);
boolean directAssignment = defineVariable && singleAssignment; //def x=y
boolean returnRightValue = !Boolean.TRUE.equals(expression.getNodeMetaData("GROOVY-11288"));

// TODO: LHS has not been visited, it could be a variable in a closure and type chooser is not aware.
// TODO: LHS has not been visited -- it could be a variable in a closure and type chooser is not aware.
ClassNode lhsType = controller.getTypeChooser().resolveType(leftExpression, controller.getClassNode());

if (directAssignment && rightExpression instanceof EmptyExpression) {
BytecodeVariable v = compileStack.defineVariable((Variable) leftExpression, lhsType, false);
operandStack.loadOrStoreVariable(v, false);
if (returnRightValue) operandStack.loadOrStoreVariable(v, false);
return;
}

// evaluate RHS and store the value
// evaluate RHS and store its value

if (rightExpression instanceof ListExpression && lhsType.isArray()) {
if (lhsType.isArray() && rightExpression instanceof ListExpression) { // array = [ ... ]
Expression array = new ArrayExpression(lhsType.getComponentType(), ((ListExpression) rightExpression).getExpressions());
array.setSourcePosition(rightExpression);
array.visit(acg);
} else if (rightExpression instanceof EmptyExpression) {
loadInitValue(lhsType); // null or zero (or false)
} else if (rightExpression instanceof EmptyExpression) { // define field
/* */ if (ClassHelper.isPrimitiveDouble(lhsType)) {
mv.visitInsn(DCONST_0);
} else if (ClassHelper.isPrimitiveFloat(lhsType)) {
mv.visitInsn(FCONST_0);
} else if (ClassHelper.isPrimitiveLong(lhsType)) {
mv.visitInsn(LCONST_0);
} else if (ClassHelper.isPrimitiveType(lhsType)) {
mv.visitInsn(ICONST_0);
} else {
mv.visitInsn(ACONST_NULL);
}
operandStack.push(lhsType);
} else {
rightExpression.visit(acg);
}

// GROOVY-10918: direct store to local variable or parameter (no temp)
if (!defineVariable && leftExpression instanceof VariableExpression) {
BytecodeVariable v = compileStack.getVariable(leftExpression.getText(), false);
if (v != null) {
operandStack.dup(); // return value of the assignment expression
operandStack.storeVar(v);
return;
}
}

ClassNode rhsType = operandStack.getTopOperand();
int rhsValueId;

if (directAssignment) {
VariableExpression var = (VariableExpression) leftExpression;
Expand All @@ -460,17 +465,33 @@ public void evaluateEqual(final BinaryExpression expression, final boolean defin
} else {
operandStack.doGroovyCast(lhsType);
}
rhsType = lhsType;
rhsValueId = compileStack.defineVariable(var, lhsType, true).getIndex();
} else {
rhsValueId = compileStack.defineTemporaryVariable("$rhs", rhsType, true);

// store value
BytecodeVariable v = compileStack.defineVariable(var, lhsType, true);
operandStack.remove(1);
if (returnRightValue) {
new VariableSlotLoader(lhsType, v.getIndex(), operandStack).visit(acg);
}
return;
}

// GROOVY-10918: direct store to local variable or parameter (no temp)
if (!defineVariable && leftExpression instanceof VariableExpression) {
BytecodeVariable v = compileStack.getVariable(leftExpression.getText(), false);
if (v != null) {
if (returnRightValue) operandStack.dup();
operandStack.storeVar(v);
return;
}
}
// TODO: if RHS is VariableSlotLoader already, then skip creating a new one

int rhsValueId = compileStack.defineTemporaryVariable("$rhs", rhsType, true);
// TODO: if RHS is already a VariableSlotLoader, then skip creating a new one
Expression rhsValueLoader = new VariableSlotLoader(rhsType, rhsValueId, operandStack);

// assignment for subscript
// subscript assignment
if (leftExpression instanceof BinaryExpression) {
BinaryExpression leftBinExpr = (BinaryExpression) leftExpression;
var leftBinExpr = (BinaryExpression) leftExpression;
if (leftBinExpr.getOperation().getType() == LEFT_SQUARE_BRACKET) {
assignToArray(expression, leftBinExpr.getLeftExpression(), leftBinExpr.getRightExpression(), rhsValueLoader, leftBinExpr.isSafe());
}
Expand All @@ -480,13 +501,6 @@ public void evaluateEqual(final BinaryExpression expression, final boolean defin

compileStack.pushLHS(true);

if (directAssignment) {
rhsValueLoader.visit(acg);
operandStack.remove(1);
compileStack.popLHS();
return;
}

if (singleAssignment) {
int mark = operandStack.getStackLength();
rhsValueLoader.visit(acg);
Expand Down Expand Up @@ -567,19 +581,10 @@ public void evaluateEqual(final BinaryExpression expression, final boolean defin

compileStack.popLHS();

// return value of assignment
rhsValueLoader.visit(acg);
compileStack.removeVar(rhsValueId);
}
if (returnRightValue)
rhsValueLoader.visit(acg);

private void loadInitValue(final ClassNode type) {
MethodVisitor mv = controller.getMethodVisitor();
if (ClassHelper.isPrimitiveType(type)) {
mv.visitLdcInsn(0);
} else {
mv.visitInsn(ACONST_NULL);
}
controller.getOperandStack().push(type);
compileStack.removeVar(rhsValueId);
}

protected void evaluateCompareExpression(final MethodCaller compareMethod, final BinaryExpression expression) {
Expand Down
26 changes: 12 additions & 14 deletions src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionMultiTypeDispatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,34 +356,32 @@ private boolean doAssignmentToLocalVariable(final String method, final BinaryExp

@Override
protected void assignToArray(final Expression orig, final Expression receiver, final Expression index, final Expression rhsValueLoader, final boolean safe) {
ClassNode current = controller.getClassNode();
ClassNode arrayType = controller.getTypeChooser().resolveType(receiver, current);
ClassNode arrayComponentType = arrayType.getComponentType();
int operationType = getOperandType(arrayComponentType);
BinaryExpressionWriter bew = binExpWriter[operationType];
AsmClassGenerator acg = controller.getAcg();

ClassNode arrayType = controller.getTypeChooser().resolveType(receiver, controller.getClassNode());
BinaryExpressionWriter bew = binExpWriter[getOperandType(arrayType.getComponentType())];
if (bew.arraySet(true) && arrayType.isArray() && !safe) {
OperandStack operandStack = controller.getOperandStack();
var asmGenerator = controller.getAcg();
var operandStack = controller.getOperandStack();

// load the array
receiver.visit(acg);
receiver.visit(asmGenerator);
operandStack.doGroovyCast(arrayType);

// load index
index.visit(acg);
index.visit(asmGenerator);
operandStack.doGroovyCast(int_TYPE);

// load rhs
rhsValueLoader.visit(acg);
operandStack.doGroovyCast(arrayComponentType);
rhsValueLoader.visit(asmGenerator);
operandStack.doGroovyCast(arrayType.getComponentType());

// store value in array
bew.arraySet(false);

// load return value && correct operand stack
// update operand stack
operandStack.remove(3);
rhsValueLoader.visit(acg);

if (!Boolean.TRUE.equals(orig.getNodeMetaData("GROOVY-11288")))
rhsValueLoader.visit(asmGenerator); // re-load result value
} else {
super.assignToArray(orig, receiver, index, rhsValueLoader, safe);
}
Expand Down
24 changes: 10 additions & 14 deletions src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,12 @@
import java.util.List;
import java.util.Map;

import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveDouble;
import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveFloat;
import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveLong;
import static org.objectweb.asm.Opcodes.ACONST_NULL;
import static org.objectweb.asm.Opcodes.ASTORE;
import static org.objectweb.asm.Opcodes.DCONST_0;
import static org.objectweb.asm.Opcodes.DUP_X1;
import static org.objectweb.asm.Opcodes.FCONST_0;
import static org.objectweb.asm.Opcodes.ICONST_0;
import static org.objectweb.asm.Opcodes.INVOKESPECIAL;
import static org.objectweb.asm.Opcodes.LCONST_0;
import static org.objectweb.asm.Opcodes.NEW;
Expand Down Expand Up @@ -641,16 +639,14 @@ void createReference(final BytecodeVariable reference) {
}

private static void pushInitValue(final ClassNode type, final MethodVisitor mv) {
if (ClassHelper.isPrimitiveType(type)) {
if (isPrimitiveLong(type)) {
mv.visitInsn(LCONST_0);
} else if (isPrimitiveDouble(type)) {
mv.visitInsn(DCONST_0);
} else if (isPrimitiveFloat(type)) {
mv.visitInsn(FCONST_0);
} else {
mv.visitLdcInsn(0);
}
/* */ if (ClassHelper.isPrimitiveDouble(type)) {
mv.visitInsn(DCONST_0);
} else if (ClassHelper.isPrimitiveFloat(type)) {
mv.visitInsn(FCONST_0);
} else if (ClassHelper.isPrimitiveLong(type)) {
mv.visitInsn(LCONST_0);
} else if (ClassHelper.isPrimitiveType(type)) {
mv.visitInsn(ICONST_0);
} else {
mv.visitInsn(ACONST_NULL);
}
Expand Down Expand Up @@ -718,7 +714,7 @@ public boolean containsVariable(final String name) {
*/
private void makeNextVariableID(final ClassNode type, final boolean useReferenceDirectly) {
currentVariableIndex = nextVariableIndex;
if ((isPrimitiveLong(type) || isPrimitiveDouble(type)) && !useReferenceDirectly) {
if (!useReferenceDirectly && (ClassHelper.isPrimitiveLong(type) || ClassHelper.isPrimitiveDouble(type))) {
nextVariableIndex += 1;
}
nextVariableIndex += 1;
Expand Down
14 changes: 10 additions & 4 deletions src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import org.codehaus.groovy.ast.ClassHelper;
import org.codehaus.groovy.ast.ClassNode;
import org.codehaus.groovy.ast.expr.BinaryExpression;
import org.codehaus.groovy.ast.expr.BooleanExpression;
import org.codehaus.groovy.ast.expr.ClosureListExpression;
import org.codehaus.groovy.ast.expr.ConstantExpression;
Expand Down Expand Up @@ -602,12 +603,17 @@ public void writeReturn(final ReturnStatement statement) {
}

public void writeExpressionStatement(final ExpressionStatement statement) {
controller.getAcg().onLineNumber(statement, "visitExpressionStatement: " + statement.getExpression().getClass().getName());
Expression expression = statement.getExpression();

controller.getAcg().onLineNumber(statement, "visitExpressionStatement: " + expression.getClass().getName());
writeStatementLabel(statement);

int mark = controller.getOperandStack().getStackLength();
Expression expression = statement.getExpression();
if (expression instanceof BinaryExpression)
expression.putNodeMetaData("GROOVY-11288", Boolean.TRUE);

var operandStack = controller.getOperandStack();
int mark = operandStack.getStackLength();
expression.visit(controller.getAcg());
controller.getOperandStack().popDownTo(mark);
operandStack.popDownTo(mark);
}
}
11 changes: 5 additions & 6 deletions ...a/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,10 +326,9 @@ private boolean makeSetPrivateFieldWithBridgeMethod(final Expression receiver, f
protected void assignToArray(final Expression enclosing, final Expression receiver, final Expression subscript, final Expression rhsValueLoader, final boolean safe) {
ClassNode receiverType = controller.getTypeChooser().resolveType(receiver, controller.getClassNode());

if (receiverType.isArray() && !safe && binExpWriter[getOperandType(receiverType.getComponentType())].arraySet(true)) {
super.assignToArray(enclosing, receiver, subscript, rhsValueLoader, safe);

} else { // this code path is needed because ACG creates array access expressions
if (!safe && receiverType.isArray() && binExpWriter[getOperandType(receiverType.getComponentType())].arraySet(true)) {
super.assignToArray(enclosing, receiver, subscript, rhsValueLoader, false);
} else { // handle safe subscript and other cases by calling the "putAt" method
if (rhsValueLoader instanceof VariableSlotLoader && enclosing instanceof BinaryExpression) { // GROOVY-6061
rhsValueLoader.putNodeMetaData(INFERRED_TYPE, controller.getTypeChooser().resolveType(enclosing, controller.getClassNode()));
}
Expand All @@ -346,8 +345,8 @@ protected void assignToArray(final Expression enclosing, final Expression receiv
operandStack.pop();
operandStack.remove(operandStack.getStackLength() - height);

// return value of assignment
rhsValueLoader.visit(controller.getAcg());
if (!Boolean.TRUE.equals(enclosing.getNodeMetaData("GROOVY-11288")))
rhsValueLoader.visit(controller.getAcg()); // re-load result value
}
}
}
1 change: 1 addition & 0 deletions src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,7 @@ && isAssignment(enclosingBinaryExpression.getOperation().getType())) {
boolean isEmptyDeclaration = (expression instanceof DeclarationExpression
&& (rightExpression instanceof EmptyExpression || rType == UNKNOWN_PARAMETER_TYPE));
if (isEmptyDeclaration) {
expression.putNodeMetaData(INFERRED_TYPE, lType);
// GROOVY-11353: "def var = null" cannot be a primitive type
if (isDynamicTyped(lType) && rType == UNKNOWN_PARAMETER_TYPE)
lType.putNodeMetaData("non-primitive type", Boolean.TRUE);
Expand Down
Loading

0 comments on commit cdb69c9

Please sign in to comment.