Repository: groovy Updated Branches: refs/heads/master 6f39e27d7 -> ad566692d
GROOVY-7970: Can't call private method from outer class when using anonymous inner classes and @CS (closes #452) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/ad566692 Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/ad566692 Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/ad566692 Branch: refs/heads/master Commit: ad566692dbc355e1962ba6b61a5b8fe1dad8b966 Parents: 6f39e27 Author: paulk <[email protected]> Authored: Tue Oct 25 14:30:08 2016 +1000 Committer: paulk <[email protected]> Committed: Fri Nov 11 06:06:10 2016 +1000 ---------------------------------------------------------------------- .../classgen/asm/sc/StaticInvocationWriter.java | 62 +++++++-- .../stc/StaticTypeCheckingVisitor.java | 7 +- src/test/groovy/bugs/Groovy7970Bug.groovy | 125 +++++++++++++++++++ 3 files changed, 184 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/ad566692/src/main/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java b/src/main/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java index 0541d20..09ca8ae 100644 --- 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.ASTNode; 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 class StaticInvocationWriter extends InvocationWriter { /** * 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 @@ public class StaticInvocationWriter extends InvocationWriter { 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 @@ public class StaticInvocationWriter extends InvocationWriter { && 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 @@ public class StaticInvocationWriter extends InvocationWriter { } 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 @@ public class StaticInvocationWriter extends InvocationWriter { 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 @@ public class StaticInvocationWriter extends InvocationWriter { 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 " : "") + http://git-wip-us.apache.org/repos/asf/groovy/blob/ad566692/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java index c21166b..f16ff82 100644 --- a/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -3849,8 +3849,11 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { 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(); http://git-wip-us.apache.org/repos/asf/groovy/blob/ad566692/src/test/groovy/bugs/Groovy7970Bug.groovy ---------------------------------------------------------------------- diff --git a/src/test/groovy/bugs/Groovy7970Bug.groovy b/src/test/groovy/bugs/Groovy7970Bug.groovy new file mode 100644 index 0000000..9bda2a3 --- /dev/null +++ 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) + } +}
