This is an automated email from the ASF dual-hosted git repository. sunlan pushed a commit to branch check-PR1293 in repository https://gitbox.apache.org/repos/asf/groovy.git
commit fb7de350135eaecc0b16c8eba1ccddfd8bf646e3 Author: Paul King <[email protected]> AuthorDate: Mon Jun 29 20:34:09 2020 +1000 GROOVY-7971: @CS flow typing incorrectly casting to map at runtime --- build.gradle | 3 +- .../transform/sc/StaticCompilationVisitor.java | 15 +++-- .../transform/stc/StaticTypeCheckingVisitor.java | 64 +++++++++++++++++----- .../groovy/transform/stc/MethodCallsSTCTest.groovy | 50 +++++++++++++++++ 4 files changed, 113 insertions(+), 19 deletions(-) diff --git a/build.gradle b/build.gradle index 33d4bf5..cea4dd3 100644 --- a/build.gradle +++ b/build.gradle @@ -208,10 +208,11 @@ dependencies { antlr2 "org.apache.ant:ant-antlr:$antVersion" testImplementation project(':groovy-ant') - testImplementation project(':groovy-xml') testImplementation project(':groovy-dateutil') + testImplementation project(':groovy-json') testImplementation project(':groovy-test') testImplementation project(':groovy-macro') + testImplementation project(':groovy-xml') } ext.generatedDirectory = "${buildDir}/generated/sources" diff --git a/src/main/java/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java b/src/main/java/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java index 79fe33a..e00d14b 100644 --- a/src/main/java/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java @@ -408,10 +408,17 @@ public class StaticCompilationVisitor extends StaticTypeCheckingVisitor { public void visitMethodCallExpression(final MethodCallExpression call) { super.visitMethodCallExpression(call); - MethodNode target = call.getNodeMetaData(DIRECT_METHOD_CALL_TARGET); - if (target != null) { - call.setMethodTarget(target); - memorizeInitialExpressions(target); + MethodNode newTarget = call.getNodeMetaData(DIRECT_METHOD_CALL_TARGET); + if (newTarget != null) { + MethodNode existingTarget = call.getMethodTarget(); + if (existingTarget != null) + if (!(newTarget.getDeclaringClass().equals(existingTarget.getDeclaringClass())) || !(newTarget.getTypeDescriptor().equals(existingTarget.getTypeDescriptor()))) { + // avoid cast class exception + newTarget.putNodeMetaData(DYNAMIC_RESOLUTION, Boolean.TRUE); + call.putNodeMetaData(DYNAMIC_RESOLUTION, OBJECT_TYPE); + } + call.setMethodTarget(newTarget); + memorizeInitialExpressions(newTarget); } if (call.getMethodTarget() == null && call.getLineNumber() > 0) { 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 38eb2f3..5e8cd04 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -52,6 +52,7 @@ import org.codehaus.groovy.ast.expr.ArgumentListExpression; import org.codehaus.groovy.ast.expr.AttributeExpression; import org.codehaus.groovy.ast.expr.BinaryExpression; import org.codehaus.groovy.ast.expr.BitwiseNegationExpression; +import org.codehaus.groovy.ast.expr.BooleanExpression; import org.codehaus.groovy.ast.expr.CastExpression; import org.codehaus.groovy.ast.expr.ClassExpression; import org.codehaus.groovy.ast.expr.ClosureExpression; @@ -184,6 +185,7 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.callX; import static org.codehaus.groovy.ast.tools.GeneralUtils.castX; import static org.codehaus.groovy.ast.tools.GeneralUtils.constX; import static org.codehaus.groovy.ast.tools.GeneralUtils.getSetterName; +import static org.codehaus.groovy.ast.tools.GeneralUtils.ifElseS; import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements; import static org.codehaus.groovy.ast.tools.GeneralUtils.localVarX; import static org.codehaus.groovy.ast.tools.GeneralUtils.thisPropX; @@ -218,6 +220,7 @@ import static org.codehaus.groovy.syntax.Types.INTDIV_EQUAL; import static org.codehaus.groovy.syntax.Types.KEYWORD_IN; import static org.codehaus.groovy.syntax.Types.KEYWORD_INSTANCEOF; import static org.codehaus.groovy.syntax.Types.LEFT_SQUARE_BRACKET; +import static org.codehaus.groovy.syntax.Types.LOGICAL_OR; import static org.codehaus.groovy.syntax.Types.MINUS_MINUS; import static org.codehaus.groovy.syntax.Types.MOD; import static org.codehaus.groovy.syntax.Types.MOD_EQUAL; @@ -3798,21 +3801,8 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { @Override public void visitIfElse(final IfStatement ifElse) { Map<VariableExpression, List<ClassNode>> oldTracker = pushAssignmentTracking(); - try { - // create a new temporary element in the if-then-else type info - typeCheckingContext.pushTemporaryTypeInfo(); - visitStatement(ifElse); - ifElse.getBooleanExpression().visit(this); - ifElse.getIfBlock().visit(this); - - // pop if-then-else temporary type info - typeCheckingContext.popTemporaryTypeInfo(); - - // GROOVY-6099: restore assignment info as before the if branch - restoreTypeBeforeConditional(); - - ifElse.getElseBlock().visit(this); + visitIfElseMaybeOrBranches(ifElse, true); } finally { popAssignmentTracking(oldTracker); } @@ -3827,6 +3817,52 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } } + private void visitIfElseMaybeOrBranches(IfStatement ifElse, boolean topLevel) { + BooleanExpression condition = ifElse.getBooleanExpression(); + BinaryExpression lor = null; + if (condition.getExpression() instanceof BinaryExpression) { + lor = (BinaryExpression) condition.getExpression(); + if (lor.getOperation().getType() != LOGICAL_OR) { + lor = null; + } + } + // for logical OR, any one branch may be true branch, so traverse separately + if (lor != null) { + IfStatement left = ifElseS(lor.getLeftExpression(), ifElse.getIfBlock(), ifElse.getElseBlock()); +// left.setSourcePosition(ifElse); + typeCheckingContext.pushTemporaryTypeInfo(); + visitIfElseMaybeOrBranches(left, false); + typeCheckingContext.popTemporaryTypeInfo(); + restoreTypeBeforeConditional(); + IfStatement right = ifElseS(lor.getRightExpression(), ifElse.getIfBlock(), ifElse.getElseBlock()); +// right.setSourcePosition(ifElse); + typeCheckingContext.pushTemporaryTypeInfo(); + visitIfElseMaybeOrBranches(right, false); + typeCheckingContext.popTemporaryTypeInfo(); + restoreTypeBeforeConditional(); + } + if (topLevel || lor == null) { + // do it all again to get correct union type for casting (hush warnings?) + visitIfElseBranches(ifElse); + } + } + + private void visitIfElseBranches(IfStatement ifElse) { + // create a new temporary element in the if-then-else type info + typeCheckingContext.pushTemporaryTypeInfo(); + visitStatement(ifElse); + ifElse.getBooleanExpression().visit(this); + ifElse.getIfBlock().visit(this); + + // pop if-then-else temporary type info + typeCheckingContext.popTemporaryTypeInfo(); + + // GROOVY-6099: restore assignment info as before the if branch + restoreTypeBeforeConditional(); + + ifElse.getElseBlock().visit(this); + } + protected void visitInstanceofNot(final BinaryExpression be) { BlockStatement currentBlock = typeCheckingContext.enclosingBlocks.getFirst(); assert currentBlock != null; diff --git a/src/test/groovy/transform/stc/MethodCallsSTCTest.groovy b/src/test/groovy/transform/stc/MethodCallsSTCTest.groovy index 16359b8..23d7467 100644 --- a/src/test/groovy/transform/stc/MethodCallsSTCTest.groovy +++ b/src/test/groovy/transform/stc/MethodCallsSTCTest.groovy @@ -424,6 +424,56 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase { ''', 'Reference to method is ambiguous' } + // GROOVY-7971 + void testShouldNotFailWithMultiplePossibleMethods() { + assertScript ''' + import groovy.json.JsonOutput + + def test(value) { + def out = new StringBuilder() + def isString = value.class == String + if (isString || value instanceof Map) { + out.append(JsonOutput.toJson(value)) + } + return out.toString() + } + + def string = test('two') + assert string == '"two"' + ''' + } + + // GROOVY-7971 + void testShouldNotFailWithUnionTypeLUB() { + assertScript ''' + class Foo extends AbstractCollection { + def peek() { 3 } + int size() { -1 } + void clear() { } + Iterator iterator() { null } + } + + def method(idx) { + def component = idx == 1 ? new ArrayDeque() : new Stack() + if (idx == 3) component = new Foo() + component.clear() // 'clear' in LUB (AbstractCollection or Serializable or Cloneable) + if (component instanceof ArrayDeque) { + component.addFirst(1) // 'addFirst' only in ArrayDeque + } else if (component instanceof Stack) { + component.addElement(2) // 'addElement' only in Stack + } + if (component instanceof Foo || component instanceof ArrayDeque || component instanceof Stack) { + // checked duck typing + assert component.peek() in 1..3 // 'peek' in ArrayDeque and Stack but not LUB + } + } + + method(1) + method(2) + method(3) + ''' + } + void testInstanceOfOnExplicitParameter() { assertScript ''' 1.with { obj ->
