groovy-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pa...@apache.org
Subject [groovy] branch master updated: GROOVY-9086: @CompileStatic of closure calls with OWNER_FIRST fail at runtime with ClassCastException (closes #917)
Date Wed, 01 May 2019 12:40:39 GMT
This is an automated email from the ASF dual-hosted git repository.

paulk pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git


The following commit(s) were added to refs/heads/master by this push:
     new 5d529da  GROOVY-9086: @CompileStatic of closure calls with OWNER_FIRST fail at runtime
with ClassCastException (closes #917)
5d529da is described below

commit 5d529da542befe55032e87de1f71a5010a97310b
Author: Paul King <paulk@asert.com.au>
AuthorDate: Wed May 1 06:52:35 2019 +1000

    GROOVY-9086: @CompileStatic of closure calls with OWNER_FIRST fail at runtime with ClassCastException
(closes #917)
---
 .../transform/stc/StaticTypeCheckingVisitor.java   | 111 +++++++------------
 src/test/gls/closures/ResolveStrategyTest.groovy   | 120 +++++++++++++++++++++
 2 files changed, 157 insertions(+), 74 deletions(-)

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 d531948..3d4dc78 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -1857,7 +1857,6 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport
{
         if (visitor != null) visitor.visitProperty(propertyNode);
         storeWithResolve(propertyNode.getOriginType(), receiver, propertyNode.getDeclaringClass(),
propertyNode.isStatic(), expressionToStoreOn);
         if (delegationData != null) {
-            delegationData = adjustData(delegationData, receiver, typeCheckingContext.delegationMetadata);
             expressionToStoreOn.putNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER, delegationData);
         }
         return true;
@@ -3281,53 +3280,51 @@ 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) {
-        if (typeCheckingContext.delegationMetadata == null || !implicitThis) {
+        if (!implicitThis || typeCheckingContext.delegationMetadata == null) {
             receivers.addAll(owners);
-            return;
-        }
-
-        DelegationMetadata dmd = typeCheckingContext.delegationMetadata;
-        StringBuilder path = new StringBuilder();
-        while (dmd != null) {
-            int strategy = dmd.getStrategy();
-            ClassNode delegate = dmd.getType();
-            dmd = dmd.getParent();
-            switch (strategy) {
-                case Closure.OWNER_FIRST:
-                    receivers.addAll(owners);
-                    path.append("delegate");
-                    doAddDelegateReceiver(receivers, path, delegate);
-                    break;
-                case Closure.DELEGATE_FIRST:
-                    path.append("delegate");
-                    doAddDelegateReceiver(receivers, path, delegate);
-                    receivers.addAll(owners);
-                    break;
-                case Closure.OWNER_ONLY:
+        } else {
+            addReceivers(receivers, owners, typeCheckingContext.delegationMetadata, "");
+        }
+    }
+
+    private static void addReceivers(final List<Receiver<String>> receivers,
+                                     final Collection<Receiver<String>> owners,
+                                     final DelegationMetadata dmd,
+                                     final String path) {
+        int strategy = dmd.getStrategy();
+        switch (strategy) {
+            case Closure.DELEGATE_ONLY:
+            case Closure.DELEGATE_FIRST:
+                addDelegateReceiver(receivers, dmd.getType(), path + "delegate");
+                if (strategy == Closure.DELEGATE_FIRST) {
+                    if (dmd.getParent() == null) {
+                        receivers.addAll(owners);
+                    } else {
+                        addReceivers(receivers, owners, dmd.getParent(), path + "owner.");
+                    }
+                }
+                break;
+            case Closure.OWNER_ONLY:
+            case Closure.OWNER_FIRST:
+                if (dmd.getParent() == null) {
                     receivers.addAll(owners);
-                    dmd = null;
-                    break;
-                case Closure.DELEGATE_ONLY:
-                    path.append("delegate");
-                    doAddDelegateReceiver(receivers, path, delegate);
-                    dmd = null;
-                    break;
-            }
-            path.append('.');
+                } else {
+                    addReceivers(receivers, owners, dmd.getParent(), path + "owner.");
+                }
+                if (strategy == Closure.OWNER_FIRST) {
+                    addDelegateReceiver(receivers, dmd.getType(), path + "delegate");
+                }
+                break;
         }
     }
 
-    private static 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()));
+    private static void addDelegateReceiver(final List<Receiver<String>> receivers,
final ClassNode delegate, final String path) {
+        receivers.add(new Receiver<String>(delegate, path));
+        if (Traits.isTrait(delegate.getOuterClass())) {
+            receivers.add(new Receiver<String>(delegate.getOuterClass(), path));
         }
     }
 
@@ -3564,7 +3561,6 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport
{
                             }
                             String data = chosenReceiver.getData();
                             if (data != null) {
-                                data = adjustData(data, chosenReceiver.getType(), typeCheckingContext.delegationMetadata);
                                 // the method which has been chosen is supposed to be a call
on delegate or owner
                                 // so we store the information so that the static compiler
may reuse it
                                 call.putNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER,
data);
@@ -3695,39 +3691,6 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport
{
         return newParameters;
     }
 
-    // adjust data to handle cases like nested .with since we didn't have enough information
earlier
-    // TODO see if we can make the earlier detection smarter and then remove this adjustment
-    private static String adjustData(String data, ClassNode type, DelegationMetadata dmd)
{
-        StringBuilder path = new StringBuilder();
-        int i = 0;
-        String[] propertyPath = data.split("\\.");
-        while (dmd != null) {
-            int strategy = dmd.getStrategy();
-            ClassNode delegate = dmd.getType();
-            dmd = dmd.getParent();
-            switch (strategy) {
-                case Closure.DELEGATE_FIRST:
-                    if (!delegate.isDerivedFrom(CLOSURE_TYPE) && !delegate.isDerivedFrom(type))
{
-                        path.append("owner"); // must be non-delegate case
-                    } else {
-                        path.append("delegate");
-                    }
-                    break;
-                default:
-                    if (i >= propertyPath.length) return data;
-                    path.append(propertyPath[i]);
-            }
-            if (type.equals(delegate)) break;
-            i++;
-            if (dmd != null) path.append('.');
-        }
-        String result = path.toString();
-        if (!result.isEmpty()) {
-            return result;
-        }
-        return data;
-    }
-
     /**
      * e.g. c(b(a())),      a() and b() are nested method call, but c() is not
      *      new C(b(a()))   a() and b() are nested method call
diff --git a/src/test/gls/closures/ResolveStrategyTest.groovy b/src/test/gls/closures/ResolveStrategyTest.groovy
new file mode 100644
index 0000000..ea7cb8d
--- /dev/null
+++ b/src/test/gls/closures/ResolveStrategyTest.groovy
@@ -0,0 +1,120 @@
+/*
+ *  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 gls.closures
+
+import groovy.transform.CompileStatic
+import static groovy.lang.Closure.*
+
+class ResolveStrategyTest extends GroovyTestCase {
+    void testDynamicSettingOfResolveStrategy() {
+        new MyClass().with {
+            assert run(DELEGATE_ONLY) == 12340000 // (*)
+            assert run(DELEGATE_FIRST) == 12040030
+            assert run(OWNER_ONLY) == 1234 // (*)
+            assert run(OWNER_FIRST) == 41230
+
+            assert runOwnerOnly { m1() + m2() + m3() } == 1230
+            assert runOwnerOnly { m1() + m2() + m3() + m4() } == 1234 // (*)
+            assert runOwnerFirst { m1() + m2() + m3() + m4() } == 41230
+            assert runDelegateOnly { m1() + m2() + m3() + m4() } == 12340000 // (*)
+            assert runDelegateOnly { m1() + m2() + m4() } == 12040000
+            assert runDelegateFirst { m1() + m2() + m3() + m4() } == 12340000 // (**)
+
+            // nested cases
+            assert runDelegateFirst { runOwnerFirst { m1() + m2() + m3() + m4() } } == 41230
+            assert runOwnerFirst { runDelegateFirst { m1() + m2() + m3() + m4() } } == 12340000
// (**)
+            assert runOwnerFirst { runOwnerFirst { m1() + m2() + m3() + m4() } } == 41230
+            assert runDelegateFirst { runDelegateFirst { m1() + m2() + m3() + m4() } } ==
12340000 // (**)
+            // (*) involves methodMissing as forced by ONLY strategy (no equivalent CS case
below)
+            // (**) involves methodMissing since delegate methodMissing has precedence over
explicit owner method
+        }
+    }
+
+    @CompileStatic
+    void testStaticCases() {
+        new MyClass().with {
+            assert runOwnerOnly { m1() + m2() + m3() } == 1230
+            assert runOwnerFirst { m1() + m2() + m3() + m4() } == 41230
+            assert runDelegateOnly { m1() + m2() + m4() } == 12040000
+            assert runDelegateFirst { m1() + m2() + m3() + m4() } == 12040030 // (*)
+
+            // nested cases (GROOVY-9086)
+            assert runDelegateFirst { runOwnerFirst { m1() + m2() + m3() + m4() } } == 12040030
// (*)
+            assert runOwnerFirst { runDelegateFirst { m1() + m2() + m3() + m4() } } == 12040030
// (*)
+            assert runOwnerFirst { runOwnerFirst { m1() + m2() + m3() + m4() } } == 41230
+            assert runDelegateFirst { runDelegateFirst { m1() + m2() + m3() + m4() } } ==
12040030 // (*)
+            // (*) different to dynamic since static assumes no methodMissing
+        }
+    }
+}
+
+class Delegate {
+    int m1() { 10000000 }
+
+    int m2() { 2000000 }
+
+    int m4() { 40000 }
+
+    def methodMissing(String name, args) {
+        if (name.size() == 2) return 300000
+        else throw new MissingMethodException(name, Delegate, args)
+    }
+}
+
+class MyClass {
+    int m1() { 1000 }
+
+    int m2() { 200 }
+
+    int m3() { 30 }
+
+    def methodMissing(String name, args) { 4 }
+
+    def run(int rs) {
+        def answer = -1
+        Closure c = { answer = m1() + m2() + m3() + m4() }
+        c.resolveStrategy = rs
+        c.delegate = new Delegate()
+        c()
+        answer
+    }
+
+    def runDelegateFirst(@DelegatesTo(value = Delegate, strategy = DELEGATE_FIRST) Closure
c) {
+        c.delegate = new Delegate()
+        c.resolveStrategy = DELEGATE_FIRST
+        c()
+    }
+
+    def runDelegateOnly(@DelegatesTo(value = Delegate, strategy = DELEGATE_ONLY) Closure
c) {
+        c.delegate = new Delegate()
+        c.resolveStrategy = DELEGATE_ONLY
+        c()
+    }
+
+    def runOwnerFirst(@DelegatesTo(Delegate) Closure c) {
+        c.delegate = new Delegate()
+        c()
+    }
+
+    def runOwnerOnly(@DelegatesTo(value = Delegate, strategy = OWNER_ONLY) Closure c) {
+        c.delegate = new Delegate()
+        c.resolveStrategy = OWNER_ONLY
+        c()
+    }
+}


Mime
View raw message