GROOVY-7971: @cs flow typing incorrectly casting to map at runtime

apache · Jun 29, 2020 · d10e044 · d10e044
1 parent 0b886e9
commit d10e044
Show file tree

Hide file tree

Showing 4 changed files with 113 additions and 19 deletions.
diff --git a/build.gradle b/build.gradle
@@ -201,10 +201,11 @@ dependencies {
     tools "com.thoughtworks.qdox:qdox:$qdoxVersion"
 
     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
@@ -408,10 +408,17 @@ private static void memorizeInitialExpressions(final MethodNode node) {
     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
@@ -52,6 +52,7 @@
 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;
@@ -185,6 +186,7 @@
 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;
@@ -219,6 +221,7 @@
 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;
@@ -3793,21 +3796,8 @@ protected void typeCheckClosureCall(final Expression callArguments, final ClassN
     @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);
         }
@@ -3822,6 +3812,52 @@ public void visitIfElse(final IfStatement ifElse) {
         }
     }
 
+    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
@@ -423,6 +423,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 ->