You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by em...@apache.org on 2021/12/27 19:47:05 UTC
[groovy] branch master updated: GROOVY-6603: STC: check closure calls in methods with parameter metadata
This is an automated email from the ASF dual-hosted git repository.
emilles 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 71b4c77 GROOVY-6603: STC: check closure calls in methods with parameter metadata
71b4c77 is described below
commit 71b4c77f02a548ba7ee0b8cac9e92b711ac5faad
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Mon Dec 27 13:31:12 2021 -0600
GROOVY-6603: STC: check closure calls in methods with parameter metadata
- warn that `SimpleType` cannot handle type with generics
---
.../ast/builder/AstSpecificationCompiler.groovy | 4 +-
.../groovy/transform/stc/ClosureSignatureHint.java | 43 +++++++++---
src/main/java/groovy/transform/stc/SimpleType.java | 13 ++--
.../transform/stc/StaticTypeCheckingVisitor.java | 81 +++++++++++++---------
.../stc/ClosureParamTypeInferenceSTCTest.groovy | 31 +++++----
.../groovy-sql/src/main/java/groovy/sql/Sql.java | 11 +--
6 files changed, 115 insertions(+), 68 deletions(-)
diff --git a/src/main/groovy/org/codehaus/groovy/ast/builder/AstSpecificationCompiler.groovy b/src/main/groovy/org/codehaus/groovy/ast/builder/AstSpecificationCompiler.groovy
index 5018803..fb60d81 100644
--- a/src/main/groovy/org/codehaus/groovy/ast/builder/AstSpecificationCompiler.groovy
+++ b/src/main/groovy/org/codehaus/groovy/ast/builder/AstSpecificationCompiler.groovy
@@ -20,7 +20,7 @@ package org.codehaus.groovy.ast.builder
import groovy.transform.CompileStatic
import groovy.transform.stc.ClosureParams
-import groovy.transform.stc.SimpleType
+import groovy.transform.stc.FromString
import org.codehaus.groovy.ast.ASTNode
import org.codehaus.groovy.ast.AnnotationNode
import org.codehaus.groovy.ast.ClassHelper
@@ -167,7 +167,7 @@ class AstSpecificationCompiler implements GroovyInterceptable {
*/
@CompileStatic
private void captureAndCreateNode(String name, @DelegatesTo(AstSpecificationCompiler) Closure argBlock,
- @ClosureParams(value=SimpleType, options="java.util.List<org.codehaus.groovy.ast.ASTNode>") Closure ctorBlock) {
+ @ClosureParams(value=FromString, options="java.util.List<org.codehaus.groovy.ast.ASTNode>") Closure ctorBlock) {
if (!argBlock) throw new IllegalArgumentException("nodes of type $name require arguments to be specified")
def oldProps = new ArrayList<>(expression)
diff --git a/src/main/java/groovy/transform/stc/ClosureSignatureHint.java b/src/main/java/groovy/transform/stc/ClosureSignatureHint.java
index ce36a03..9bf986b 100644
--- a/src/main/java/groovy/transform/stc/ClosureSignatureHint.java
+++ b/src/main/java/groovy/transform/stc/ClosureSignatureHint.java
@@ -26,6 +26,7 @@ import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.Parameter;
import org.codehaus.groovy.control.CompilationUnit;
import org.codehaus.groovy.control.SourceUnit;
+import org.codehaus.groovy.control.messages.WarningMessage;
import java.util.List;
@@ -58,7 +59,7 @@ public abstract class ClosureSignatureHint {
* @param gtIndex the index of the generic type to extract
* @return the n-th generic type, or {@link org.codehaus.groovy.ast.ClassHelper#OBJECT_TYPE} if it doesn't exist.
*/
- public static ClassNode pickGenericType(ClassNode type, int gtIndex) {
+ public static ClassNode pickGenericType(final ClassNode type, final int gtIndex) {
final GenericsType[] genericsTypes = type.getGenericsTypes();
if (genericsTypes==null || genericsTypes.length<gtIndex) {
return ClassHelper.OBJECT_TYPE;
@@ -73,7 +74,7 @@ public abstract class ClosureSignatureHint {
* @param gtIndex the index of the generic type to extract
* @return the generic type, or {@link org.codehaus.groovy.ast.ClassHelper#OBJECT_TYPE} if it doesn't exist.
*/
- public static ClassNode pickGenericType(MethodNode node, int parameterIndex, int gtIndex) {
+ public static ClassNode pickGenericType(final MethodNode node, final int parameterIndex, final int gtIndex) {
final Parameter[] parameters = node.getParameters();
final ClassNode type = parameters[parameterIndex].getOriginType();
return pickGenericType(type, gtIndex);
@@ -120,24 +121,46 @@ public abstract class ClosureSignatureHint {
public abstract List<ClassNode[]> getClosureSignatures(MethodNode node, SourceUnit sourceUnit, CompilationUnit compilationUnit, String[] options, ASTNode usage);
/**
- * Finds a class node given a string representing the type. Performs a lookup in the compilation unit to check if it is done in the same source unit.
+ * Produces a {@link ClassNode} given a string representing the type. Checks
+ * the supplied compilation unit in case it is also being compiled.
+ *
* @param sourceUnit source unit
* @param compilationUnit compilation unit
- * @param className the name of the class we want to get a {@link org.codehaus.groovy.ast.ClassNode} for
- * @return a ClassNode representing the type
+ * @param className the type name to resolve
*/
- protected ClassNode findClassNode(final SourceUnit sourceUnit, final CompilationUnit compilationUnit, final String className) {
+ protected ClassNode findClassNode(final SourceUnit sourceUnit, final CompilationUnit compilationUnit, String className) {
if (className.endsWith("[]")) {
return findClassNode(sourceUnit, compilationUnit, className.substring(0, className.length() - 2)).makeArray();
}
+ int i = className.indexOf('<');
+ if (i > 0) {
+ className = className.substring(0, i);
+ String message = getClass().getSimpleName() + " doesn't support generics";
+ sourceUnit.getErrorCollector().addWarning(WarningMessage.LIKELY_ERRORS, message,
+ null, null); // TODO: include reference to the source method & parameter
+ }
+
ClassNode cn = compilationUnit.getClassNode(className);
if (cn == null) {
- try {
- cn = ClassHelper.make(Class.forName(className, false, sourceUnit.getClassLoader()));
- } catch (ClassNotFoundException e) {
- cn = ClassHelper.make(className);
+ cn = ClassHelper.make(className);
+ if (!ClassHelper.isCachedType(cn)) {
+ Class<?> c = tryLoadClass(className, sourceUnit.getClassLoader());
+ if (c != null) {
+ cn = ClassHelper.make(c);
+ }
+ }
+ if (cn.getGenericsTypes() != null) {
+ cn = cn.getPlainNodeReference();
}
}
return cn;
}
+
+ private static Class<?> tryLoadClass(final String className, final ClassLoader classLoader) {
+ try {
+ return Class.forName(className, false, classLoader);
+ } catch (ClassNotFoundException ignore) {
+ return null;
+ }
+ }
}
diff --git a/src/main/java/groovy/transform/stc/SimpleType.java b/src/main/java/groovy/transform/stc/SimpleType.java
index 8866e3b..5d7a1cb 100644
--- a/src/main/java/groovy/transform/stc/SimpleType.java
+++ b/src/main/java/groovy/transform/stc/SimpleType.java
@@ -24,13 +24,12 @@ import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.control.CompilationUnit;
import org.codehaus.groovy.control.SourceUnit;
+import static java.util.Arrays.stream;
+
public class SimpleType extends SingleSignatureClosureHint {
+
@Override
- public ClassNode[] getParameterTypes(final MethodNode node, final String[] options, final SourceUnit sourceUnit, final CompilationUnit unit, final ASTNode usage) {
- ClassNode[] result = new ClassNode[options.length];
- for (int i = 0; i < result.length; i++) {
- result[i] = findClassNode(sourceUnit, unit, options[i]);
- }
- return result;
+ public ClassNode[] getParameterTypes(final MethodNode node, final String[] options, final SourceUnit sourceUnit, final CompilationUnit compilationUnit, final ASTNode usage) {
+ return stream(options).map(option -> findClassNode(sourceUnit, compilationUnit, option)).toArray(ClassNode[]::new);
}
- }
+}
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 0d7fcf8..e42bf45 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -2582,21 +2582,10 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
@Override
protected void visitConstructorOrMethod(final MethodNode node, final boolean isConstructor) {
typeCheckingContext.pushEnclosingMethod(node);
+ readClosureParameterAnnotation(node); // GROOVY-6603
super.visitConstructorOrMethod(node, isConstructor);
if (node.hasDefaultValue()) {
- for (Parameter parameter : node.getParameters()) {
- if (!parameter.hasInitialExpression()) continue;
- // GROOVY-10094: visit param default argument expression
- visitInitialExpression(parameter.getInitialExpression(), varX(parameter), parameter);
- // GROOVY-10104: remove direct target setting to prevent errors
- parameter.getInitialExpression().visit(new CodeVisitorSupport() {
- @Override
- public void visitMethodCallExpression(final MethodCallExpression mce) {
- super.visitMethodCallExpression(mce);
- mce.setMethodTarget(null);
- }
- });
- }
+ visitDefaultParameterArguments(node.getParameters());
}
if (!isConstructor) {
returnAdder.visitMethod(node); // GROOVY-7753: we cannot count these auto-generated return statements, see `typeCheckingContext.pushEnclosingReturnStatement`
@@ -2604,6 +2593,38 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
typeCheckingContext.popEnclosingMethod();
}
+ private void readClosureParameterAnnotation(final MethodNode node) {
+ for (Parameter parameter : node.getParameters()) {
+ for (AnnotationNode annotation : parameter.getAnnotations()) {
+ if (annotation.getClassNode().equals(CLOSUREPARAMS_CLASSNODE)) {
+ // GROOVY-6603: propagate closure parameter types
+ Expression value = annotation.getMember("value");
+ Expression options = annotation.getMember("options");
+ List<ClassNode[]> signatures = getSignaturesFromHint(node, value, options, annotation);
+ if (signatures.size() == 1) { // TODO: handle multiple signatures
+ parameter.putNodeMetaData(CLOSURE_ARGUMENTS, Arrays.stream(signatures.get(0)).map(t -> new Parameter(t,"")).toArray(Parameter[]::new));
+ }
+ }
+ }
+ }
+ }
+
+ private void visitDefaultParameterArguments(final Parameter[] parameters) {
+ for (Parameter parameter : parameters) {
+ if (!parameter.hasInitialExpression()) continue;
+ // GROOVY-10094: visit param default argument expression
+ visitInitialExpression(parameter.getInitialExpression(), varX(parameter), parameter);
+ // GROOVY-10104: remove direct target setting to prevent errors
+ parameter.getInitialExpression().visit(new CodeVisitorSupport() {
+ @Override
+ public void visitMethodCallExpression(final MethodCallExpression mce) {
+ super.visitMethodCallExpression(mce);
+ mce.setMethodTarget(null);
+ }
+ });
+ }
+ }
+
@Override
protected void visitObjectInitializerStatements(final ClassNode node) {
// GROOVY-5450: create fake constructor node so final field analysis can allow write within non-static initializer block(s)
@@ -2883,12 +2904,10 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
List<AnnotationNode> annotations = target.getAnnotations(CLOSUREPARAMS_CLASSNODE);
if (annotations != null && !annotations.isEmpty()) {
for (AnnotationNode annotation : annotations) {
- Expression hintClass = annotation.getMember("value");
- if (hintClass instanceof ClassExpression) {
- Expression options = annotation.getMember("options");
- Expression resolverClass = annotation.getMember("conflictResolutionStrategy");
- doInferClosureParameterTypes(receiver, arguments, expression, method, hintClass, resolverClass, options);
- }
+ Expression value = annotation.getMember("value");
+ Expression options = annotation.getMember("options");
+ Expression conflictResolver = annotation.getMember("conflictResolutionStrategy");
+ doInferClosureParameterTypes(receiver, arguments, expression, method, value, conflictResolver, options);
}
} else if (isSAMType(target.getOriginType())) { // SAM-type coercion
Map<GenericsTypeName, GenericsType> context = extractPlaceHoldersVisibleToDeclaration(receiver, method, arguments);
@@ -2961,23 +2980,21 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
}
}
- private List<ClassNode[]> getSignaturesFromHint(final ClosureExpression expression, final MethodNode selectedMethod, final Expression hintClass, final Expression options) {
- // initialize hints
- List<ClassNode[]> closureSignatures;
+ private List<ClassNode[]> getSignaturesFromHint(final MethodNode mn, final Expression hintType, final Expression options, final ASTNode usage) {
+ String hintTypeName = hintType.getText(); // load hint class using compiler's classloader
try {
- ClassLoader transformLoader = getTransformLoader();
@SuppressWarnings("unchecked")
- Class<? extends ClosureSignatureHint> hint = (Class<? extends ClosureSignatureHint>) transformLoader.loadClass(hintClass.getText());
- ClosureSignatureHint hintInstance = hint.getDeclaredConstructor().newInstance();
- closureSignatures = hintInstance.getClosureSignatures(
- selectedMethod instanceof ExtensionMethodNode ? ((ExtensionMethodNode) selectedMethod).getExtensionMethodNode() : selectedMethod,
+ Class<? extends ClosureSignatureHint> hintClass = (Class<? extends ClosureSignatureHint>) getTransformLoader().loadClass(hintTypeName);
+ List<ClassNode[]> closureSignatures = hintClass.getDeclaredConstructor().newInstance().getClosureSignatures(
+ mn instanceof ExtensionMethodNode ? ((ExtensionMethodNode) mn).getExtensionMethodNode() : mn,
typeCheckingContext.getSource(),
typeCheckingContext.getCompilationUnit(),
- convertToStringArray(options), expression);
- } catch (ClassNotFoundException | IllegalAccessException | InstantiationException | NoSuchMethodException | InvocationTargetException e) {
+ convertToStringArray(options),
+ usage);
+ return closureSignatures;
+ } catch (ReflectiveOperationException e) {
throw new GroovyBugError(e);
}
- return closureSignatures;
}
private List<ClassNode[]> resolveWithResolver(final List<ClassNode[]> candidates, final ClassNode receiver, final Expression arguments, final ClosureExpression expression, final MethodNode selectedMethod, final Expression resolverClass, final Expression options) {
@@ -3009,7 +3026,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
Parameter[] closureParams = expression.getParameters();
if (closureParams == null) return; // no-arg closure
- List<ClassNode[]> closureSignatures = getSignaturesFromHint(expression, selectedMethod, hintClass, options);
+ List<ClassNode[]> closureSignatures = getSignaturesFromHint(selectedMethod, hintClass, options, expression);
List<ClassNode[]> candidates = new LinkedList<>();
for (ClassNode[] signature : closureSignatures) {
// in order to compute the inferred types of the closure parameters, we're using the following trick:
@@ -3356,7 +3373,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
if (parameters != null) {
typeCheckClosureCall(callArguments, args, parameters);
}
- ClassNode type = getType(((ASTNode) variable));
+ ClassNode type = getType((ASTNode) variable);
if (type.equals(CLOSURE_TYPE)) { // GROOVY-10098, et al.
GenericsType[] genericsTypes = type.getGenericsTypes();
if (genericsTypes != null && genericsTypes.length == 1
diff --git a/src/test/groovy/transform/stc/ClosureParamTypeInferenceSTCTest.groovy b/src/test/groovy/transform/stc/ClosureParamTypeInferenceSTCTest.groovy
index bf2e779..8d2039b 100644
--- a/src/test/groovy/transform/stc/ClosureParamTypeInferenceSTCTest.groovy
+++ b/src/test/groovy/transform/stc/ClosureParamTypeInferenceSTCTest.groovy
@@ -1223,25 +1223,34 @@ assert result == ['b', 'r', 'e', 'a', 'd', 'b', 'u', 't', 't', 'e', 'r']
}
void testGroovy6602() {
- shouldFailWithMessages '''import groovy.transform.stc.FromString
- void foo(@ClosureParams(value=FromString, options="java.lang.Number") Closure cl) {
- cl.call(4.5)
+ shouldFailWithMessages '''import groovy.transform.stc.SimpleType
+ void foo(@ClosureParams(value=SimpleType, options="java.lang.Number") Closure c) {
+ c.call(4.5)
}
foo { Integer i -> println i }
''',
'Expected type java.lang.Number for closure parameter: i'
}
+ void testGroovy6603() {
+ shouldFailWithMessages '''import groovy.transform.stc.SimpleType
+ void foo(@ClosureParams(value=SimpleType, options="java.lang.Number") Closure c) {
+ c.call("x")
+ }
+ ''',
+ 'Cannot call closure that accepts [java.lang.Number] with [java.lang.String]'
+ }
+
void testGroovy6729() {
assertScript '''import groovy.transform.stc.FirstParam
- static <T> List<T> callee01(List<T>self, @ClosureParams(FirstParam.FirstGenericType) Closure c) {
- self.each {
+ static <T> List<T> foo(List<T> list, @ClosureParams(FirstParam.FirstGenericType) Closure c) {
+ list.each {
c.call(it)
}
- return self
+ return list
}
- callee01(["a","b","c"]) { a ->
- println(a.toUpperCase()) // [Static type checking] - Cannot find matching method java.lang.Object#toUpperCase(). Please check if the declared type is correct and if the method exists.
+ foo(["a","b","c"]) { s ->
+ s.toUpperCase() // Cannot find matching method java.lang.Object#toUpperCase()
}
'''
}
@@ -1304,11 +1313,9 @@ assert result == ['b', 'r', 'e', 'a', 'd', 'b', 'u', 't', 't', 'e', 'r']
}
void testGroovy9518b() {
- assertScript '''
- import groovy.transform.stc.SimpleType
-
+ assertScript '''import groovy.transform.stc.FromString
class C {
- C(String s, @ClosureParams(value=SimpleType, options='java.util.List') Closure<Integer> c) {
+ C(String s, @ClosureParams(value=FromString,options="java.util.List<java.lang.Integer>") Closure<Integer> c) {
}
}
diff --git a/subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java b/subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java
index 5186bef..92c602b 100644
--- a/subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java
+++ b/subprojects/groovy-sql/src/main/java/groovy/sql/Sql.java
@@ -24,6 +24,7 @@ import groovy.lang.MissingPropertyException;
import groovy.lang.Tuple;
import groovy.transform.NamedParam;
import groovy.transform.stc.ClosureParams;
+import groovy.transform.stc.FromString;
import groovy.transform.stc.SimpleType;
import org.codehaus.groovy.runtime.InvokerHelper;
import org.codehaus.groovy.vmplugin.VMPluginFactory;
@@ -2398,7 +2399,7 @@ public class Sql implements AutoCloseable {
* @throws SQLException if a database access error occurs
* @since 2.3.2
*/
- public void execute(String sql, @ClosureParams(value=SimpleType.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
+ public void execute(String sql, @ClosureParams(value=FromString.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
Connection connection = createConnection();
Statement statement = null;
try {
@@ -2485,7 +2486,7 @@ public class Sql implements AutoCloseable {
* @see #execute(String, Closure)
* @since 2.3.2
*/
- public void execute(String sql, List<Object> params, @ClosureParams(value=SimpleType.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
+ public void execute(String sql, List<Object> params, @ClosureParams(value=FromString.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
Connection connection = createConnection();
PreparedStatement statement = null;
try {
@@ -2542,7 +2543,7 @@ public class Sql implements AutoCloseable {
* @throws SQLException if a database access error occurs
* @since 2.3.2
*/
- public void execute(Map params, String sql, @ClosureParams(value=SimpleType.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
+ public void execute(Map params, String sql, @ClosureParams(value=FromString.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
execute(sql, singletonList(params), processResults);
}
@@ -2582,7 +2583,7 @@ public class Sql implements AutoCloseable {
* @see #execute(String, List, Closure)
* @since 2.3.2
*/
- public void execute(String sql, Object[] params, @ClosureParams(value=SimpleType.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
+ public void execute(String sql, Object[] params, @ClosureParams(value=FromString.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
execute(sql, Arrays.asList(params), processResults);
}
@@ -2628,7 +2629,7 @@ public class Sql implements AutoCloseable {
* @see #execute(String, List, Closure)
* @since 2.3.2
*/
- public void execute(GString gstring, @ClosureParams(value=SimpleType.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
+ public void execute(GString gstring, @ClosureParams(value=FromString.class, options={"boolean,java.util.List<groovy.sql.GroovyRowResult>", "boolean,int"}) Closure processResults) throws SQLException {
List<Object> params = getParameters(gstring);
String sql = asSql(gstring, params);
execute(sql, params, processResults);