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/04/20 16:33:56 UTC
[groovy] branch master updated: GROOVY-10033: check generics usage
for constructors, coercions and casts
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 09b3833 GROOVY-10033: check generics usage for constructors, coercions and casts
09b3833 is described below
commit 09b38330f010c011c12f27ab6fdff7ebb6bdb1d1
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Tue Apr 20 10:58:04 2021 -0500
GROOVY-10033: check generics usage for constructors, coercions and casts
---
.../codehaus/groovy/control/GenericsVisitor.java | 206 ++++++++++-----------
src/test/gls/generics/GenericsUsageTest.groovy | 100 ++++++----
src/test/groovy/bugs/Groovy3731Bug.groovy | 44 -----
3 files changed, 166 insertions(+), 184 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/control/GenericsVisitor.java b/src/main/java/org/codehaus/groovy/control/GenericsVisitor.java
index 4fbc10d..69356f2 100644
--- a/src/main/java/org/codehaus/groovy/control/GenericsVisitor.java
+++ b/src/main/java/org/codehaus/groovy/control/GenericsVisitor.java
@@ -25,10 +25,11 @@ import org.codehaus.groovy.ast.GenericsType;
import org.codehaus.groovy.ast.InnerClassNode;
import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.Parameter;
+import org.codehaus.groovy.ast.expr.ArrayExpression;
+import org.codehaus.groovy.ast.expr.CastExpression;
import org.codehaus.groovy.ast.expr.ConstructorCallExpression;
import org.codehaus.groovy.ast.expr.DeclarationExpression;
import org.codehaus.groovy.ast.expr.Expression;
-import org.codehaus.groovy.ast.expr.TupleExpression;
import org.codehaus.groovy.transform.trait.Traits;
import static org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.isUnboundedWildcard;
@@ -43,143 +44,163 @@ import static org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.isUnbo
* </ul>
*/
public class GenericsVisitor extends ClassCodeVisitorSupport {
- private final SourceUnit source;
- public GenericsVisitor(SourceUnit source) {
- this.source = source;
- }
+ private final SourceUnit source;
@Override
protected SourceUnit getSourceUnit() {
return source;
}
+ public GenericsVisitor(final SourceUnit source) {
+ this.source = source;
+ }
+
+ //--------------------------------------------------------------------------
+
@Override
- public void visitClass(ClassNode node) {
- boolean error = checkWildcard(node);
- if (error) return;
- boolean isAnon = node instanceof InnerClassNode && ((InnerClassNode) node).isAnonymous();
- checkGenericsUsage(node.getUnresolvedSuperClass(false), node.getSuperClass(), isAnon ? true : null);
- ClassNode[] interfaces = node.getInterfaces();
- for (ClassNode anInterface : interfaces) {
- checkGenericsUsage(anInterface, anInterface.redirect());
+ public void visitClass(final ClassNode node) {
+ ClassNode sn = node.getUnresolvedSuperClass(false);
+ if (checkWildcard(sn)) return;
+
+ boolean isAIC = node instanceof InnerClassNode && ((InnerClassNode) node).isAnonymous();
+ checkGenericsUsage(sn, node.getSuperClass(), isAIC ? Boolean.TRUE : null);
+ for (ClassNode face : node.getInterfaces()) {
+ checkGenericsUsage(face);
}
+
node.visitContents(this);
}
@Override
- public void visitField(FieldNode node) {
- ClassNode type = node.getType();
- checkGenericsUsage(type, type.redirect());
+ public void visitField(final FieldNode node) {
+ checkGenericsUsage(node.getType());
+
super.visitField(node);
}
@Override
- public void visitConstructorCallExpression(ConstructorCallExpression call) {
- ClassNode type = call.getType();
- boolean isAnon = type instanceof InnerClassNode && ((InnerClassNode) type).isAnonymous();
- checkGenericsUsage(type, type.redirect(), isAnon);
+ protected void visitConstructorOrMethod(final MethodNode node, final boolean isConstructor) {
+ for (Parameter p : node.getParameters()) {
+ checkGenericsUsage(p.getType());
+ }
+ if (!isConstructor) {
+ checkGenericsUsage(node.getReturnType());
+ }
+
+ super.visitConstructorOrMethod(node, isConstructor);
}
@Override
- public void visitMethod(MethodNode node) {
- Parameter[] parameters = node.getParameters();
- for (Parameter param : parameters) {
- ClassNode paramType = param.getType();
- checkGenericsUsage(paramType, paramType.redirect());
- }
- ClassNode returnType = node.getReturnType();
- checkGenericsUsage(returnType, returnType.redirect());
- super.visitMethod(node);
+ public void visitConstructorCallExpression(final ConstructorCallExpression expression) {
+ ClassNode type = expression.getType();
+ boolean isAIC = type instanceof InnerClassNode
+ && ((InnerClassNode) type).isAnonymous();
+ checkGenericsUsage(type, type.redirect(), isAIC);
+
+ super.visitConstructorCallExpression(expression);
}
@Override
- public void visitDeclarationExpression(DeclarationExpression expression) {
+ public void visitDeclarationExpression(final DeclarationExpression expression) {
if (expression.isMultipleAssignmentDeclaration()) {
- TupleExpression tExpr = expression.getTupleExpression();
- for (Expression nextExpr : tExpr.getExpressions()) {
- ClassNode declType = nextExpr.getType();
- checkGenericsUsage(declType, declType.redirect());
+ for (Expression e : expression.getTupleExpression().getExpressions()) {
+ checkGenericsUsage(e.getType());
}
} else {
- ClassNode declType = expression.getVariableExpression().getType();
- checkGenericsUsage(declType, declType.redirect());
+ checkGenericsUsage(expression.getVariableExpression().getType());
}
+
super.visitDeclarationExpression(expression);
}
- private boolean checkWildcard(ClassNode cn) {
- ClassNode sn = cn.getUnresolvedSuperClass(false);
- if (sn == null) return false;
- GenericsType[] generics = sn.getGenericsTypes();
- if (generics == null) return false;
- boolean error = false;
- for (GenericsType generic : generics) {
- if (generic.isWildcard()) {
- addError("A supertype may not specify a wildcard type", sn);
- error = true;
+ @Override
+ public void visitArrayExpression(final ArrayExpression expression) {
+ checkGenericsUsage(expression.getType());
+
+ super.visitArrayExpression(expression);
+ }
+
+ @Override
+ public void visitCastExpression(final CastExpression expression) {
+ checkGenericsUsage(expression.getType());
+
+ super.visitCastExpression(expression);
+ }
+
+ //--------------------------------------------------------------------------
+
+ private boolean checkWildcard(final ClassNode sn) {
+ boolean wildcard = false;
+ if (sn.getGenericsTypes() != null) {
+ for (GenericsType gt : sn.getGenericsTypes()) {
+ if (gt.isWildcard()) {
+ addError("A supertype may not specify a wildcard type", sn);
+ wildcard = true;
+ }
}
}
- return error;
+ return wildcard;
}
- private void checkGenericsUsage(ClassNode n, ClassNode cn) {
- checkGenericsUsage(n, cn, null);
+ private void checkGenericsUsage(ClassNode cn) {
+ while (cn.isArray())
+ cn = cn.getComponentType();
+ checkGenericsUsage(cn, cn.redirect(), null);
}
- private void checkGenericsUsage(ClassNode n, ClassNode cn, Boolean isAnonInnerClass) {
- if (n.isGenericsPlaceHolder()) return;
- GenericsType[] nTypes = n.getGenericsTypes();
+ private void checkGenericsUsage(final ClassNode cn, final ClassNode rn, final Boolean isAIC) {
+ if (cn.isGenericsPlaceHolder()) return;
GenericsType[] cnTypes = cn.getGenericsTypes();
+ GenericsType[] rnTypes = rn.getGenericsTypes();
// raw type usage is always allowed
- if (nTypes == null) return;
+ if (cnTypes == null) return;
// you can't parameterize a non-generified type
- if (cnTypes == null) {
- String message = "The class " + getPrintName(n) + " (supplied with " + plural("type parameter", nTypes.length) +
- ") refers to the class " + getPrintName(cn) + " which takes no parameters";
- if (nTypes.length == 0) {
+ if (rnTypes == null) {
+ String message = "The class " + cn.toString(false) + " (supplied with " + plural("type parameter", cnTypes.length) +
+ ") refers to the class " + rn.toString(false) + " which takes no parameters";
+ if (cnTypes.length == 0) {
message += " (invalid Diamond <> usage?)";
}
- addError(message, n);
+ addError(message, cn);
return;
}
// parameterize a type by using all of the parameters only
- if (nTypes.length != cnTypes.length) {
- if (Boolean.FALSE.equals(isAnonInnerClass) && nTypes.length == 0) {
+ if (cnTypes.length != rnTypes.length) {
+ if (Boolean.FALSE.equals(isAIC) && cnTypes.length == 0) {
return; // allow Diamond for non-AIC cases from CCE
}
String message;
- if (Boolean.TRUE.equals(isAnonInnerClass) && nTypes.length == 0) {
+ if (Boolean.TRUE.equals(isAIC) && cnTypes.length == 0) {
message = "Cannot use diamond <> with anonymous inner classes";
} else {
- message = "The class " + getPrintName(n) + " (supplied with " + plural("type parameter", nTypes.length) +
- ") refers to the class " + getPrintName(cn) +
- " which takes " + plural("parameter", cnTypes.length);
- if (nTypes.length == 0) {
+ message = "The class " + cn.toString(false) + " (supplied with " + plural("type parameter", cnTypes.length) +
+ ") refers to the class " + rn.toString(false) + " which takes " + plural("parameter", rnTypes.length);
+ if (cnTypes.length == 0) {
message += " (invalid Diamond <> usage?)";
}
}
- addError(message, n);
+ addError(message, cn);
return;
}
- for (int i = 0; i < nTypes.length; i++) {
- ClassNode nType = nTypes[i].getType();
+ for (int i = 0; i < cnTypes.length; i++) {
ClassNode cnType = cnTypes[i].getType();
+ ClassNode rnType = rnTypes[i].getType();
// check nested type parameters
- checkGenericsUsage(nType, nType.redirect());
+ checkGenericsUsage(cnType);
// check bounds: unbounded wildcard (aka "?") is universal substitute
- if (!isUnboundedWildcard(nTypes[i])) {
+ if (!isUnboundedWildcard(cnTypes[i])) {
// check upper bound(s)
- ClassNode[] bounds = cnTypes[i].getUpperBounds();
+ ClassNode[] bounds = rnTypes[i].getUpperBounds();
// first can be class or interface
- boolean valid = nType.isDerivedFrom(cnType) || ((cnType.isInterface() || Traits.isTrait(cnType)) && nType.implementsInterface(cnType));
+ boolean valid = cnType.isDerivedFrom(rnType) || ((rnType.isInterface() || Traits.isTrait(rnType)) && cnType.implementsInterface(rnType));
// subsequent bounds if present can be interfaces
if (valid && bounds != null && bounds.length > 1) {
for (int j = 1; j < bounds.length; j++) {
ClassNode bound = bounds[j];
- if (!nType.implementsInterface(bound)) {
+ if (!cnType.implementsInterface(bound)) {
valid = false;
break;
}
@@ -187,46 +208,13 @@ public class GenericsVisitor extends ClassCodeVisitorSupport {
}
if (!valid) {
- addError("The type " + nTypes[i].getName() + " is not a valid substitute for the bounded parameter <" +
- getPrintName(cnTypes[i]) + ">", nTypes[i]);
+ addError("The type " + cnTypes[i].getName() + " is not a valid substitute for the bounded parameter <" + rnTypes[i] + ">", cnTypes[i]);
}
}
}
}
- private String plural(String orig, int count) {
- return "" + count + " " + (count == 1 ? orig : orig + "s");
- }
-
- private static String getPrintName(GenericsType gt) {
- StringBuilder ret = new StringBuilder(gt.getName());
- ClassNode[] upperBounds = gt.getUpperBounds();
- ClassNode lowerBound = gt.getLowerBound();
- if (upperBounds != null) {
- if (upperBounds.length != 1 || !"java.lang.Object".equals(getPrintName(upperBounds[0]))) {
- ret.append(" extends ");
- for (int i = 0; i < upperBounds.length; i++) {
- ret.append(getPrintName(upperBounds[i]));
- if (i + 1 < upperBounds.length) ret.append(" & ");
- }
- }
- } else if (lowerBound != null) {
- ret.append(" super ").append(getPrintName(lowerBound));
- }
- return ret.toString();
- }
-
- private static String getPrintName(ClassNode cn) {
- StringBuilder ret = new StringBuilder(cn.getName());
- GenericsType[] gts = cn.getGenericsTypes();
- if (gts != null) {
- ret.append("<");
- for (int i = 0; i < gts.length; i++) {
- if (i != 0) ret.append(",");
- ret.append(getPrintName(gts[i]));
- }
- ret.append(">");
- }
- return ret.toString();
+ private static String plural(final String string, final int count) {
+ return "" + count + " " + (count == 1 ? string : string + "s");
}
}
diff --git a/src/test/gls/generics/GenericsUsageTest.groovy b/src/test/gls/generics/GenericsUsageTest.groovy
index b8f3e27..5b8ec7d 100644
--- a/src/test/gls/generics/GenericsUsageTest.groovy
+++ b/src/test/gls/generics/GenericsUsageTest.groovy
@@ -241,67 +241,105 @@ final class GenericsUsageTest extends CompilableTestSupport {
'''
}
- // GROOVY-7865
- void testFriendlyErrorMessageForGenericsArityErrors() {
+ // GROOVY-3731, GROOVY-7865, GROOVY-10033
+ void testFriendlyErrorMessageForGenericsErrors() {
+ // superclass and interfaces
shouldFailCompilationWithMessages '''
- class MyList extends ArrayList<String, String> {}
+ class C extends ArrayList<> { }
+ ''', ["Unexpected input: '<'"]
+ shouldFailCompilationWithMessages '''
+ class C extends ArrayList<? extends Number> { }
+ ''', ['A supertype may not specify a wildcard type']
+ shouldFailCompilationWithMessages '''
+ class C extends ArrayList<String, String> { }
''', ['(supplied with 2 type parameters)', 'which takes 1 parameter']
-
shouldFailCompilationWithMessages '''
- class MyList extends ArrayList<> {}
- ''', ['Unexpected input: \'<\'']
-
+ class C extends HashMap<String> { }
+ ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
shouldFailCompilationWithMessages '''
- class MyMap extends HashMap<String> {}
+ class C implements Map<> { }
+ ''', ["Unexpected input: '<'"]
+ shouldFailCompilationWithMessages '''
+ class MyMap implements Map<String> { }
''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
shouldFailCompilationWithMessages '''
- class MyList implements List<String, String> {}
+ class C implements List<String, String> { }
''', ['(supplied with 2 type parameters)', 'which takes 1 parameter']
+ // constructor call
+ assertScript '''
+ List<String> list = new LinkedList<>()
+ '''
+ shouldFailCompilationWithMessage '''
+ new LinkedList<>() { }
+ ''', 'Cannot use diamond <> with anonymous inner classes'
shouldFailCompilationWithMessages '''
- class MyList implements Map<> {}
- ''', ['Unexpected input: \'<\'']
-
-
+ new LinkedList<Integer, String>()
+ ''', ['(supplied with 2 type parameters)', 'which takes 1 parameter']
shouldFailCompilationWithMessages '''
- class MyMap implements Map<String> {}
- ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
+ new LinkedList<String, String>()
+ ''', ['(supplied with 2 type parameters)', 'which takes 1 parameter']
shouldFailCompilationWithMessages '''
- List<String> ss = new LinkedList<Integer, String>()
+ new LinkedList<String, String>() { }
''', ['(supplied with 2 type parameters)', 'which takes 1 parameter']
- shouldFailCompilationWithMessage '''
- List<String> ss = new LinkedList<>(){}
- ''', 'Cannot use diamond <> with anonymous inner classes'
shouldFailCompilationWithMessages '''
- List<String> ss = new LinkedList<String, String>(){}
+ new Date<Calendar>()
+ ''', ['(supplied with 1 type parameter)', 'which takes no parameters']
+
+ // constructor declaration
+ shouldFailCompilationWithMessages '''
+ class C { C(Map<String> m) { } }
+ ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
+ shouldFailCompilationWithMessages '''
+ class C { C(Closure<String,Number> c) { } }
''', ['(supplied with 2 type parameters)', 'which takes 1 parameter']
+
+ // method declaration
shouldFailCompilationWithMessages '''
- List<String> ss = new LinkedList<String, String>()
- ''', ['supplied with 2 type parameters', 'which takes 1 parameter']
+ def method(Map<String> map) { }
+ ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
shouldFailCompilationWithMessages '''
- def now = new Date<Calendar>()
- ''', ['supplied with 1 type parameter', 'which takes no parameters']
+ Map<String> method() { }
+ ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
shouldFailCompilationWithMessages '''
- def method(Map<String> map) { map.toString() }
+ def method(Map<String, Map<String>> map) { }
''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
shouldFailCompilationWithMessages '''
- def method(Map<String, Map<String>> map) { map.toString() }
+ def method(Map<String, Map<String>> map) { }
''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
+
+ // field declaration
shouldFailCompilationWithMessages '''
- class MyClass { Map<String> map }
+ class C { Map<String> map }
''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
shouldFailCompilationWithMessages '''
- class MyClass { Map<String, Map<String>> map }
+ class C { Map<String, Map<String>> map }
''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
+
+ // variable declaration
shouldFailCompilationWithMessages '''
def method() { Map<String> map }
''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
shouldFailCompilationWithMessages '''
def method() { Map<String, Map<String>> map }
''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
- assertScript '''
- List<String> ss = new LinkedList<>()
- '''
+ shouldFailCompilationWithMessages '''
+ def (Map<String,String> one, Map<String> two) = [ [:], [:] ]
+ ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
+ shouldFailCompilationWithMessages '''
+ Map<String>[][] array = new Map[0][0]
+ ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
+ shouldFailCompilationWithMessages '''
+ Map<String,String>[][] array = new Map<String>[0][]
+ ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
+
+ // casting and coercion
+ shouldFailCompilationWithMessages '''
+ def map = (Map<String>) null
+ ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
+ shouldFailCompilationWithMessages '''
+ def map = null as Map<String>
+ ''', ['(supplied with 1 type parameter)', 'which takes 2 parameters']
}
// GROOVY-8990
diff --git a/src/test/groovy/bugs/Groovy3731Bug.groovy b/src/test/groovy/bugs/Groovy3731Bug.groovy
deleted file mode 100644
index 3ba1957..0000000
--- a/src/test/groovy/bugs/Groovy3731Bug.groovy
+++ /dev/null
@@ -1,44 +0,0 @@
-/*
- * 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 groovy.bugs
-
-import gls.CompilableTestSupport
-
-class Groovy3731Bug extends CompilableTestSupport {
-
- void testWrongGenericsUseInFieldsAndMethods() {
- shouldNotCompile """
- public class G3731A {
- Map<Object> m = [:]
- }
- """
-
- shouldNotCompile """
- public class G3731B {
- void m1(x, Map<Object> y) {}
- }
- """
-
- shouldNotCompile """
- public class G3731C {
- Map<Object> m1(x, y) {null}
- }
- """
- }
-}
\ No newline at end of file