Repository: incubator-groovy Updated Branches: refs/heads/master 1dd170db1 -> 6fe6b3181
GROOVY-7456: Do not transform method calls in closures for traits unless they are private trait methods Project: http://git-wip-us.apache.org/repos/asf/incubator-groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-groovy/commit/6fe6b318 Tree: http://git-wip-us.apache.org/repos/asf/incubator-groovy/tree/6fe6b318 Diff: http://git-wip-us.apache.org/repos/asf/incubator-groovy/diff/6fe6b318 Branch: refs/heads/master Commit: 6fe6b31816e43ae3408e2dfdbf2d2bad1a9791c0 Parents: 1dd170d Author: Cedric Champeau <cchamp...@apache.org> Authored: Fri Jun 5 13:51:31 2015 +0200 Committer: Cedric Champeau <cchamp...@apache.org> Committed: Fri Jun 5 13:51:31 2015 +0200 ---------------------------------------------------------------------- .../stc/StaticTypeCheckingVisitor.java | 22 +++++-- .../trait/TraitReceiverTransformer.java | 61 ++++++++++++-------- .../classgen/asm/sc/bugs/Groovy7242Bug.groovy | 18 ++++++ .../transform/traitx/Groovy7456Bug.groovy | 50 ++++++++++++++++ 4 files changed, 121 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-groovy/blob/6fe6b318/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 0c2cfec..f52f736 100644 --- a/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -2696,6 +2696,10 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } } + private static boolean isTraitHelper(ClassNode node) { + return node instanceof InnerClassNode && Traits.isTrait(node.getOuterClass()); + } + protected void addReceivers(final List<Receiver<String>> receivers, final Collection<Receiver<String>> owners, final boolean implicitThis) { @@ -2710,16 +2714,15 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { int strategy = dmd.getStrategy(); ClassNode delegate = dmd.getType(); dmd = dmd.getParent(); - switch (strategy) { case Closure.OWNER_FIRST: receivers.addAll(owners); path.append("delegate"); - receivers.add(new Receiver<String>(delegate, path.toString())); + doAddDelegateReceiver(receivers, path, delegate); break; case Closure.DELEGATE_FIRST: path.append("delegate"); - receivers.add(new Receiver<String>(delegate, path.toString())); + doAddDelegateReceiver(receivers, path, delegate); receivers.addAll(owners); break; case Closure.OWNER_ONLY: @@ -2728,7 +2731,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { break; case Closure.DELEGATE_ONLY: path.append("delegate"); - receivers.add(new Receiver<String>(delegate, path.toString())); + doAddDelegateReceiver(receivers, path, delegate); dmd = null; break; } @@ -2736,6 +2739,13 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } } + private void doAddDelegateReceiver(final List<Receiver<String>> receivers, final StringBuilder path, final ClassNode delegate) { + receivers.add(new Receiver<String>(delegate, path.toString())); + if (isTraitHelper(delegate)) { + receivers.add(new Receiver<String>(delegate.getOuterClass(), path.toString())); + } + } + @Override public void visitMethodCallExpression(MethodCallExpression call) { final String name = call.getMethodAsString(); @@ -3920,10 +3930,10 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { return node; } else if (exp instanceof VariableExpression) { VariableExpression vexp = (VariableExpression) exp; - if (vexp == VariableExpression.THIS_EXPRESSION) return makeThis(); - if (vexp == VariableExpression.SUPER_EXPRESSION) return makeSuper(); ClassNode selfTrait = isTraitSelf(vexp); if (selfTrait!=null) return makeSelf(selfTrait); + if (vexp == VariableExpression.THIS_EXPRESSION) return makeThis(); + if (vexp == VariableExpression.SUPER_EXPRESSION) return makeSuper(); final Variable variable = vexp.getAccessedVariable(); if (variable instanceof FieldNode) { checkOrMarkPrivateAccess(vexp, (FieldNode) variable); http://git-wip-us.apache.org/repos/asf/incubator-groovy/blob/6fe6b318/src/main/org/codehaus/groovy/transform/trait/TraitReceiverTransformer.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/transform/trait/TraitReceiverTransformer.java b/src/main/org/codehaus/groovy/transform/trait/TraitReceiverTransformer.java index fe1298e..4cd28f8 100644 --- a/src/main/org/codehaus/groovy/transform/trait/TraitReceiverTransformer.java +++ b/src/main/org/codehaus/groovy/transform/trait/TraitReceiverTransformer.java @@ -53,13 +53,10 @@ import java.util.Collection; import java.util.List; /** - * This expression transformer is used internally by the {@link org.codehaus.groovy.transform.trait.TraitASTTransformation trait} - * AST transformation to change the receiver of a message on "this" into a static method call on the trait helper class. - * <p></p> - * In a nutshell, code like this one in a trait:<p></p> - * <code>void foo() { this.bar() }</code> - * is transformed into: - * <code>void foo() { TraitHelper$bar(this) }</code> + * This expression transformer is used internally by the {@link org.codehaus.groovy.transform.trait.TraitASTTransformation + * trait} AST transformation to change the receiver of a message on "this" into a static method call on the trait helper + * class. <p></p> In a nutshell, code like this one in a trait:<p></p> <code>void foo() { this.bar() }</code> is + * transformed into: <code>void foo() { TraitHelper$bar(this) }</code> * * @author Cedric Champeau * @since 2.3.0 @@ -74,6 +71,8 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { private final ClassNode fieldHelper; private final Collection<String> knownFields; + private boolean inClosure; + public TraitReceiverTransformer(VariableExpression thisObject, SourceUnit unit, final ClassNode traitClass, ClassNode fieldHelper, Collection<String> knownFields) { this.weaved = thisObject; this.unit = unit; @@ -91,7 +90,7 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { public Expression transform(final Expression exp) { ClassNode weavedType = weaved.getOriginType(); if (exp instanceof BinaryExpression) { - return transformBinaryExpression((BinaryExpression)exp, weavedType); + return transformBinaryExpression((BinaryExpression) exp, weavedType); } else if (exp instanceof StaticMethodCallExpression) { StaticMethodCallExpression call = (StaticMethodCallExpression) exp; ClassNode ownerType = call.getOwnerType(); @@ -116,7 +115,7 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { return transformSuperMethodCall(call); } } else if (exp instanceof FieldExpression) { - return transformFieldExpression((FieldExpression)exp); + return transformFieldExpression((FieldExpression) exp); } else if (exp instanceof VariableExpression) { VariableExpression vexp = (VariableExpression) exp; Variable accessedVariable = vexp.getAccessedVariable(); @@ -129,9 +128,9 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { receiver = createStaticReceiver(receiver); } mce = new MethodCallExpression( - receiver, - Traits.helperGetterName(fn), - ArgumentListExpression.EMPTY_ARGUMENTS + receiver, + Traits.helperGetterName(fn), + ArgumentListExpression.EMPTY_ARGUMENTS ); mce.setSourcePosition(exp); mce.setImplicitThis(false); @@ -198,7 +197,10 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { ); mce.setImplicitThis(false); mce.setSourcePosition(exp); + boolean oldInClosure = inClosure; + inClosure = true; ((ClosureExpression) exp).getCode().visit(this); + inClosure = oldInClosure; // The rewrite we do is causing some troubles with type checking, which will // not be able to perform closure parameter type inference // so we store the replacement, which will be done *after* type checking. @@ -238,18 +240,18 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { && (((PropertyExpression) leftExpression).isImplicitThis() || "this".equals(((PropertyExpression) leftExpression).getObjectExpression().getText()))) { leftFieldName = ((PropertyExpression) leftExpression).getPropertyAsString(); FieldNode fn = tryGetFieldNode(weavedType, leftFieldName); - if (fieldHelper == null || fn==null && !fieldHelper.hasPossibleMethod(Traits.helperSetterName(new FieldNode(leftFieldName, 0, ClassHelper.OBJECT_TYPE, weavedType, null)), rightExpression)) { + if (fieldHelper == null || fn == null && !fieldHelper.hasPossibleMethod(Traits.helperSetterName(new FieldNode(leftFieldName, 0, ClassHelper.OBJECT_TYPE, weavedType, null)), rightExpression)) { return createAssignmentToField(rightExpression, operation, leftFieldName); } } - if (leftFieldName!=null) { + if (leftFieldName != null) { FieldNode fn = weavedType.getDeclaredField(leftFieldName); FieldNode staticField = tryGetFieldNode(weavedType, leftFieldName); - if (fn==null) { + if (fn == null) { fn = new FieldNode(leftFieldName, 0, ClassHelper.OBJECT_TYPE, weavedType, null); } Expression receiver = createFieldHelperReceiver(); - boolean isStatic = staticField!=null && staticField.isStatic(); + boolean isStatic = staticField != null && staticField.isStatic(); if (fn.isStatic()) { // DO NOT USE isStatic variable here! receiver = new PropertyExpression(receiver, "class"); } @@ -268,10 +270,10 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { Expression leftTransform = transform(leftExpression); Expression rightTransform = transform(rightExpression); Expression ret = - exp instanceof DeclarationExpression ?new DeclarationExpression( + exp instanceof DeclarationExpression ? new DeclarationExpression( leftTransform, operation, rightTransform - ): - new BinaryExpression(leftTransform, operation, rightTransform); + ) : + new BinaryExpression(leftTransform, operation, rightTransform); ret.setSourcePosition(exp); ret.copyNodeMetaData(exp); return ret; @@ -307,9 +309,9 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { private FieldNode tryGetFieldNode(final ClassNode weavedType, final String fieldName) { FieldNode fn = weavedType.getDeclaredField(fieldName); - if (fn==null && ClassHelper.CLASS_Type.equals(weavedType)) { + if (fn == null && ClassHelper.CLASS_Type.equals(weavedType)) { GenericsType[] genericsTypes = weavedType.getGenericsTypes(); - if (genericsTypes !=null && genericsTypes.length==1) { + if (genericsTypes != null && genericsTypes.length == 1) { // for static properties fn = genericsTypes[0].getType().getDeclaredField(fieldName); } @@ -323,7 +325,7 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { private Expression transformSuperMethodCall(final MethodCallExpression call) { String method = call.getMethodAsString(); - if (method==null) { + if (method == null) { throwSuperError(call); } @@ -350,7 +352,6 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { } - private Expression transformMethodCallOnThis(final MethodCallExpression call) { Expression method = call.getMethod(); Expression arguments = call.getArguments(); @@ -363,6 +364,18 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { } } } + if (inClosure) { + MethodCallExpression transformed = new MethodCallExpression( + (Expression) call.getReceiver(), + call.getMethod(), + transform(call.getArguments()) + ); + transformed.setSourcePosition(call); + transformed.setSafe(call.isSafe()); + transformed.setSpreadSafe(call.isSpreadSafe()); + transformed.setImplicitThis(call.isImplicitThis()); + return transformed; + } MethodCallExpression transformed = new MethodCallExpression( weaved, @@ -391,7 +404,7 @@ class TraitReceiverTransformer extends ClassCodeExpressionTransformer { } private Expression createFieldHelperReceiver() { - return ClassHelper.CLASS_Type.equals(weaved.getOriginType())?weaved:new CastExpression(fieldHelper,weaved); + return ClassHelper.CLASS_Type.equals(weaved.getOriginType()) ? weaved : new CastExpression(fieldHelper, weaved); } private ArgumentListExpression createArgumentList(final Expression origCallArgs) { http://git-wip-us.apache.org/repos/asf/incubator-groovy/blob/6fe6b318/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy7242Bug.groovy ---------------------------------------------------------------------- diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy7242Bug.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy7242Bug.groovy index 817443e..17e6d62 100644 --- a/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy7242Bug.groovy +++ b/src/test/org/codehaus/groovy/classgen/asm/sc/bugs/Groovy7242Bug.groovy @@ -80,4 +80,22 @@ class Groovy7242Bug extends StaticTypeCheckingTestCase implements StaticCompilat assert a.x == 1 ''' } + + void testCallPrivateMethodOfTraitInsideClosure() { + assertScript ''' + trait MyTrait { + def f() { + ['a'].collect {String it -> f2(it)} + } + + private f2(String s) { + s.toUpperCase() + } + } + + class A implements MyTrait {} + def a = new A() + assert a.f() == ['A'] + ''' + } } http://git-wip-us.apache.org/repos/asf/incubator-groovy/blob/6fe6b318/src/test/org/codehaus/groovy/transform/traitx/Groovy7456Bug.groovy ---------------------------------------------------------------------- diff --git a/src/test/org/codehaus/groovy/transform/traitx/Groovy7456Bug.groovy b/src/test/org/codehaus/groovy/transform/traitx/Groovy7456Bug.groovy new file mode 100644 index 0000000..ecc5c46 --- /dev/null +++ b/src/test/org/codehaus/groovy/transform/traitx/Groovy7456Bug.groovy @@ -0,0 +1,50 @@ +/* + * Copyright 2003-2015 the original author or authors. + * + * Licensed 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 org.codehaus.groovy.transform.traitx + +class Groovy7456Bug extends GroovyTestCase { + void testShouldAllowBuilderInTrait() { + assertScript ''' + import groovy.xml.* + + trait MyBuilder { + def build2() { + def mkp = new MarkupBuilder() + + mkp.foo { + bar() + } + } + } + + class Foo implements MyBuilder { + + def build1() { + def mkp = new MarkupBuilder() + + mkp.foo { + bar() + } + } + } + + def foo = new Foo() + foo.build1() + foo.build2() + ''' + } +}