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 c8606ba996d8d9b102fed5a6b0137965d8b3d6ef Author: Eric Milles <[email protected]> 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 +--- .../transform/stc/StaticTypeCheckingVisitor.java | 54 ++++++++++++++++++---- .../groovy/transform/stc/STCAssignmentTest.groovy | 13 +++++- .../classgen/asm/sc/BugsStaticCompileTest.groovy | 14 +++--- .../classgen/asm/sc/StaticCompileMathTest.groovy | 18 ++++++-- 6 files changed, 81 insertions(+), 31 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/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() {
