groovy-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pa...@apache.org
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 GMT
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 <arndt@menedev.de>
Authored: Wed May 16 20:28:13 2018 +0200
Committer: Paul King <paulk@asert.com.au>
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> T closureExecuter(
+            ADelegate d,
+            @DelegatesTo(value = ADelegate, strategy = $strategy) Closure<T> 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> T closureExecuter(
+            ADelegate d,
+            @DelegatesTo(value = ADelegate, strategy = $strategy) Closure<T> 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() }


Mime
View raw message