You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by pa...@apache.org on 2019/05/01 13:27:33 UTC
[groovy] branch GROOVY_2_5_X updated: GROOVY-9086: @CompileStatic
of closure calls with OWNER_FIRST fail at runtime with ClassCastException
(closes #917)
This is an automated email from the ASF dual-hosted git repository.
paulk pushed a commit to branch GROOVY_2_5_X
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/GROOVY_2_5_X by this push:
new 8021c73 GROOVY-9086: @CompileStatic of closure calls with OWNER_FIRST fail at runtime with ClassCastException (closes #917)
8021c73 is described below
commit 8021c73d386854aca3c2028d11db90bdbdff1486
Author: Paul King <pa...@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 749cbef..9c8b763 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -1752,7 +1752,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;
@@ -3137,53 +3136,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));
}
}
@@ -3418,7 +3415,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);
@@ -3477,39 +3473,6 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
}
}
- // 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()
+ }
+}