From commits-return-6749-archive-asf-public=cust-asf.ponee.io@groovy.apache.org Thu May 24 15:29:03 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 4B9AB180636 for ; Thu, 24 May 2018 15:29:02 +0200 (CEST) Received: (qmail 69545 invoked by uid 500); 24 May 2018 13:29:01 -0000 Mailing-List: contact commits-help@groovy.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@groovy.apache.org Delivered-To: mailing list commits@groovy.apache.org Received: (qmail 69536 invoked by uid 99); 24 May 2018 13:29:01 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 24 May 2018 13:29:01 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 54274E0B37; Thu, 24 May 2018 13:29:01 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: paulk@apache.org To: commits@groovy.apache.org Message-Id: <63eebb9c1ba347b1b76928de41d1a72b@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: groovy git commit: GROOVY-8562: Wrong closure delegation for property access when using @CompileStatic and DELEGATE_FIRST/DELEGATE_ONLY (closes #717) Date: Thu, 24 May 2018 13:29:01 +0000 (UTC) Repository: groovy Updated Branches: refs/heads/GROOVY_2_5_X 48fc8c248 -> b492c874f GROOVY-8562: Wrong closure delegation for property access when using @CompileStatic and DELEGATE_FIRST/DELEGATE_ONLY (closes #717) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/b492c874 Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/b492c874 Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/b492c874 Branch: refs/heads/GROOVY_2_5_X Commit: b492c874f46d0489dc7aa6712d8c0b8de6a3dcdc Parents: 48fc8c2 Author: Mene Authored: Wed May 16 20:28:13 2018 +0200 Committer: Paul King Committed: Thu May 24 23:28:49 2018 +1000 ---------------------------------------------------------------------- .../VariableExpressionTransformer.java | 4 +- .../stc/StaticTypeCheckingVisitor.java | 21 +++ .../typing/TypeCheckingExtensionSpecTest.groovy | 138 +++++++++++++++++++ 3 files changed, 161 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/b492c874/src/main/java/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java b/src/main/java/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java index d055fc6..548d9b2 100644 --- a/src/main/java/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java +++ b/src/main/java/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java @@ -34,11 +34,11 @@ import org.codehaus.groovy.transform.stc.StaticTypesMarker; public class VariableExpressionTransformer { public Expression transformVariableExpression(VariableExpression expr) { - Expression trn = tryTransformPrivateFieldAccess(expr); + Expression trn = tryTransformDelegateToProperty(expr); if (trn != null) { return trn; } - trn = tryTransformDelegateToProperty(expr); + trn = tryTransformPrivateFieldAccess(expr); if (trn != null) { return trn; } http://git-wip-us.apache.org/repos/asf/groovy/blob/b492c874/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java ---------------------------------------------------------------------- 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 ea3c54b..71d3298 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -100,6 +100,7 @@ import org.codehaus.groovy.syntax.Token; import org.codehaus.groovy.syntax.TokenUtil; import org.codehaus.groovy.syntax.Types; import org.codehaus.groovy.transform.StaticTypesTransformation; +import org.codehaus.groovy.transform.sc.StaticCompilationMetadataKeys; import org.codehaus.groovy.transform.trait.Traits; import org.codehaus.groovy.util.ListHashMap; import org.objectweb.asm.Opcodes; @@ -595,6 +596,26 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } else if (accessedVariable instanceof FieldNode) { FieldNode fieldNode = (FieldNode) accessedVariable; + TypeCheckingContext.EnclosingClosure enclosingClosure = typeCheckingContext.getEnclosingClosure(); + if (enclosingClosure != null) { + // GROOVY-8562 + // when vexp has the same name as a property of the owner, + // the IMPLICIT_RECEIVER must be set in case it's the delegate + if (tryVariableExpressionAsProperty(vexp, vexp.getName())) { + // IMPLICIT_RECEIVER is handled elsewhere + // however other access needs to be fixed for private access + if (vexp.getNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER) == null) { + ClassNode owner = (ClassNode) vexp.getNodeMetaData(StaticCompilationMetadataKeys.PROPERTY_OWNER); + if (owner != null) { + boolean lhsOfEnclosingAssignment = isLHSOfEnclosingAssignment(vexp); + fieldNode = owner.getField(vexp.getName()); + vexp.setAccessedVariable(fieldNode); + checkOrMarkPrivateAccess(vexp, fieldNode, lhsOfEnclosingAssignment); + } + } + } + } + ClassNode parameterizedType = GenericsUtils.findParameterizedType(fieldNode.getDeclaringClass(), typeCheckingContext.getEnclosingClassNode()); if (null != parameterizedType) { ClassNode originalType = fieldNode.getOriginType(); http://git-wip-us.apache.org/repos/asf/groovy/blob/b492c874/src/spec/test/typing/TypeCheckingExtensionSpecTest.groovy ---------------------------------------------------------------------- diff --git a/src/spec/test/typing/TypeCheckingExtensionSpecTest.groovy b/src/spec/test/typing/TypeCheckingExtensionSpecTest.groovy index 7d7baa9..e6b310e 100644 --- a/src/spec/test/typing/TypeCheckingExtensionSpecTest.groovy +++ b/src/spec/test/typing/TypeCheckingExtensionSpecTest.groovy @@ -448,6 +448,144 @@ runner.run() ''' } + void doDelegateResolutionForPropertyReadTest(String strategy, String expected) { + assertScript """import groovy.transform.CompileStatic +class ADelegate { + def x = "delegate" +} + +@CompileStatic +class AClass { + public T closureExecuter( + ADelegate d, + @DelegatesTo(value = ADelegate, strategy = $strategy) Closure c) { + c.resolveStrategy = $strategy + c.delegate = d + return c() + } + + def x = "owner" + + def test() { + def theDelegate = new ADelegate() + def res = closureExecuter(theDelegate) { + return x + } + + return res + } +} +assert new AClass().test() == "$expected" +""" + } + + + void doDelegateResolutionForPropertyWriteTest(String strategy, String expected) { + assertScript """import groovy.transform.CompileStatic +class ADelegate { + def x = "delegate" +} + +@CompileStatic +class AClass { + public T closureExecuter( + ADelegate d, + @DelegatesTo(value = ADelegate, strategy = $strategy) Closure c) { + c.resolveStrategy = $strategy + c.delegate = d + return c() + } + + def x = "owner" + + def test() { + def theDelegate = new ADelegate() + def res = closureExecuter(theDelegate) { + x = "changed" + } + + return [theDelegate.x, this.x].toSet() + } +} +def result = new AClass().test() +def expected = (["owner", "delegate", "changed"] - ["$expected"]).toSet() +assert expected == result +""" + } + + void testDelegateResolutionToPropertyWhenReadingUsingDelegateOnly() { + doDelegateResolutionForPropertyReadTest("Closure.DELEGATE_ONLY", "delegate") + } + + void testDelegateResolutionToPropertyWhenReadingUsingDelegateFirst() { + doDelegateResolutionForPropertyReadTest("Closure.DELEGATE_FIRST", "delegate") + } + + void testDelegateResolutionToPropertyWhenReadingUsingOwnerOnly() { + doDelegateResolutionForPropertyReadTest("Closure.OWNER_ONLY", "owner") + } + + void testDelegateResolutionToPropertyWhenReadingUsingOwnerFirst() { + doDelegateResolutionForPropertyReadTest("Closure.OWNER_FIRST", "owner") + } + + void testDelegateResolutionToPropertyWhenWritingUsingDelegateOnly() { + doDelegateResolutionForPropertyWriteTest("Closure.DELEGATE_ONLY", "delegate") + } + + void testDelegateResolutionToPropertyWhenWritingUsingDelegateFirst() { + doDelegateResolutionForPropertyWriteTest("Closure.DELEGATE_FIRST", "delegate") + } + + void testDelegateResolutionToPropertyWhenWritingUsingOwnerOnly() { + doDelegateResolutionForPropertyWriteTest("Closure.OWNER_ONLY", "owner") + } + + void testDelegateResolutionToPropertyWhenWritingUsingOwnerFirst() { + doDelegateResolutionForPropertyWriteTest("Closure.OWNER_FIRST", "owner") + } + + void testDelegateResolutionToPropertyWhenWritingInsideWith() { + // Failing example provided by Jan Hackel (@jhunovis) in groovy slack + // https://groovy-community.slack.com/files/U9CM8G6AJ/FAR1PJT1U/behavior_of__with__and___compilestatic.groovy + + assertScript '''import groovy.transform.CompileStatic +class DelegateTest { + + @CompileStatic + private static class Person { + String name + int age + + Person copyWithName(String newName) { + return new Person().with { + name = newName + age = this.age + it + } + } + } + + void delegate() { + def oldTim = new Person().with { + name = 'Tim Old' + age = 20 + it + } + def newTim = new Person().with { + name = 'Tim New' + age = 20 + it + } + def copiedTim = oldTim.copyWithName('Tim New') + assert oldTim.name == 'Tim Old' + assert copiedTim.name == newTim.name + assert copiedTim.age == newTim.age + } +} +new DelegateTest().delegate() +''' + } private static class SpecSupport { static int getLongueur(String self) { self.length() }