This is an automated email from the ASF dual-hosted git repository.
emilles 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 7b65f5d8a3 GROOVY-11563: STC: check compound assignment `x op= y` like
`x = x op y`
7b65f5d8a3 is described below
commit 7b65f5d8a31fdeb901cde8034195fc365a1bf243
Author: Eric Milles <[email protected]>
AuthorDate: Sat Feb 15 10:03:42 2025 -0600
GROOVY-11563: STC: check compound assignment `x op= y` like `x = x op y`
---
src/main/groovy/groovy/grape/GrapeIvy.groovy | 7 +-
.../groovy/ast/expr/ExpressionTransformer.java | 8 +-
.../groovy/ast/expr/VariableExpression.java | 228 ++++++++++++---------
.../transform/stc/StaticTypeCheckingVisitor.java | 39 ++--
.../groovy/transform/stc/STCAssignmentTest.groovy | 13 +-
.../classgen/asm/sc/BugsStaticCompileTest.groovy | 14 +-
.../classgen/asm/sc/StaticCompileMathTest.groovy | 18 +-
7 files changed, 190 insertions(+), 137 deletions(-)
diff --git a/src/main/groovy/groovy/grape/GrapeIvy.groovy
b/src/main/groovy/groovy/grape/GrapeIvy.groovy
index 75486334c9..b2d6a61217 100644
--- a/src/main/groovy/groovy/grape/GrapeIvy.groovy
+++ b/src/main/groovy/groovy/grape/GrapeIvy.groovy
@@ -589,7 +589,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
}
@@ -655,8 +655,9 @@ class GrapeIvy implements GrapeEngine {
List<URI> results = []
for (ArtifactDownloadReport adl : report.getAllArtifactsReports()) {
// TODO: check artifact type, jar vs library, etc.
- if (adl.getLocalFile()) {
- results += adl.getLocalFile().toURI()
+ def adlLocalFile = adl.getLocalFile()
+ if (adlLocalFile) {
+ results.add(adlLocalFile.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/ast/expr/VariableExpression.java
b/src/main/java/org/codehaus/groovy/ast/expr/VariableExpression.java
index 96c3841e8b..829b8b0cf2 100644
--- a/src/main/java/org/codehaus/groovy/ast/expr/VariableExpression.java
+++ b/src/main/java/org/codehaus/groovy/ast/expr/VariableExpression.java
@@ -24,31 +24,23 @@ import org.codehaus.groovy.ast.GroovyCodeVisitor;
import org.codehaus.groovy.ast.Variable;
/**
- * Represents a local variable name, the simplest form of expression.
e.g. "foo".
+ * Represents a local variable, the simplest form of expression. e.g. "foo".
*/
-public class VariableExpression extends Expression implements Variable {
+public class VariableExpression extends Expression implements Cloneable,
Variable {
+
// The following fields are only used internally; every occurrence of a
user-defined expression of the same kind
// has its own instance so as to preserve line information. Consequently,
to test for such an expression, don't
// compare against the field but call isXXXExpression() instead.
- public static final VariableExpression THIS_EXPRESSION = new
VariableExpression("this", ClassHelper.dynamicType());
- public static final VariableExpression SUPER_EXPRESSION = new
VariableExpression("super", ClassHelper.dynamicType());
+ public static final VariableExpression THIS_EXPRESSION = new
VariableExpression("this");
+ public static final VariableExpression SUPER_EXPRESSION = new
VariableExpression("super");
- private final String variable;
private int modifiers;
- private boolean inStaticContext;
- private boolean isDynamicTyped = false;
+ private final String variable;
private Variable accessedVariable;
- boolean closureShare = false;
- boolean useRef = false;
private final ClassNode originType;
+ private boolean inStaticContext, isDynamicTyped, closureShare, useRef;
- public Variable getAccessedVariable() {
- return accessedVariable;
- }
-
- public void setAccessedVariable(Variable origin) {
- this.accessedVariable = origin;
- }
+ //
public VariableExpression(final String name, final ClassNode type) {
variable = name;
@@ -56,39 +48,87 @@ public class VariableExpression extends Expression
implements Variable {
setType(ClassHelper.isPrimitiveType(type) ?
ClassHelper.getWrapper(type) : type);
}
- public VariableExpression(String variable) {
- this(variable, ClassHelper.dynamicType());
+ public VariableExpression(final String name) {
+ this(name, ClassHelper.dynamicType());
}
- public VariableExpression(Variable variable) {
- this(variable.getName(), variable.getOriginType());
- setAccessedVariable(variable);
- setModifiers(variable.getModifiers());
+ public VariableExpression(final Variable av) {
+ this(av.getName(), av.getOriginType());
+ setModifiers(av.getModifiers());
+ setAccessedVariable(av);
}
@Override
- public void visit(GroovyCodeVisitor visitor) {
- visitor.visitVariableExpression(this);
+ public VariableExpression clone() {
+ var copy = new VariableExpression(variable, originType);
+ copy.setAccessedVariable(accessedVariable);
+ copy.setModifiers(modifiers);
+
+ copy.setClosureSharedVariable(closureShare);
+ copy.setInStaticContext(inStaticContext);
+ copy.setUseReferenceDirectly(useRef);
+
+ copy.setSynthetic(isSynthetic());
+ copy.setType(super.getType());
+ copy.setSourcePosition(this);
+ copy.copyNodeMetaData(this);
+
+ return copy;
}
- @Override
- public Expression transformExpression(ExpressionTransformer transformer) {
- return this;
+ //
+
+ public void setAccessedVariable(final Variable variable) {
+ accessedVariable = variable;
}
+ /**
+ * Use this method to tell if a variable is used in a closure, like in the
following example:
+ * <pre>def str = 'Hello'
+ * def cl = { println str }
+ * </pre>
+ * The "str" variable is closure shared. The variable expression inside
the closure references an
+ * accessed variable "str" which must have the closure shared flag set.
+ *
+ * @param inClosure indicates if this variable is referenced from a closure
+ */
@Override
- public String getText() {
- return variable;
+ public void setClosureSharedVariable(final boolean inClosure) {
+ closureShare = inClosure;
}
- @Override
- public String getName() {
- return variable;
+ public void setInStaticContext(final boolean inStaticContext) {
+ this.inStaticContext = inStaticContext;
}
+ public void setModifiers(final int modifiers) {
+ this.modifiers = modifiers;
+ }
+
+ /**
+ * Set the type of this variable. If you call this method from an AST
transformation and that
+ * the {@link #getAccessedVariable() accessed variable} is ({@link
#isClosureSharedVariable() shared},
+ * this operation is unsafe and may lead to a verify error at compile
time. Instead, set the type of
+ * the {@link #getAccessedVariable() accessed variable}
+ */
@Override
- public String toString() {
- return super.toString() + "[variable: " + variable +
(this.isDynamicTyped() ? "" : " type: " + getType()) + "]";
+ public void setType(final ClassNode type) {
+ super.setType(type);
+ isDynamicTyped |= ClassHelper.isDynamicTyped(type);
+ }
+
+ /**
+ * For internal use only. This flag is used by compiler internals and
should probably
+ * be converted to a node metadata in the future.
+ */
+ public void setUseReferenceDirectly(final boolean useRef) {
+ this.useRef = useRef;
+ }
+
+
//--------------------------------------------------------------------------
+
+ public Variable getAccessedVariable() {
+ return accessedVariable;
}
@Override
@@ -102,33 +142,40 @@ public class VariableExpression extends Expression
implements Variable {
}
@Override
- public boolean isInStaticContext() {
- if (accessedVariable != null && accessedVariable != this) return
accessedVariable.isInStaticContext();
- return inStaticContext;
+ public int getModifiers() {
+ return modifiers;
}
- public void setInStaticContext(boolean inStaticContext) {
- this.inStaticContext = inStaticContext;
+ @Override
+ public String getName() {
+ return variable;
+ }
+
+ @Override
+ public String getText() {
+ return variable;
}
- /**
- * Set the type of this variable. If you call this method from an AST
transformation and that
- * the {@link #getAccessedVariable() accessed variable} is ({@link
#isClosureSharedVariable() shared},
- * this operation is unsafe and may lead to a verify error at compile
time. Instead, set the type of
- * the {@link #getAccessedVariable() accessed variable}
- *
- * @param cn the type to be set on this variable
- */
@Override
- public void setType(ClassNode cn) {
- super.setType(cn);
- isDynamicTyped |= ClassHelper.isDynamicTyped(cn);
+ public ClassNode getType() {
+ if (accessedVariable != null && accessedVariable != this) {
+ return accessedVariable.getType();
+ }
+ return super.getType();
}
+ /**
+ * Returns the type which was used when this variable expression was
created. For example,
+ * {@link #getType()} may return a boxed type while this method would
return the primitive type.
+ *
+ * @return the type which was used to define this variable expression
+ */
@Override
- public boolean isDynamicTyped() {
- if (accessedVariable != null && accessedVariable != this) return
accessedVariable.isDynamicTyped();
- return isDynamicTyped;
+ public ClassNode getOriginType() {
+ if (accessedVariable != null && accessedVariable != this) {
+ return accessedVariable.getOriginType();
+ }
+ return originType;
}
/**
@@ -143,75 +190,56 @@ public class VariableExpression extends Expression
implements Variable {
*/
@Override
public boolean isClosureSharedVariable() {
- if (accessedVariable != null && accessedVariable != this) return
accessedVariable.isClosureSharedVariable();
+ if (accessedVariable != null && accessedVariable != this) {
+ return accessedVariable.isClosureSharedVariable();
+ }
return closureShare;
}
- /**
- * Use this method to tell if a variable is used in a closure, like in the
following example:
- * <pre>def str = 'Hello'
- * def cl = { println str }
- * </pre>
- * The "str" variable is closure shared. The variable expression inside
the closure references an
- * accessed variable "str" which must have the closure shared flag set.
- *
- * @param inClosure tells if this variable is later referenced in a closure
- */
@Override
- public void setClosureSharedVariable(boolean inClosure) {
- closureShare = inClosure;
+ public boolean isDynamicTyped() {
+ if (accessedVariable != null && accessedVariable != this) {
+ return accessedVariable.isDynamicTyped();
+ }
+ return isDynamicTyped;
}
@Override
- public int getModifiers() {
- return modifiers;
+ public boolean isInStaticContext() {
+ if (accessedVariable != null && accessedVariable != this) {
+ return accessedVariable.isInStaticContext();
+ }
+ return inStaticContext;
}
- /**
- * For internal use only. This flag is used by compiler internals and
should probably
- * be converted to a node metadata in the future.
- *
- * @param useRef
- */
- public void setUseReferenceDirectly(boolean useRef) {
- this.useRef = useRef;
+ public boolean isSuperExpression() {
+ return "super".equals(variable);
+ }
+
+ public boolean isThisExpression() {
+ return "this".equals(variable);
}
/**
- * For internal use only. This flag is used by compiler internals and
should probably
- * be converted to a node metadata in the future.
+ * For internal use only. This flag is used by compiler internals and
should
+ * probably be converted to a node metadata in the future.
*/
public boolean isUseReferenceDirectly() {
return useRef;
}
@Override
- public ClassNode getType() {
- if (accessedVariable != null && accessedVariable != this) return
accessedVariable.getType();
- return super.getType();
+ public String toString() {
+ return super.toString() + "[variable: " + getName() +
(isDynamicTyped() ? "" : " type: " + getType()) + "]";
}
- /**
- * Returns the type which was used when this variable expression was
created. For example,
- * {@link #getType()} may return a boxed type while this method would
return the primitive type.
- *
- * @return the type which was used to define this variable expression
- */
@Override
- public ClassNode getOriginType() {
- if (accessedVariable != null && accessedVariable != this) return
accessedVariable.getOriginType();
- return originType;
- }
-
- public boolean isThisExpression() {
- return "this".equals(variable);
- }
-
- public boolean isSuperExpression() {
- return "super".equals(variable);
+ public Expression transformExpression(final ExpressionTransformer
transformer) {
+ return this;
}
- public void setModifiers(int modifiers) {
- this.modifiers = modifiers;
+ @Override
+ public void visit(final GroovyCodeVisitor visitor) {
+ visitor.visitVariableExpression(this);
}
}
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..4bfc06d5ec 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;
@@ -816,20 +817,36 @@ public class StaticTypeCheckingVisitor extends
ClassCodeVisitorSupport {
Expression leftExpression = expression.getLeftExpression();
Expression rightExpression = expression.getRightExpression();
+ if (isAssignment(op) && op != EQUAL) {
+ ExpressionTransformer cloneMaker = new ExpressionTransformer()
{
+ @Override
+ public Expression transform(final Expression expr) {
+ if (expr instanceof VariableExpression) {
+ return ((VariableExpression) expr).clone();
+ }
+ return expr.transformExpression(this);
+ }
+ };
+ // GROOVY-10628, GROOVY-11563: for "x op= y", find type as if
it was "x = x op y"
+ rightExpression = (op == ELVIS_EQUAL ?
elvisX(cloneMaker.transform(leftExpression), rightExpression) :
+ binX(cloneMaker.transform(leftExpression),
Token.newSymbol(TokenUtil.removeAssignment(op),
expression.getOperation().getStartLine(),
expression.getOperation().getStartColumn()), rightExpression));
+ rightExpression.setSourcePosition(expression);
+
+ visitBinaryExpression(assignX(leftExpression, rightExpression,
expression));
+ expression.copyNodeMetaData(rightExpression);
+ return;
+ }
+
leftExpression.visit(this);
- SetterInfo setterInfo = removeSetterInfo(leftExpression);
- ClassNode lType = null;
+ ClassNode lType = getType(leftExpression);
+ var setterInfo = removeSetterInfo(leftExpression);
if (setterInfo != null) {
if (ensureValidSetter(expression, leftExpression,
rightExpression, setterInfo)) {
return;
}
- lType = getType(leftExpression);
} else {
- if (op != EQUAL && op != ELVIS_EQUAL) {
- lType = getType(leftExpression);
- } else {
+ if (op == EQUAL) {
lType = getOriginalDeclarationType(leftExpression);
-
applyTargetType(lType, rightExpression);
}
rightExpression.visit(this);
@@ -848,14 +865,6 @@ public class StaticTypeCheckingVisitor extends
ClassCodeVisitorSupport {
storeTargetMethod(expression,
reverseExpression.getNodeMetaData(DIRECT_METHOD_CALL_TARGET));
} else {
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);
- }
}
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() {
