GROOVY-7970: Can't call private method from outer class when using anonymous inner classes and @cs (closes groovy#452)

paulk-asert · paulk-asert · commit ad566692dbc3 · 2016-11-11T06:06:10.000+10:00
diff --git a/src/main/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java b/src/main/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java
@@ -22,6 +22,7 @@
 import org.codehaus.groovy.ast.ClassHelper;
 import org.codehaus.groovy.ast.ClassNode;
 import org.codehaus.groovy.ast.ConstructorNode;
+import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.GroovyCodeVisitor;
 import org.codehaus.groovy.ast.InnerClassNode;
 import org.codehaus.groovy.ast.MethodNode;
@@ -186,7 +187,16 @@ public void writeSpecialConstructorCall(final ConstructorCallExpression call) {
     /**
      * Attempts to make a direct method call on a bridge method, if it exists.
      */
+    @Deprecated
     protected boolean tryBridgeMethod(MethodNode target, Expression receiver, boolean implicitThis, TupleExpression args) {
+        return tryBridgeMethod(target, receiver, implicitThis, args, null);
+    }
+
+    /**
+     * Attempts to make a direct method call on a bridge method, if it exists.
+     */
+    protected boolean tryBridgeMethod(MethodNode target, Expression receiver, boolean implicitThis,
+                                      TupleExpression args, ClassNode thisClass) {
         ClassNode lookupClassNode;
         if (target.isProtected()) {
             lookupClassNode = controller.getClassNode();
@@ -203,8 +213,22 @@ protected boolean tryBridgeMethod(MethodNode target, Expression receiver, boolea
         MethodNode bridge = bridges==null?null:bridges.get(target);
         if (bridge != null) {
             Expression fixedReceiver = receiver;
-            if (implicitThis && !controller.isInClosure()) {
-                fixedReceiver = new PropertyExpression(new ClassExpression(lookupClassNode), "this");
+            if (implicitThis) {
+                if (!controller.isInClosure()) {
+                    fixedReceiver = new PropertyExpression(new ClassExpression(lookupClassNode), "this");
+                } else if (thisClass != null) {
+                    ClassNode current = thisClass.getOuterClass();
+                    fixedReceiver = new VariableExpression("thisObject", current);
+                    // adjust for multiple levels of nesting if needed
+                    while (current != null && current instanceof InnerClassNode && !lookupClassNode.equals(current)) {
+                        FieldNode thisField = current.getField("this$0");
+                        current = current.getOuterClass();
+                        if (thisField != null) {
+                            fixedReceiver = new PropertyExpression(fixedReceiver, "this$0");
+                            fixedReceiver.setType(current);
+                        }
+                    }
+                }
             }
             ArgumentListExpression newArgs = new ArgumentListExpression(target.isStatic()?new ConstantExpression(null):fixedReceiver);
             for (Expression expression : args.getExpressions()) {
@@ -261,7 +285,7 @@ protected boolean writeDirectMethodCall(final MethodNode target, final boolean i
                     && controller.isInClosure()
                     && !target.isPublic()
                     && target.getDeclaringClass() != classNode) {
-                if (!tryBridgeMethod(target, receiver, implicitThis, args)) {
+                if (!tryBridgeMethod(target, receiver, implicitThis, args, classNode)) {
                     // replace call with an invoker helper call
                     ArrayExpression arr = new ArrayExpression(ClassHelper.OBJECT_TYPE, args.getExpressions());
                     MethodCallExpression mce = new MethodCallExpression(
@@ -281,6 +305,8 @@ protected boolean writeDirectMethodCall(final MethodNode target, final boolean i
                 }
                 return true;
             }
+            Expression fixedReceiver = null;
+            boolean fixedImplicitThis = implicitThis;
             if (target.isPrivate()) {
                 if (tryPrivateMethod(target, implicitThis, receiver, args, classNode)) return true;
             } else if (target.isProtected()) {
@@ -295,16 +321,36 @@ protected boolean writeDirectMethodCall(final MethodNode target, final boolean i
                     controller.getSourceUnit().addError(
                             new SyntaxException("Method " + target.getName() + " is protected in " + target.getDeclaringClass().toString(false),
                                     src.getLineNumber(), src.getColumnNumber(), src.getLastLineNumber(), src.getLastColumnNumber()));
-                } else if (!node.isDerivedFrom(target.getDeclaringClass()) && tryBridgeMethod(target, receiver, implicitThis, args)) {
+                } else if (!node.isDerivedFrom(target.getDeclaringClass()) && tryBridgeMethod(target, receiver, implicitThis, args, classNode)) {
                     return true;
                 }
+            } else if (target.isPublic() && receiver != null) {
+                if (implicitThis
+                        && !classNode.isDerivedFrom(target.getDeclaringClass())
+                        && !classNode.implementsInterface(target.getDeclaringClass())
+                        && classNode instanceof InnerClassNode && controller.isInClosure()) {
+                    ClassNode current = classNode.getOuterClass();
+                    fixedReceiver = new VariableExpression("thisObject", current);
+                    // adjust for multiple levels of nesting if needed
+                    while (current != null && current instanceof InnerClassNode && !classNode.equals(current)) {
+                        FieldNode thisField = current.getField("this$0");
+                        current = current.getOuterClass();
+                        if (thisField != null) {
+                            fixedReceiver = new PropertyExpression(fixedReceiver, "this$0");
+                            fixedReceiver.setType(current);
+                            fixedImplicitThis = false;
+                        }
+                    }
+                }
             }
             if (receiver != null) {
-                if (!(receiver instanceof VariableExpression) || !((VariableExpression) receiver).isSuperExpression()) {
+                boolean callToSuper = receiver instanceof VariableExpression && ((VariableExpression) receiver).isSuperExpression();
+                if (!callToSuper) {
+                    fixedReceiver = fixedReceiver == null ? receiver : fixedReceiver;
                     // in order to avoid calls to castToType, which is the dynamic behaviour, we make sure that we call CHECKCAST instead
                     // then replace the top operand type
-                    Expression checkCastReceiver = new CheckcastReceiverExpression(receiver, target);
-                    return super.writeDirectMethodCall(target, implicitThis, checkCastReceiver, args);
+                    Expression checkCastReceiver = new CheckcastReceiverExpression(fixedReceiver, target);
+                    return super.writeDirectMethodCall(target, fixedImplicitThis, checkCastReceiver, args);
                 }
             }
             return super.writeDirectMethodCall(target, implicitThis, receiver, args);
@@ -316,7 +362,7 @@ private boolean tryPrivateMethod(final MethodNode target, final boolean implicit
         if ((isPrivateBridgeMethodsCallAllowed(declaringClass, classNode) || isPrivateBridgeMethodsCallAllowed(classNode, declaringClass))
                 && declaringClass.getNodeMetaData(PRIVATE_BRIDGE_METHODS) != null
                 && !declaringClass.equals(classNode)) {
-            if (tryBridgeMethod(target, receiver, implicitThis, args)) {
+            if (tryBridgeMethod(target, receiver, implicitThis, args, classNode)) {
                 return true;
             } else if (declaringClass != classNode) {
                 controller.getSourceUnit().addError(new SyntaxException("Cannot call private method " + (target.isStatic() ? "static " : "") +
diff --git a/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -3849,8 +3849,11 @@ protected List<MethodNode> findMethod(
                 collectAllInterfaceMethodsByName(receiver, name, methods);
                 methods.addAll(OBJECT_TYPE.getMethods(name));
             }
-            if (typeCheckingContext.getEnclosingClosure() == null) {
-                // not in a closure
+            // TODO: investigate the trait exclusion a bit further, needed otherwise
+            // CallMethodOfTraitInsideClosureAndClosureParamTypeInference fails saying
+            // not static method can't be called from a static context
+            if (typeCheckingContext.getEnclosingClosure() == null || (receiver instanceof InnerClassNode && !receiver.getName().endsWith("$Trait$Helper"))) {
+                // not in a closure or within an inner class
                 ClassNode parent = receiver;
                 while (parent instanceof InnerClassNode && !parent.isStaticClass()) {
                     parent = parent.getOuterClass();
diff --git a/src/test/groovy/bugs/Groovy7970Bug.groovy b/src/test/groovy/bugs/Groovy7970Bug.groovy
@@ -0,0 +1,125 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package groovy.bugs
+
+class Groovy7970Bug extends GroovyTestCase {
+
+    private static final String getScriptAIC(String visibility, boolean cs) {
+        """
+        ${ cs ? '@groovy.transform.CompileStatic' : ''}
+        class Bar {
+            $visibility String renderTemplate(String arg) { "dummy\$arg" }
+            def foo() {
+                new Object() {
+                    String bar() {
+                       'A'.with{ renderTemplate(it) }
+                    }
+                }
+            }
+        }
+        assert new Bar().foo().bar() == 'dummyA'
+        """
+    }
+
+    private static final String getScriptNestedAIC(String visibility, boolean cs) {
+        """
+        ${ cs ? '@groovy.transform.CompileStatic' : ''}
+        class Bar {
+            $visibility String renderTemplate(String arg) { "dummy\$arg" }
+            def foo() {
+                new Object() {
+                    def bar() {
+                       new Object() { String xxx() { 'B'.with{ renderTemplate(it) } }}
+                    }
+                }
+            }
+        }
+        assert new Bar().foo().bar().xxx() == 'dummyB'
+        """
+    }
+
+    private static final String getScriptInner(String visibility, boolean cs) {
+        """
+        ${ cs ? '@groovy.transform.CompileStatic' : ''}
+        class Bar {
+            $visibility String renderTemplate(String arg) { "dummy\$arg" }
+            class Inner {
+                String baz() {
+                    'C'.with{ renderTemplate(it) }
+                }
+            }
+        }
+        def b = new Bar()
+        assert new Bar.Inner(b).baz() == 'dummyC'
+        """
+    }
+
+    private static final String getScriptNestedInner(String visibility, boolean cs) {
+        """
+        ${ cs ? '@groovy.transform.CompileStatic' : ''}
+        class Bar {
+            $visibility String renderTemplate(String arg) { "dummy\$arg" }
+            class Inner {
+                class InnerInner {
+                    String xxx() { 'D'.with{ renderTemplate(it) } }
+                }
+            }
+        }
+        def b = new Bar()
+        def bi = new Bar.Inner(b)
+        assert new Bar.Inner.InnerInner(bi).xxx() == 'dummyD'
+        """
+    }
+
+    void testMethodAccessFromClosureWithinInnerClass() {
+        assertScript getScriptInner('public', false)
+        assertScript getScriptInner('protected', false)
+        assertScript getScriptInner('private', false)
+        assertScript getScriptNestedInner('public', false)
+        assertScript getScriptNestedInner('protected', false)
+        assertScript getScriptNestedInner('private', false)
+    }
+
+    void testMethodAccessFromClosureWithinInnerClass_CS() {
+        assertScript getScriptInner('public', true)
+        assertScript getScriptInner('protected', true)
+        assertScript getScriptInner('private', true)
+        assertScript getScriptNestedInner('public', true)
+        assertScript getScriptNestedInner('protected', true)
+        assertScript getScriptNestedInner('private', true)
+    }
+
+    void testMethodAccessFromClosureWithinAIC() {
+        assertScript getScriptAIC('public', false)
+        assertScript getScriptAIC('protected', false)
+        assertScript getScriptAIC('private', false)
+        assertScript getScriptNestedAIC('public', false)
+        assertScript getScriptNestedAIC('protected', false)
+        assertScript getScriptNestedAIC('private', false)
+    }
+
+    void testMethodAccessFromClosureWithinAIC_CS() {
+        assertScript getScriptAIC('public', true)
+        assertScript getScriptAIC('protected', true)
+        assertScript getScriptAIC('private', true)
+        assertScript getScriptNestedAIC('public', true)
+        assertScript getScriptNestedAIC('protected', true)
+        assertScript getScriptNestedAIC('private', true)
+    }
+}
