You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by su...@apache.org on 2022/10/10 08:15:21 UTC
[groovy] branch master updated: GROOVY-5744, GROOVY-10666: multi-assign via `iterator()` or `getAt(int)`
This is an automated email from the ASF dual-hosted git repository.
sunlan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new 3f9f88c187 GROOVY-5744, GROOVY-10666: multi-assign via `iterator()` or `getAt(int)`
3f9f88c187 is described below
commit 3f9f88c187b06b54eb5db33886dec56fdc192c66
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Sun Jul 31 21:07:21 2022 -0500
GROOVY-5744, GROOVY-10666: multi-assign via `iterator()` or `getAt(int)`
---
.../classgen/asm/BinaryExpressionHelper.java | 172 ++++++++++++++-------
.../groovy/classgen/asm/BytecodeVariable.java | 65 ++++----
.../codehaus/groovy/classgen/asm/CompileStack.java | 17 +-
.../MultipleAssignmentDeclarationTest.groovy | 55 ++++++-
4 files changed, 210 insertions(+), 99 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java
index e5810243ea..c6bde894b4 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java
@@ -33,7 +33,6 @@ import org.codehaus.groovy.ast.expr.Expression;
import org.codehaus.groovy.ast.expr.FieldExpression;
import org.codehaus.groovy.ast.expr.ListExpression;
import org.codehaus.groovy.ast.expr.MethodCallExpression;
-import org.codehaus.groovy.ast.expr.NotExpression;
import org.codehaus.groovy.ast.expr.PostfixExpression;
import org.codehaus.groovy.ast.expr.PrefixExpression;
import org.codehaus.groovy.ast.expr.PropertyExpression;
@@ -53,8 +52,13 @@ import org.objectweb.asm.MethodVisitor;
import static org.apache.groovy.ast.tools.ExpressionUtils.isNullConstant;
import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
import static org.codehaus.groovy.ast.tools.GeneralUtils.binX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.boolX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.constX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.elvisX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.notX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.nullX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.ternaryX;
import static org.codehaus.groovy.syntax.Types.ASSIGN;
import static org.codehaus.groovy.syntax.Types.BITWISE_AND;
import static org.codehaus.groovy.syntax.Types.BITWISE_AND_EQUAL;
@@ -105,10 +109,14 @@ import static org.codehaus.groovy.syntax.Types.RIGHT_SHIFT_EQUAL;
import static org.codehaus.groovy.syntax.Types.RIGHT_SHIFT_UNSIGNED;
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.DUP;
import static org.objectweb.asm.Opcodes.GOTO;
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.POP;
public class BinaryExpressionHelper {
// compare
@@ -133,7 +141,7 @@ public class BinaryExpressionHelper {
public BinaryExpressionHelper(final WriterController wc) {
this.controller = wc;
- this.unaryExpressionHelper = new UnaryExpressionHelper(this.controller);
+ this.unaryExpressionHelper = new UnaryExpressionHelper(wc);
}
public WriterController getController() {
@@ -347,12 +355,11 @@ public class BinaryExpressionHelper {
}
protected void assignToArray(final Expression parent, final Expression receiver, final Expression index, final Expression rhsValueLoader, final boolean safe) {
- // let's replace this assignment to a subscript operator with a
- // method call
+ // let's replace this assignment to a subscript operator with a method call
// e.g. x[5] = 10
// -> (x, [], 5), =, 10
// -> methodCall(x, "putAt", [5, 10])
- ArgumentListExpression ae = new ArgumentListExpression(index,rhsValueLoader);
+ ArgumentListExpression ae = new ArgumentListExpression(index, rhsValueLoader);
controller.getInvocationWriter().makeCall(parent, receiver, constX("putAt"), ae, InvocationWriter.invokeMethod, safe, false, false);
controller.getOperandStack().pop();
// return value of assignment
@@ -360,22 +367,25 @@ public class BinaryExpressionHelper {
}
public void evaluateElvisEqual(final BinaryExpression expression) {
- Token operation = expression.getOperation();
- BinaryExpression elvisAssignmentExpression = binX(
- expression.getLeftExpression(),
- Token.newSymbol(ASSIGN, operation.getStartLine(), operation.getStartColumn()),
- new ElvisOperatorExpression(expression.getLeftExpression(), expression.getRightExpression())
+ Expression lhs = expression.getLeftExpression();
+ Expression rhs = elvisX(lhs, expression.getRightExpression());
+ BinaryExpression assignment = binX(
+ lhs,
+ Token.newSymbol(ASSIGN, expression.getOperation().getStartLine(), expression.getOperation().getStartColumn()),
+ rhs
);
- this.evaluateEqual(elvisAssignmentExpression, false);
+ evaluateEqual(assignment, false);
}
public void evaluateEqual(final BinaryExpression expression, final boolean defineVariable) {
AsmClassGenerator acg = controller.getAcg();
+ MethodVisitor mv = controller.getMethodVisitor();
CompileStack compileStack = controller.getCompileStack();
OperandStack operandStack = controller.getOperandStack();
Expression leftExpression = expression.getLeftExpression();
Expression rightExpression = expression.getRightExpression();
- boolean directAssignment = defineVariable && !(leftExpression instanceof TupleExpression);
+ boolean singleAssignment = !(leftExpression instanceof TupleExpression);
+ boolean directAssignment = defineVariable && singleAssignment; //def x=y
if (directAssignment && rightExpression instanceof EmptyExpression) {
VariableExpression ve = (VariableExpression) leftExpression;
@@ -387,9 +397,8 @@ public class BinaryExpressionHelper {
// 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 (rightExpression instanceof ListExpression && lhsType.isArray()) {
- ListExpression list = (ListExpression) rightExpression;
- ArrayExpression array = new ArrayExpression(lhsType.getComponentType(), list.getExpressions());
- array.setSourcePosition(list);
+ Expression array = new ArrayExpression(lhsType.getComponentType(), ((ListExpression) rightExpression).getExpressions());
+ array.setSourcePosition(rightExpression);
array.visit(acg);
} else if (rightExpression instanceof EmptyExpression) {
loadInitValue(leftExpression.getType()); // TODO: lhsType?
@@ -430,7 +439,7 @@ public class BinaryExpressionHelper {
rhsValueId = compileStack.defineTemporaryVariable("$rhs", rhsType, true);
}
// TODO: if RHS is VariableSlotLoader already, then skip creating a new one
- BytecodeExpression rhsValueLoader = new VariableSlotLoader(rhsType,rhsValueId,operandStack);
+ Expression rhsValueLoader = new VariableSlotLoader(rhsType, rhsValueId, operandStack);
// assignment for subscript
if (leftExpression instanceof BinaryExpression) {
@@ -444,12 +453,49 @@ public class BinaryExpressionHelper {
compileStack.pushLHS(true);
- if (leftExpression instanceof TupleExpression) {
- // multiple declaration
- TupleExpression tuple = (TupleExpression) leftExpression;
- int i = 0;
- for (Expression e : tuple.getExpressions()) {
- callX(rhsValueLoader, "getAt", args(constX(i++))).visit(acg);
+ if (directAssignment) {
+ rhsValueLoader.visit(acg);
+ operandStack.remove(1);
+ compileStack.popLHS();
+ return;
+ }
+
+ if (singleAssignment) {
+ int mark = operandStack.getStackLength();
+ rhsValueLoader.visit(acg);
+ leftExpression.visit(acg);
+ operandStack.remove(operandStack.getStackLength() - mark);
+ } else { // multiple declaration or assignment
+ MethodCallExpression iterator = callX(rhsValueLoader, "iterator");
+ iterator.setImplicitThis(false);
+ iterator.visit(acg);
+
+ int iteratorId = compileStack.defineTemporaryVariable("$iter", true);
+ Expression seq = new VariableSlotLoader(iteratorId, operandStack);
+
+ MethodCallExpression hasNext = callX(seq, "hasNext");
+ hasNext.setImplicitThis(false);
+ boolX(hasNext).visit(acg);
+
+ Label done = new Label(), useGetAt = new Label();
+ Label useGetAt_noPop = operandStack.jump(IFEQ);
+
+ MethodCallExpression next = callX(seq, "next");
+ next.setImplicitThis(false);
+ next.visit(acg);
+
+ // check if first element is RHS; indicative of DGM#iterator(Object)
+ mv.visitInsn(DUP);
+ mv.visitVarInsn(ALOAD, rhsValueId);
+ mv.visitJumpInsn(IF_ACMPEQ, useGetAt);
+
+ boolean first = true;
+ for (Expression e : (TupleExpression) leftExpression) {
+ if (first) {
+ first = false;
+ } else {
+ ternaryX(hasNext, next, nullX()).visit(acg);
+ }
if (defineVariable) {
Variable v = (Variable) e;
operandStack.doGroovyCast(v);
@@ -459,19 +505,39 @@ public class BinaryExpressionHelper {
e.visit(acg);
}
}
- } else if (defineVariable) {
- // single declaration
- rhsValueLoader.visit(acg);
- operandStack.remove(1);
- compileStack.popLHS();
- return;
- } else {
- // normal assignment
- int mark = operandStack.getStackLength();
- rhsValueLoader.visit(acg);
- leftExpression.visit(acg);
- operandStack.remove(operandStack.getStackLength() - mark);
+
+ mv.visitJumpInsn(GOTO, done);
+
+ mv.visitLabel(useGetAt);
+
+ mv.visitInsn(POP); // discard result of "rhs.iterator().next()"
+
+ mv.visitLabel(useGetAt_noPop);
+
+ int i = 0;
+ for (Expression e : (TupleExpression) leftExpression) {
+ MethodCallExpression getAt = callX(rhsValueLoader, "getAt", constX(i++, true));
+ getAt.setImplicitThis(false);
+ getAt.visit(acg);
+
+ if (defineVariable) {
+ Variable v = (Variable) e;
+ operandStack.doGroovyCast(v);
+ BytecodeVariable bcv = compileStack.getVariable(v.getName());
+ if (bcv.isHolder()) {
+ operandStack.box();
+ operandStack.remove(1);
+ compileStack.createReference(bcv);
+ continue; // Reference stored in v
+ }
+ }
+ e.visit(acg);
+ }
+
+ mv.visitLabel(done);
+ compileStack.removeVar(iteratorId);
}
+
compileStack.popLHS();
// return value of assignment
@@ -490,11 +556,10 @@ public class BinaryExpressionHelper {
}
protected void evaluateCompareExpression(final MethodCaller compareMethod, final BinaryExpression expression) {
- ClassNode classNode = controller.getClassNode();
Expression leftExp = expression.getLeftExpression();
Expression rightExp = expression.getRightExpression();
- ClassNode leftType = controller.getTypeChooser().resolveType(leftExp, classNode);
- ClassNode rightType = controller.getTypeChooser().resolveType(rightExp, classNode);
+ ClassNode leftType = controller.getTypeChooser().resolveType(leftExp, controller.getClassNode());
+ ClassNode rightType = controller.getTypeChooser().resolveType(rightExp, controller.getClassNode());
boolean done = false;
if (ClassHelper.isPrimitiveType(leftType) && ClassHelper.isPrimitiveType(rightType)) {
@@ -613,12 +678,13 @@ public class BinaryExpressionHelper {
MethodCallExpression putAt = callX(receiver, "putAt", args(subscript, ret));
AsmClassGenerator acg = controller.getAcg();
+ CompileStack compileStack = controller.getCompileStack();
+ OperandStack operandStack = controller.getOperandStack();
+
putAt.visit(acg);
- OperandStack os = controller.getOperandStack();
- os.pop();
- os.load(ret.getType(), ret.getIndex());
+ operandStack.pop();
+ operandStack.load(ret.getType(), ret.getIndex());
- CompileStack compileStack = controller.getCompileStack();
compileStack.removeVar(ret.getIndex());
compileStack.removeVar(subscript.getIndex());
compileStack.removeVar(receiver.getIndex());
@@ -657,8 +723,8 @@ public class BinaryExpressionHelper {
private void evaluateNotInstanceof(final BinaryExpression expression) {
unaryExpressionHelper.writeNotExpression(
- new NotExpression(
- new BinaryExpression(
+ notX(
+ binX(
expression.getLeftExpression(),
GeneralUtils.INSTANCEOF,
expression.getRightExpression()
@@ -821,7 +887,15 @@ public class BinaryExpressionHelper {
// would be a.getAt(1).next() for the rhs, "lhs" code is a.putAt(1, rhs)
}
- private void evaluateElvisOperatorExpression(final ElvisOperatorExpression expression) {
+ public void evaluateTernary(final TernaryExpression expression) {
+ if (expression instanceof ElvisOperatorExpression) {
+ evaluateElvisExpression(expression);
+ } else {
+ evaluateTernaryExpression(expression);
+ }
+ }
+
+ private void evaluateElvisExpression(final TernaryExpression expression) {
MethodVisitor mv = controller.getMethodVisitor();
CompileStack compileStack = controller.getCompileStack();
OperandStack operandStack = controller.getOperandStack();
@@ -876,7 +950,7 @@ public class BinaryExpressionHelper {
operandStack.replace(common, 2);
}
- private void evaluateNormalTernary(final TernaryExpression expression) {
+ private void evaluateTernaryExpression(final TernaryExpression expression) {
MethodVisitor mv = controller.getMethodVisitor();
TypeChooser typeChooser = controller.getTypeChooser();
OperandStack operandStack = controller.getOperandStack();
@@ -916,12 +990,4 @@ public class BinaryExpressionHelper {
mv.visitLabel(l1);
operandStack.replace(common, 2);
}
-
- public void evaluateTernary(final TernaryExpression expression) {
- if (expression instanceof ElvisOperatorExpression) {
- evaluateElvisOperatorExpression((ElvisOperatorExpression) expression);
- } else {
- evaluateNormalTernary(expression);
- }
- }
}
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/BytecodeVariable.java b/src/main/java/org/codehaus/groovy/classgen/asm/BytecodeVariable.java
index 0ec751a374..4c4c5dda5f 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/BytecodeVariable.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/BytecodeVariable.java
@@ -34,28 +34,34 @@ public class BytecodeVariable {
private ClassNode type;
private String name;
private final int prevCurrent;
+ private boolean dynamicTyped;
private boolean holder;
// br for setting on the LocalVariableTable in the class file
// these fields should probably go to jvm Operand class
- private Label startLabel = null;
- private Label endLabel = null;
- private boolean dynamicTyped;
+ private Label startLabel;
+ private Label endLabel;
- private BytecodeVariable(){
+ private BytecodeVariable() {
+ index = 0;
+ prevCurrent = 0;
dynamicTyped = true;
- index=0;
- holder=false;
- prevCurrent=0;
}
- public BytecodeVariable(int index, ClassNode type, String name, int prevCurrent) {
+ public BytecodeVariable(final int index, final ClassNode type, final String name, final int prevCurrent) {
this.index = index;
this.type = type;
this.name = name;
this.prevCurrent = prevCurrent;
}
+ /**
+ * @return the stack index for this variable
+ */
+ public int getIndex() {
+ return index;
+ }
+
public String getName() {
return name;
}
@@ -64,11 +70,21 @@ public class BytecodeVariable {
return type;
}
- /**
- * @return the stack index for this variable
- */
- public int getIndex() {
- return index;
+ public void setType(final ClassNode type) {
+ this.type = type;
+ dynamicTyped = dynamicTyped || ClassHelper.isDynamicTyped(type);
+ }
+
+ public int getPrevIndex() {
+ return prevCurrent;
+ }
+
+ public boolean isDynamicTyped() {
+ return dynamicTyped;
+ }
+
+ public void setDynamicTyped(final boolean b) {
+ dynamicTyped = b;
}
/**
@@ -78,7 +94,7 @@ public class BytecodeVariable {
return holder;
}
- public void setHolder(boolean holder) {
+ public void setHolder(final boolean holder) {
this.holder = holder;
}
@@ -86,7 +102,7 @@ public class BytecodeVariable {
return startLabel;
}
- public void setStartLabel(Label startLabel) {
+ public void setStartLabel(final Label startLabel) {
this.startLabel = startLabel;
}
@@ -94,7 +110,7 @@ public class BytecodeVariable {
return endLabel;
}
- public void setEndLabel(Label endLabel) {
+ public void setEndLabel(final Label endLabel) {
this.endLabel = endLabel;
}
@@ -102,21 +118,4 @@ public class BytecodeVariable {
public String toString() {
return name + "(index=" + index + ",type=" + type + ",holder="+holder+")";
}
-
- public void setType(ClassNode type) {
- this.type = type;
- dynamicTyped |= ClassHelper.isDynamicTyped(type);
- }
-
- public void setDynamicTyped(boolean b) {
- dynamicTyped = b;
- }
-
- public boolean isDynamicTyped() {
- return dynamicTyped;
- }
-
- public int getPrevIndex() {
- return prevCurrent;
- }
}
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java
index 64a8ce05a3..72209f985c 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java
@@ -235,9 +235,12 @@ public class CompileStack {
inSpecialConstructorCall = element.inSpecialConstructorCall;
}
- public void removeVar(final int tempIndex) {
+ /**
+ * Indicates that the specified temporary variable is no longer used.
+ */
+ public void removeVar(final int variableIndex) {
BytecodeVariable head = temporaryVariables.removeFirst();
- if (head.getIndex() != tempIndex) {
+ if (head.getIndex() != variableIndex) {
temporaryVariables.addFirst(head);
MethodNode methodNode = controller.getMethodNode();
if (methodNode == null) {
@@ -246,7 +249,7 @@ public class CompileStack {
throw new GroovyBugError(
"In method "+ (methodNode!=null?methodNode.getText():"<unknown>") + ", " +
"CompileStack#removeVar: tried to remove a temporary " +
- "variable with index "+ tempIndex + " in wrong order. " +
+ "variable with index " + variableIndex + " in wrong order. " +
"Current temporary variables=" + temporaryVariables);
}
}
@@ -266,9 +269,9 @@ public class CompileStack {
}
/**
- * creates a temporary variable.
+ * Creates a temporary variable.
*
- * @param var defines type and name
+ * @param var specifies name and type
* @param store defines if the toplevel argument of the stack should be stored
* @return the index used for this temporary variable
*/
@@ -276,7 +279,7 @@ public class CompileStack {
return defineTemporaryVariable(var.getName(), var.getType(),store);
}
- public BytecodeVariable getVariable(final String variableName ) {
+ public BytecodeVariable getVariable(final String variableName) {
return getVariable(variableName, true);
}
@@ -627,7 +630,7 @@ public class CompileStack {
nextVariableIndex = localVariableOffset;
}
- private void createReference(final BytecodeVariable reference) {
+ void createReference(final BytecodeVariable reference) {
MethodVisitor mv = controller.getMethodVisitor();
mv.visitTypeInsn(NEW, "groovy/lang/Reference");
mv.visitInsn(DUP_X1);
diff --git a/src/test/gls/statements/MultipleAssignmentDeclarationTest.groovy b/src/test/gls/statements/MultipleAssignmentDeclarationTest.groovy
index 8ae6744b3e..b2ddd6aeeb 100644
--- a/src/test/gls/statements/MultipleAssignmentDeclarationTest.groovy
+++ b/src/test/gls/statements/MultipleAssignmentDeclarationTest.groovy
@@ -18,7 +18,6 @@
*/
package gls.statements
-import groovy.test.NotYetImplemented
import org.junit.Test
import static groovy.test.GroovyAssert.assertScript
@@ -79,10 +78,10 @@ final class MultipleAssignmentDeclarationTest {
}
@Test
- void testNestedScope() {
+ void testNestedScope1() {
assertScript '''import static groovy.test.GroovyAssert.shouldFail
- def c = {
+ def c = { ->
def (i,j) = [1,2]
assert i == 1
assert j == 2
@@ -107,6 +106,33 @@ final class MultipleAssignmentDeclarationTest {
'''
}
+ @Test
+ void testNestedScope2() {
+ assertScript '''
+ class C {
+ int m() {
+ def (i,j) = [1,2]
+ assert i == 1
+ assert j == 2
+
+ def x = { ->
+ assert i == 1
+ assert j == 2
+
+ i = 3
+ assert i == 3
+ }
+ x()
+
+ assert i == 3
+ return j
+ }
+ }
+ int n = new C().m()
+ assert n == 2
+ '''
+ }
+
@Test
void testMultiAssignChain() {
assertScript '''
@@ -117,7 +143,24 @@ final class MultipleAssignmentDeclarationTest {
'''
}
- @NotYetImplemented @Test // GROOVY-5744
+ @Test
+ void testMultiAssignFromObject() {
+ shouldFail MissingMethodException, '''
+ def obj = new Object()
+ def (x) = obj
+ '''
+ }
+
+ @Test
+ void testMultiAssignFromCalendar() {
+ assertScript '''
+ def (_, y, m) = Calendar.instance
+ assert y >= 2022
+ assert m in 0..11
+ '''
+ }
+
+ @Test // GROOVY-5744
void testMultiAssignFromIterator() {
assertScript '''
def list = [1,2,3]
@@ -129,7 +172,7 @@ final class MultipleAssignmentDeclarationTest {
'''
}
- @NotYetImplemented @Test // GROOVY-10666
+ @Test // GROOVY-10666
void testMultiAssignFromIterable() {
assertScript '''
class MySet {
@@ -172,7 +215,7 @@ final class MultipleAssignmentDeclarationTest {
'''
}
- @NotYetImplemented @Test // GROOVY-10666
+ @Test // GROOVY-10666
void testMultiAssignFromJavaStream() {
assertScript '''import java.util.stream.Stream