You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by em...@apache.org on 2025/02/02 20:55:32 UTC
(groovy) branch GROOVY-11563 updated (18c00c1ed2 -> 7ba1012b05)
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a change to branch GROOVY-11563
in repository https://gitbox.apache.org/repos/asf/groovy.git
discard 18c00c1ed2 GROOVY-11563: STC: check compound assignment `x op= y` like `x = x op y`
new 7ba1012b05 GROOVY-11563: STC: check compound assignment `x op= y` like `x = x op y`
This update added new revisions after undoing existing revisions.
That is to say, some revisions that were in the old version of the
branch are not in the new version. This situation occurs
when a user --force pushes a change and generates a repository
containing something like this:
* -- * -- B -- O -- O -- O (18c00c1ed2)
\
N -- N -- N refs/heads/GROOVY-11563 (7ba1012b05)
You should already have received notification emails for all of the O
revisions, and so the following emails describe only the N revisions
from the common base, B.
Any revisions marked "omit" are not gone; other references still
refer to them. Any revisions marked "discard" are gone forever.
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../classgen/asm/sc/BugsStaticCompileTest.groovy | 14 ++++++--------
.../classgen/asm/sc/StaticCompileMathTest.groovy | 18 +++++++++++++++---
2 files changed, 21 insertions(+), 11 deletions(-)
(groovy) 01/01: GROOVY-11563: STC: check compound assignment `x op= y` like `x = x op y`
Posted by em...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY-11563
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 7ba1012b0539990f2bdb47d70ff830d12a958807
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Sun Feb 2 10:37:48 2025 -0600
GROOVY-11563: STC: check compound assignment `x op= y` like `x = x op y`
---
src/main/groovy/groovy/grape/GrapeIvy.groovy | 5 +-
.../groovy/ast/expr/ExpressionTransformer.java | 8 +---
.../classgen/asm/sc/StaticTypesCallSiteWriter.java | 2 +-
.../transform/stc/StaticTypeCheckingVisitor.java | 54 ++++++++++++++++++----
.../groovy/transform/stc/STCAssignmentTest.groovy | 13 +++++-
.../classgen/asm/sc/BugsStaticCompileTest.groovy | 14 +++---
.../classgen/asm/sc/StaticCompileMathTest.groovy | 18 ++++++--
7 files changed, 82 insertions(+), 32 deletions(-)
diff --git a/src/main/groovy/groovy/grape/GrapeIvy.groovy b/src/main/groovy/groovy/grape/GrapeIvy.groovy
index 75486334c9..750466de1d 100644
--- 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
index 814ebf3295..80d2b901fa 100644
--- 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
index cd641ceb40..a1c344030e 100644
--- 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 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter {
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
index f0cdc32950..c80a8356f3 100644
--- 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.DeclarationExpression;
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.MOD_EQUAL;
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 class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
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 class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
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 class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
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
index 52d6594f08..670cc96374 100644
--- 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'
'''
}
diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/BugsStaticCompileTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/BugsStaticCompileTest.groovy
index 65fb93c79d..f0b0d6b685 100644
--- a/src/test/org/codehaus/groovy/classgen/asm/sc/BugsStaticCompileTest.groovy
+++ b/src/test/org/codehaus/groovy/classgen/asm/sc/BugsStaticCompileTest.groovy
@@ -222,10 +222,9 @@ final class BugsStaticCompileTest extends BugsSTCTest implements StaticCompilati
@CompileStatic
class Tool {
@CompileStatic // annotated too, even if class is already annotated
- String relativePath(File relbase, File file) {
- def pathParts = []
- def currentFile = file
- while (currentFile != null && currentFile != relbase) {
+ String relativePath(File base, File file) {
+ List<String> pathParts = []; File currentFile = file
+ while (currentFile != null && currentFile != base) {
pathParts += currentFile.name
currentFile = currentFile.parentFile
}
@@ -243,10 +242,9 @@ final class BugsStaticCompileTest extends BugsSTCTest implements StaticCompilati
@CompileStatic
class Tool {
@CompileStatic // annotated too, even if class is already annotated
- String relativePath(File relbase, File file) {
- def pathParts = []
- def currentFile = file
- while (currentFile != null && currentFile != relbase) {
+ String relativePath(File base, File file) {
+ List<String> pathParts = []; File currentFile = file
+ while (currentFile != null && currentFile != base) {
pathParts += currentFile.name
currentFile = currentFile.parentFile
}
diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileMathTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileMathTest.groovy
index b03f839d72..95ec62776e 100644
--- a/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileMathTest.groovy
+++ b/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileMathTest.groovy
@@ -20,10 +20,13 @@ package org.codehaus.groovy.classgen.asm.sc
import org.codehaus.groovy.classgen.asm.AbstractBytecodeTestCase
+import static groovy.test.GroovyAssert.shouldFail
+
/**
* Unit tests for static compilation: basic math operations.
*/
-class StaticCompileMathTest extends AbstractBytecodeTestCase {
+final class StaticCompileMathTest extends AbstractBytecodeTestCase {
+
void testIntSum() {
assertScript '''
@groovy.transform.CompileStatic
@@ -243,13 +246,22 @@ class StaticCompileMathTest extends AbstractBytecodeTestCase {
void testStaticCompileDivideEquals() {
assertScript '''
@groovy.transform.CompileStatic
- int foo() {
- int i = 4
+ def foo() {
+ def i = 4
i /= 2
i
}
assert foo()==2
'''
+
+ def err = shouldFail '''
+ @groovy.transform.CompileStatic
+ int foo() {
+ int i = 4
+ i /= 2
+ }
+ '''
+ assert err =~ /Cannot assign value of type java.math.BigDecimal to variable of type int/
}
void testStaticCompileMultiplyEquals() {