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 2f2a0a71b3a7be146510729fcfdd589dee8da73f 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() {
