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 2020/07/18 13:22:07 UTC
[groovy] branch master updated: GROOVY-7996: STC: error for
mismatched closure resolve strategies (closes #1303)
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 fa6c04c GROOVY-7996: STC: error for mismatched closure resolve strategies (closes #1303)
fa6c04c is described below
commit fa6c04cfb94242579f754294bd78a96f6c3c3c5c
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Wed Jul 8 13:40:50 2020 -0500
GROOVY-7996: STC: error for mismatched closure resolve strategies (closes #1303)
Provide the user some clue that runtime behavior will not match declared
behavior (in explicit or implicit @DelegatesTo metadata).
---
.../codehaus/groovy/ast/tools/ClosureUtils.java | 31 ++++++++++++++---
.../transform/stc/StaticTypeCheckingVisitor.java | 39 ++++++++++++++++++----
.../groovy/transform/stc/DelegatesToSTCTest.groovy | 25 ++++++++++++++
.../asm/sc/DelegatesToStaticCompileTest.groovy | 4 +--
4 files changed, 85 insertions(+), 14 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/ast/tools/ClosureUtils.java b/src/main/java/org/codehaus/groovy/ast/tools/ClosureUtils.java
index 761e8eb..f9e59d5 100644
--- a/src/main/java/org/codehaus/groovy/ast/tools/ClosureUtils.java
+++ b/src/main/java/org/codehaus/groovy/ast/tools/ClosureUtils.java
@@ -37,7 +37,7 @@ public class ClosureUtils {
* @throws java.lang.IllegalArgumentException when expression is null
* @throws java.lang.Exception when closure can't be read from source
*/
- public static String convertClosureToSource(ReaderSource readerSource, ClosureExpression expression) throws Exception {
+ public static String convertClosureToSource(final ReaderSource readerSource, final ClosureExpression expression) throws Exception {
String source = GeneralUtils.convertASTToSource(readerSource, expression);
if (!source.startsWith("{")) {
throw new Exception("Error converting ClosureExpression into source code. Closures must start with {. Found: " + source);
@@ -50,7 +50,7 @@ public class ClosureUtils {
* @param c a Closure
* @return true if it has exactly one argument and the type is char or Character
*/
- public static boolean hasSingleCharacterArg(Closure c) {
+ public static boolean hasSingleCharacterArg(final Closure c) {
if (c.getMaximumNumberOfParameters() != 1) return false;
String typeName = c.getParameterTypes()[0].getName();
return typeName.equals("char") || typeName.equals("java.lang.Character");
@@ -61,7 +61,7 @@ public class ClosureUtils {
* @param c a Closure
* @return true if it has exactly one argument and the type is String
*/
- public static boolean hasSingleStringArg(Closure c) {
+ public static boolean hasSingleStringArg(final Closure c) {
if (c.getMaximumNumberOfParameters() != 1) return false;
String typeName = c.getParameterTypes()[0].getName();
return typeName.equals("java.lang.String");
@@ -70,14 +70,35 @@ public class ClosureUtils {
/**
* @return true if the ClosureExpression has an implicit 'it' parameter
*/
- public static boolean hasImplicitParameter(ClosureExpression ce) {
+ public static boolean hasImplicitParameter(final ClosureExpression ce) {
return ce.getParameters() != null && ce.getParameters().length == 0;
}
/**
* @return the parameters for the ClosureExpression
*/
- public static Parameter[] getParametersSafe(ClosureExpression ce) {
+ public static Parameter[] getParametersSafe(final ClosureExpression ce) {
return ce.getParameters() != null ? ce.getParameters() : Parameter.EMPTY_ARRAY;
}
+
+ /**
+ * Returns the constant name associated with the given resolve strategy.
+ *
+ * @see {@link Closure#DELEGATE_FIRST}, et al.
+ *
+ * @since 3.0.5
+ */
+ public static String getResolveStrategyName(final int resolveStrategy) {
+ switch (resolveStrategy) {
+ case Closure.DELEGATE_FIRST:
+ return "DELEGATE_FIRST";
+ case Closure.DELEGATE_ONLY:
+ return "DELEGATE_ONLY";
+ case Closure.OWNER_ONLY:
+ return "OWNER_ONLY";
+ case Closure.TO_SELF:
+ return "TO_SELF";
+ }
+ return "OWNER_FIRST";
+ }
}
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 cc8cfc1..f29513c 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -177,6 +177,7 @@ import static org.codehaus.groovy.ast.ClassHelper.long_TYPE;
import static org.codehaus.groovy.ast.ClassHelper.short_TYPE;
import static org.codehaus.groovy.ast.ClassHelper.void_WRAPPER_TYPE;
import static org.codehaus.groovy.ast.tools.ClosureUtils.getParametersSafe;
+import static org.codehaus.groovy.ast.tools.ClosureUtils.getResolveStrategyName;
import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
import static org.codehaus.groovy.ast.tools.GeneralUtils.binX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.block;
@@ -3487,14 +3488,27 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
visitMethodCallArguments(receiver, argumentList, true, target);
}
if (target != null) {
- List<Expression> argExpressions = argumentList.getExpressions();
+ List<Expression> arguments = argumentList.getExpressions();
Parameter[] parameters = target.getParameters();
- for (int i = 0; i < argExpressions.size() && i < parameters.length; i += 1) {
- Expression arg = argExpressions.get(i);
- ClassNode aType = getType(arg), pType = parameters[i].getType();
- if (CLOSURE_TYPE.equals(aType) && CLOSURE_TYPE.equals(pType) && !isAssignableTo(aType, pType)) {
- addNoMatchingMethodError(receiver, name, getArgumentTypes(argumentList), call);
- call.removeNodeMetaData(DIRECT_METHOD_CALL_TARGET);
+ for (int i = 0, n = Math.min(arguments.size(), parameters.length); i < n; i += 1) {
+ Expression argument = arguments.get(i);
+ ClassNode aType = getType(argument), pType = parameters[i].getType();
+ if (CLOSURE_TYPE.equals(aType) && CLOSURE_TYPE.equals(pType)) {
+ // GROOVY-8310: check closure generics
+ if (!isAssignableTo(aType, pType) /*&& !extension.handleIncompatibleReturnType(getReturnStatement(argument), aType)*/) {
+ addNoMatchingMethodError(receiver, name, getArgumentTypes(argumentList), call);
+ call.removeNodeMetaData(DIRECT_METHOD_CALL_TARGET);
+ break;
+ }
+ // GROOVY-7996: check delegation metadata of closure parameter used as method call argument
+ if (argument instanceof VariableExpression && ((VariableExpression) argument).getAccessedVariable() instanceof Parameter) {
+ // TODO: Check additional delegation metadata like type (see checkClosureWithDelegatesTo).
+ int incomingStrategy = getResolveStrategy((Parameter) ((VariableExpression) argument).getAccessedVariable());
+ int outgoingStrategy = getResolveStrategy(parameters[i]);
+ if (incomingStrategy != outgoingStrategy) {
+ addStaticTypeError("Closure parameter with resolve strategy " + getResolveStrategyName(incomingStrategy) + " passed to method with resolve strategy " + getResolveStrategyName(outgoingStrategy), argument);
+ }
+ }
}
}
}
@@ -3506,6 +3520,17 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
}
}
+ private int getResolveStrategy(final Parameter parameter) {
+ List<AnnotationNode> annotations = parameter.getAnnotations(DELEGATES_TO);
+ if (annotations != null && !annotations.isEmpty()) {
+ Expression strategy = annotations.get(0).getMember("strategy");
+ if (strategy != null) {
+ return (Integer) evaluateExpression(castX(Integer_TYPE, strategy), getSourceUnit().getConfiguration());
+ }
+ }
+ return Closure.OWNER_FIRST;
+ }
+
private void inferMethodReferenceType(final MethodCallExpression call, final ClassNode receiver, final ArgumentListExpression argumentList) {
if (call == null) return;
if (receiver == null) return;
diff --git a/src/test/groovy/transform/stc/DelegatesToSTCTest.groovy b/src/test/groovy/transform/stc/DelegatesToSTCTest.groovy
index cb155f0..ef6ef59 100644
--- a/src/test/groovy/transform/stc/DelegatesToSTCTest.groovy
+++ b/src/test/groovy/transform/stc/DelegatesToSTCTest.groovy
@@ -817,4 +817,29 @@ class DelegatesToSTCTest extends StaticTypeCheckingTestCase {
}
'''
}
+
+ // GROOVY-7996
+ void testErrorForMismatchedClosureResolveStrategy() {
+ shouldFailWithMessages '''
+ class Foo {
+ def build(Closure x) { // resolve strategy OWNER_FIRST
+ this.with(x) // resolve strategy DELEGATE_FIRST
+ }
+ def propertyMissing(String name) {
+ 'something'
+ }
+ }
+
+ class Bar {
+ protected List bars = []
+ def baz() {
+ new Foo().build {
+ bars.isEmpty() // fails if executed; delegate's propertyMissing takes precedence
+ }
+ }
+ }
+
+ new Bar().baz()
+ ''', 'Closure parameter with resolve strategy OWNER_FIRST passed to method with resolve strategy DELEGATE_FIRST'
+ }
}
diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/DelegatesToStaticCompileTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/DelegatesToStaticCompileTest.groovy
index f6ce151..73c9c53 100644
--- a/src/test/org/codehaus/groovy/classgen/asm/sc/DelegatesToStaticCompileTest.groovy
+++ b/src/test/org/codehaus/groovy/classgen/asm/sc/DelegatesToStaticCompileTest.groovy
@@ -92,7 +92,7 @@ class DelegatesToStaticCompileTest extends DelegatesToSTCTest implements StaticC
}
class Handler {
- def doWithEntity(@DelegatesTo(Entity) Closure c) {
+ def doWithEntity(@DelegatesTo(value=Entity, strategy=Closure.DELEGATE_FIRST) Closure c) {
new Entity().with(c)
}
@@ -111,4 +111,4 @@ class DelegatesToStaticCompileTest extends DelegatesToSTCTest implements StaticC
assert !bytecode.contains('INVOKESTATIC org/codehaus/groovy/runtime/ScriptBytecodeAdapter.castToType')
}
}
-}
\ No newline at end of file
+}