GROOVY-11563: STC: check compound assignment x op= y like x = x op y

apache · Feb 2, 2025 · 18c00c1 · 18c00c1
1 parent 6dff05e
commit 18c00c1
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 21 deletions.
diff --git a/src/main/groovy/groovy/grape/GrapeIvy.groovy b/src/main/groovy/groovy/grape/GrapeIvy.groovy
@@ -57,6 +57,7 @@ import org.codehaus.groovy.runtime.metaclass.MetaClassRegistryImpl
 import org.w3c.dom.Element
 
 import javax.xml.parsers.DocumentBuilderFactory
+
 import java.text.ParseException
 import java.util.jar.JarFile
 import java.util.regex.Pattern
@@ -589,7 +590,7 @@ class GrapeIvy implements GrapeEngine {
                 List<String> versions = []
                 moduleDir.eachFileMatch(ivyFilePattern) { File ivyFile ->
                     def m = ivyFilePattern.matcher(ivyFile.getName())
-                    if (m.matches()) versions += m.group(1)
+                    if (m.matches()) versions.add(m.group(1))
                 }
                 grapes[moduleDir.getName()] = versions
             }
@@ -656,7 +657,7 @@ class GrapeIvy implements GrapeEngine {
         for (ArtifactDownloadReport adl : report.getAllArtifactsReports()) {
             // TODO: check artifact type, jar vs library, etc.
             if (adl.getLocalFile()) {
-                results += adl.getLocalFile().toURI()
+                results.add(adl.getLocalFile().toURI())
             }
         }
 

diff --git a/src/main/java/org/codehaus/groovy/ast/expr/ExpressionTransformer.java b/src/main/java/org/codehaus/groovy/ast/expr/ExpressionTransformer.java
@@ -18,14 +18,10 @@
  */
 package org.codehaus.groovy.ast.expr;
 
-
 /**
- * Provides a way to transform expressions
+ * Provides a way to transform expressions.
  */
+@FunctionalInterface
 public interface ExpressionTransformer {
-
-    /** 
-     * Transforms the given expression into another expression
-     */
     Expression transform(Expression expression);
 }
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
@@ -702,7 +702,7 @@ private void writeStringPlusCall(final Expression receiver, final String message
         visitBoxedArgument(arguments);
         int m2 = operandStack.getStackLength();
         MethodVisitor mv = controller.getMethodVisitor();
-        mv.visitMethodInsn(INVOKESTATIC, "org/codehaus/groovy/runtime/DefaultGroovyMethods", "plus", "(Ljava/lang/String;Ljava/lang/Object;)Ljava/lang/String;", false);
+        mv.visitMethodInsn(INVOKESTATIC, "org/codehaus/groovy/runtime/StringGroovyMethods", "plus", "(Ljava/lang/CharSequence;Ljava/lang/Object;)Ljava/lang/String;", false);
         operandStack.replace(STRING_TYPE, m2 - m1);
     }
 

diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -65,6 +65,7 @@
 import org.codehaus.groovy.ast.expr.ElvisOperatorExpression;
 import org.codehaus.groovy.ast.expr.EmptyExpression;
 import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.ast.expr.ExpressionTransformer;
 import org.codehaus.groovy.ast.expr.FieldExpression;
 import org.codehaus.groovy.ast.expr.LambdaExpression;
 import org.codehaus.groovy.ast.expr.ListExpression;
@@ -271,6 +272,7 @@
 import static org.codehaus.groovy.syntax.Types.PLUS_PLUS;
 import static org.codehaus.groovy.syntax.Types.REMAINDER;
 import static org.codehaus.groovy.syntax.Types.REMAINDER_EQUAL;
+import static org.codehaus.groovy.transform.sc.StaticCompilationMetadataKeys.BINARY_EXP_TARGET;
 import static org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.ArrayList_TYPE;
 import static org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.Collection_TYPE;
 import static org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.LinkedHashMap_TYPE;
@@ -815,15 +817,49 @@ public void visitBinaryExpression(final BinaryExpression expression) {
             int op = expression.getOperation().getType();
             Expression leftExpression = expression.getLeftExpression();
             Expression rightExpression = expression.getRightExpression();
+            Expression wrongExpression = null; // GROOVY-10628, GROOVY-11563
+
+            if (isAssignment(op) && op != EQUAL) {
+                // for "x op= y", find type as if it was "x = x op y"
+                ExpressionTransformer cloneMaker = new ExpressionTransformer() {
+                    @Override
+                    public Expression transform(final Expression expr) {
+                        // TODO: is there a beter way to clone?
+                        if (expr instanceof VariableExpression) {
+                            var variable = (VariableExpression) expr;
+                            var copy = new VariableExpression(variable.getAccessedVariable());
+                            copy.setClosureSharedVariable(variable.isClosureSharedVariable());
+                            copy.setUseReferenceDirectly(variable.isUseReferenceDirectly());
+                            copy.setInStaticContext(variable.isInStaticContext());
+                            copy.setSynthetic(variable.isSynthetic());
+                            copy.setType(variable.getType());
+                            copy.setSourcePosition(variable);
+                            copy.copyNodeMetaData(variable);
+                            return copy;
+                        }
+                        return expr.transformExpression(this);
+                    }
+                };
+                if (op == ELVIS_EQUAL) {
+                    wrongExpression = elvisX(cloneMaker.transform(leftExpression), rightExpression);
+                } else {
+                    wrongExpression = binX(cloneMaker.transform(leftExpression), Token.newSymbol(TokenUtil.removeAssignment(op), expression.getOperation().getStartLine(), expression.getOperation().getStartColumn()), rightExpression);
+                }
+                wrongExpression.setSourcePosition(expression);
+                op = EQUAL;
+            }
 
             leftExpression.visit(this);
             SetterInfo setterInfo = removeSetterInfo(leftExpression);
             ClassNode lType = null;
+// TODO: can visit and getType be split out of if/else for simpler structure?
             if (setterInfo != null) {
                 if (ensureValidSetter(expression, leftExpression, rightExpression, setterInfo)) {
                     return;
                 }
                 lType = getType(leftExpression);
+// TODO: ensureValidSetter expands compound operator and visits rightExpression
+if (wrongExpression != null) wrongExpression.visit(this);
             } else {
                 if (op != EQUAL && op != ELVIS_EQUAL) {
                     lType = getType(leftExpression);
@@ -832,7 +868,9 @@ public void visitBinaryExpression(final BinaryExpression expression) {
 
                     applyTargetType(lType, rightExpression);
                 }
-                rightExpression.visit(this);
+// TODO: want to visit before left expression above...
+if (wrongExpression != null) wrongExpression.visit(this);
+else                         rightExpression.visit(this);
             }
 
             ClassNode rType = isNullConstant(rightExpression)
@@ -846,16 +884,12 @@ public void visitBinaryExpression(final BinaryExpression expression) {
                 resultType = getResultType(rType, op, lType, reverseExpression);
                 if (resultType == null) resultType = boolean_TYPE; // GROOVY-10239
                 storeTargetMethod(expression, reverseExpression.getNodeMetaData(DIRECT_METHOD_CALL_TARGET));
-            } else {
+            } else if (wrongExpression == null) {
                 resultType = getResultType(lType, op, rType, expression);
-                if (op == ELVIS_EQUAL) {
-                    // TODO: Should this transform and visit be done before left and right are visited above?
-                    Expression fullExpression = new ElvisOperatorExpression(leftExpression, rightExpression);
-                    fullExpression.setSourcePosition(expression);
-                    fullExpression.visit(this);
-
-                    resultType = getType(fullExpression);
-                }
+            } else {
+                resultType = getResultType(lType, op, getType(wrongExpression), expression);
+expression.putNodeMetaData(BINARY_EXP_TARGET, wrongExpression.getNodeMetaData(BINARY_EXP_TARGET));
+expression.putNodeMetaData(DIRECT_METHOD_CALL_TARGET, wrongExpression.getNodeMetaData(DIRECT_METHOD_CALL_TARGET));
             }
             if (resultType == null) {
                 resultType = lType;

diff --git a/src/test/groovy/transform/stc/STCAssignmentTest.groovy b/src/test/groovy/transform/stc/STCAssignmentTest.groovy
@@ -341,10 +341,19 @@ class STCAssignmentTest extends StaticTypeCheckingTestCase {
         'Cannot find matching method java.lang.Integer#minus(java.lang.Object)'
     }
 
+    // GROOVY-11563
+    void testNumberPlusEqualsString() {
+        shouldFailWithMessages '''
+        Number n = 0
+        n += "no"
+    ''',
+        'Cannot assign value of type java.lang.String to variable of type java.lang.Number'
+    }
+
     void testStringPlusEqualsString() {
         assertScript '''
-            String str = 'test'
-            str += 'test2'
+            String s = 'prefix'
+            s += 'suffix'
         '''
     }