diff --git a/src/main/java/org/codehaus/groovy/ast/ClassNode.java b/src/main/java/org/codehaus/groovy/ast/ClassNode.java index 84467c50210..65c22efbae4 100644 --- a/src/main/java/org/codehaus/groovy/ast/ClassNode.java +++ b/src/main/java/org/codehaus/groovy/ast/ClassNode.java @@ -904,12 +904,15 @@ public boolean isDerivedFrom(ClassNode type) { if (type.equals(ClassHelper.OBJECT_TYPE)) { return true; } - ClassNode node = this; - while (node != null) { + if (this.isArray() && type.isArray() + && type.getComponentType().equals(ClassHelper.OBJECT_TYPE) + && !ClassHelper.isPrimitiveType(this.getComponentType())){ + return true; + } + for (ClassNode node = this; node != null; node = node.getSuperClass()) { if (type.equals(node)) { return true; } - node = node.getSuperClass(); } return false; } diff --git a/src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java b/src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java index 532efaa81ff..5690bb675f6 100644 --- a/src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java +++ b/src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java @@ -1310,12 +1310,18 @@ public static List unique(List self) { * @since 1.8.1 */ public static Collection unique(Collection self, boolean mutate) { - List answer = uniqueItems(self); + Collection answer = null; + if (mutate || (self != null && self.size() > 1)) { + answer = uniqueItems(self); + } if (mutate) { self.clear(); self.addAll(answer); + + return self; + } else { + return answer; } - return mutate ? self : answer ; } private static List uniqueItems(Iterable self) { 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 9cf9c36b868..04a7a4be28d 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -194,6 +194,7 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.binX; import static org.codehaus.groovy.ast.tools.GeneralUtils.callX; import static org.codehaus.groovy.ast.tools.GeneralUtils.castX; +import static org.codehaus.groovy.ast.tools.GeneralUtils.classX; import static org.codehaus.groovy.ast.tools.GeneralUtils.constX; import static org.codehaus.groovy.ast.tools.GeneralUtils.elvisX; import static org.codehaus.groovy.ast.tools.GeneralUtils.getGetterName; @@ -1131,18 +1132,6 @@ private void adjustGenerics(final ClassNode source, final ClassNode target) { target.setGenericsTypes(genericsTypes); } - /** - * Stores information about types when [objectOfInstanceof instanceof typeExpression] is visited. - * - * @param objectOfInstanceOf the expression which must be checked against instanceof - * @param typeExpression the expression which represents the target type - */ - protected void pushInstanceOfTypeInfo(final Expression objectOfInstanceOf, final Expression typeExpression) { - List potentialTypes = typeCheckingContext.temporaryIfBranchTypeInformation.peek() - .computeIfAbsent(extractTemporaryTypeInfoKey(objectOfInstanceOf), key -> new LinkedList<>()); - potentialTypes.add(typeExpression.getType()); - } - private boolean typeCheckMultipleAssignmentAndContinue(final Expression leftExpression, Expression rightExpression) { // multiple assignment check if (!(leftExpression instanceof TupleExpression)) return true; @@ -1441,37 +1430,6 @@ protected MethodNode checkGroovyStyleConstructor(final ClassNode node, final Cla return constructors.get(0); } - /** - * When instanceof checks are found in the code, we store temporary type - * information data in the {@link TypeCheckingContext#temporaryIfBranchTypeInformation} - * table. This method computes the key which must be used to store this type - * info. - * - * @param expression the expression for which to compute the key - * @return a key to be used for {@link TypeCheckingContext#temporaryIfBranchTypeInformation} - */ - protected Object extractTemporaryTypeInfoKey(final Expression expression) { - return expression instanceof VariableExpression ? findTargetVariable((VariableExpression) expression) : expression.getText(); - } - - /** - * A helper method which determines which receiver class should be used in error messages when a field or attribute - * is not found. The returned type class depends on whether we have temporary type information available (due to - * instanceof checks) and whether there is a single candidate in that case. - * - * @param expr the expression for which an unknown field has been found - * @param type the type of the expression (used as fallback type) - * @return if temporary information is available and there's only one type, returns the temporary type class - * otherwise falls back to the provided type class. - */ - protected ClassNode findCurrentInstanceOfClass(final Expression expr, final ClassNode type) { - if (!typeCheckingContext.temporaryIfBranchTypeInformation.isEmpty()) { - List nodes = getTemporaryTypesForExpression(expr); - if (nodes != null && nodes.size() == 1) return nodes.get(0); - } - return type; - } - protected boolean existsProperty(final PropertyExpression pexp, final boolean checkForReadOnly) { return existsProperty(pexp, checkForReadOnly, null); } @@ -2249,7 +2207,14 @@ public void visitMethodCallExpression(final MethodCallExpression mce) { public void visitExpressionStatement(final ExpressionStatement statement) { typeCheckingContext.pushTemporaryTypeInfo(); super.visitExpressionStatement(statement); - typeCheckingContext.popTemporaryTypeInfo(); + Map> tti = typeCheckingContext.temporaryIfBranchTypeInformation.pop(); + if (!tti.isEmpty() && !typeCheckingContext.temporaryIfBranchTypeInformation.isEmpty()) { + tti.forEach((k, tempTypes) -> { + if (tempTypes.contains(VOID_TYPE)) + typeCheckingContext.temporaryIfBranchTypeInformation.peek() + .computeIfAbsent(k, x -> new LinkedList<>()).add(VOID_TYPE); + }); + } } @Override @@ -2405,29 +2370,6 @@ protected ClassNode[] getArgumentTypes(final ArgumentListExpression args) { ).toArray(ClassNode[]::new); } - private ClassNode getInferredTypeFromTempInfo(final Expression expression, final ClassNode expressionType) { - if (expression instanceof VariableExpression && !typeCheckingContext.temporaryIfBranchTypeInformation.isEmpty()) { - List tempTypes = getTemporaryTypesForExpression(expression); - if (tempTypes != null && !tempTypes.isEmpty()) { - List types = new ArrayList<>(tempTypes.size() + 1); - if (expressionType != null && !expressionType.equals(OBJECT_TYPE) // GROOVY-7333 - && tempTypes.stream().noneMatch(t -> implementsInterfaceOrIsSubclassOf(t, expressionType))) { // GROOVY-9769 - types.add(expressionType); - } - types.addAll(tempTypes); - - if (types.isEmpty()) { - return OBJECT_TYPE; - } else if (types.size() == 1) { - return types.get(0); - } else { - return new UnionTypeClassNode(types.toArray(ClassNode.EMPTY_ARRAY)); - } - } - } - return expressionType; - } - @Override public void visitClosureExpression(final ClosureExpression expression) { // collect every variable expression used in the closure body @@ -3925,19 +3867,6 @@ protected void checkForbiddenSpreadArgument(final ArgumentListExpression argumen } } - protected List getTemporaryTypesForExpression(final Expression objectExpression) { - List classNodes = null; - int depth = typeCheckingContext.temporaryIfBranchTypeInformation.size(); - while (classNodes == null && depth > 0) { - Map> tempo = typeCheckingContext.temporaryIfBranchTypeInformation.get(--depth); - Object key = objectExpression instanceof ParameterVariableExpression - ? ((ParameterVariableExpression) objectExpression).parameter - : extractTemporaryTypeInfoKey(objectExpression); - classNodes = tempo.get(key); - } - return classNodes; - } - protected void storeTargetMethod(final Expression call, final MethodNode directMethodCallCandidate) { call.putNodeMetaData(DIRECT_METHOD_CALL_TARGET, directMethodCallCandidate); @@ -4495,13 +4424,9 @@ protected void storeType(final Expression exp, ClassNode cn) { List assignedTypes = typeCheckingContext.closureSharedVariablesAssignmentTypes.computeIfAbsent(var, k -> new LinkedList<>()); assignedTypes.add(cn); } - if (!typeCheckingContext.temporaryIfBranchTypeInformation.isEmpty()) { - List temporaryTypesForExpression = getTemporaryTypesForExpression(var); - if (temporaryTypesForExpression != null && !temporaryTypesForExpression.isEmpty()) { - // a type inference has been made on a variable whose type was defined in an instanceof block - // erase available information with the new type - temporaryTypesForExpression.clear(); - } + if (!var.isThisExpression() && !var.isSuperExpression() && !typeCheckingContext.temporaryIfBranchTypeInformation.isEmpty()) { + // GROOVY-5226, GROOVY-11290: assignment voids instanceof + pushInstanceOfTypeInfo(var, classX(VOID_TYPE)); } } } @@ -5909,8 +5834,8 @@ protected void addCategoryMethodCallError(final Expression call) { addStaticTypeError("Due to their dynamic nature, usage of categories is not possible with static type checking active", call); } - protected void addAssignmentError(final ClassNode leftType, final ClassNode rightType, final Expression assignmentExpression) { - addStaticTypeError("Cannot assign value of type " + rightType.toString(false) + " to variable of type " + leftType.toString(false), assignmentExpression); + protected void addAssignmentError(final ClassNode leftType, final ClassNode rightType, final Expression expression) { + addStaticTypeError("Cannot assign value of type " + prettyPrintType(rightType) + " to variable of type " + prettyPrintType(leftType), expression); } protected void addUnsupportedPreOrPostfixExpressionError(final Expression expression) { @@ -5946,7 +5871,7 @@ public void performSecondPass() { addStaticTypeError("A closure shared variable [" + target.getName() + "] has been assigned with various types and the method" + " [" + toMethodParametersString(message, getType(((BinaryExpression) expression).getRightExpression())) + "]" + " does not exist in the lowest upper bound of those types: [" + - lub.toString(false) + "]. In general, this is a bad practice (variable reuse) because the compiler cannot" + + prettyPrintType(lub) + "]. In general, this is a bad practice (variable reuse) because the compiler cannot" + " determine safely what is the type of the variable at the moment of the call in a multithreaded context.", expression); } } @@ -5974,7 +5899,7 @@ public void performSecondPass() { addStaticTypeError("A closure shared variable [" + target.getName() + "] has been assigned with various types and the method" + " [" + toMethodParametersString(methodNode.getName(), params) + "]" + " does not exist in the lowest upper bound of those types: [" + - lub.toString(false) + "]. In general, this is a bad practice (variable reuse) because the compiler cannot" + + prettyPrintType(lub) + "]. In general, this is a bad practice (variable reuse) because the compiler cannot" + " determine safely what is the type of the variable at the moment of the call in a multithreaded context.", call); } } @@ -6025,15 +5950,115 @@ private static BinaryExpression assignX(final Expression lhs, final Expression r return exp; } + //-------------------------------------------------------------------------- + // temporaryIfBranchTypeInformation support; migrate to TypeCheckingContext? + + /** + * Stores information about types when [objectOfInstanceof instanceof typeExpression] is visited. + * + * @param objectOfInstanceOf the expression to be checked against instanceof + * @param typeExpression the expression which represents the target type + */ + protected void pushInstanceOfTypeInfo(final Expression objectOfInstanceOf, final Expression typeExpression) { + Object ttiKey = extractTemporaryTypeInfoKey(objectOfInstanceOf); ClassNode type = typeExpression.getType(); + typeCheckingContext.temporaryIfBranchTypeInformation.peek().computeIfAbsent(ttiKey, x -> new LinkedList<>()).add(type); + } + + /** + * Computes the key to use for {@link TypeCheckingContext#temporaryIfBranchTypeInformation}. + */ + protected Object extractTemporaryTypeInfoKey(final Expression expression) { + return expression instanceof VariableExpression ? findTargetVariable((VariableExpression) expression) : expression.getText(); + } + + /** + * A helper method which determines which receiver class should be used in error messages when a field or attribute + * is not found. The returned type class depends on whether we have temporary type information available (due to + * instanceof checks) and whether there is a single candidate in that case. + * + * @param expression the expression for which an unknown field has been found + * @param type the type of the expression (used as fallback type) + * @return if temporary information is available and there's only one type, returns the temporary type class + * otherwise falls back to the provided type class. + */ + protected ClassNode findCurrentInstanceOfClass(final Expression expression, final ClassNode type) { + List tempTypes = getTemporaryTypesForExpression(expression); + if (tempTypes.size() == 1) return tempTypes.get(0); + return type; + } + + protected List getTemporaryTypesForExpression(final Expression expression) { + Object key = extractTemporaryTypeInfoKey(expression); + List tempTypes = typeCheckingContext.temporaryIfBranchTypeInformation.stream().flatMap(tti -> + tti.getOrDefault(key, Collections.emptyList()).stream() + ).collect(Collectors.toList()); + int i = tempTypes.lastIndexOf(VOID_TYPE); + if (i != -1) { // assignment overwrites instanceof + tempTypes = tempTypes.subList(i + 1, tempTypes.size()); + } + return DefaultGroovyMethods.unique(tempTypes); // GROOVY-6429 + } + + private ClassNode getInferredTypeFromTempInfo(final Expression expression, final ClassNode expressionType) { + if (expression instanceof VariableExpression) { + List tempTypes = getTemporaryTypesForExpression(expression); + if (!tempTypes.isEmpty()) { + ClassNode superclass; + ClassNode[] interfaces; + if (expressionType instanceof WideningCategories.LowestUpperBoundClassNode) { + superclass = expressionType.getSuperClass(); + interfaces = expressionType.getInterfaces(); + } else if (expressionType != null && expressionType.isInterface()) { + superclass = OBJECT_TYPE; + interfaces = new ClassNode[]{expressionType}; + } else { + superclass = expressionType; + interfaces = ClassNode.EMPTY_ARRAY; + } + + List types = new ArrayList<>(); + if (superclass != null && !superclass.equals(OBJECT_TYPE) // GROOVY-7333 + && tempTypes.stream().noneMatch(t -> !t.equals(superclass) && t.isDerivedFrom(superclass))) { // GROOVY-9769 + types.add(superclass); + } + for (ClassNode anInterface : interfaces) { + if (tempTypes.stream().noneMatch(t -> t.implementsInterface(anInterface))) { // GROOVY-9769 + types.add(anInterface); + } + } + int tempTypesCount = tempTypes.size(); + if (tempTypesCount == 1 && types.isEmpty()) { + types.add(tempTypes.get(0)); + } else for (ClassNode tempType : tempTypes) { + if (!tempType.isInterface() // GROOVY-11290: keep most-specific types + ? (superclass == null || !superclass.isDerivedFrom(tempType)) + && (tempTypesCount == 1 || tempTypes.stream().noneMatch(t -> !t.equals(tempType) && t.isDerivedFrom(tempType))) + : (expressionType == null || !isOrImplements(expressionType, tempType)) + && (tempTypesCount == 1 || tempTypes.stream().noneMatch(t -> t != tempType && t.implementsInterface(tempType)))) { + types.add(tempType); + } + } + + int typesCount = types.size(); + if (typesCount == 1) { + return types.get(0); + } else if (typesCount > 1) { + return new UnionTypeClassNode(types.toArray(ClassNode.EMPTY_ARRAY)); + } + } + } + return expressionType; + } + //-------------------------------------------------------------------------- public static class SignatureCodecFactory { public static SignatureCodec getCodec(final int version, final ClassLoader classLoader) { switch (version) { - case 1: - return new SignatureCodecVersion1(classLoader); - default: - return null; + case 1: + return new SignatureCodecVersion1(classLoader); + default: + return null; } } } @@ -6107,7 +6132,7 @@ private class ParameterVariableExpression extends VariableExpression { if (inferredType == null) { inferredType = getTypeFromClosureArguments(parameter); // @ClosureParams or SAM-type coercion } - setNodeMetaData(INFERRED_TYPE, inferredType != null ? inferredType : parameter.getType()); // to parameter + setNodeMetaData(INFERRED_TYPE, inferredType != null ? inferredType : parameter.getType()); // GROOVY-10651 } } diff --git a/src/test/groovy/transform/stc/MethodCallsSTCTest.groovy b/src/test/groovy/transform/stc/MethodCallsSTCTest.groovy index 88465b2c497..ec291e6fa1c 100644 --- a/src/test/groovy/transform/stc/MethodCallsSTCTest.groovy +++ b/src/test/groovy/transform/stc/MethodCallsSTCTest.groovy @@ -859,42 +859,68 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase { ''' } + // GROOVY-5226, GROOVY-11290 void testShouldFailBecauseVariableIsReassigned() { - shouldFailWithMessages ''' - static String foo(String s) { - 'String' - } + String foo = 'def foo(CharSequence cs) { }' + + shouldFailWithMessages foo + ''' def it if (it instanceof String) { it = new Date() foo(it) } ''', - 'foo(java.util.Date)' + 'Cannot find matching method','#foo(java.util.Date)' + + shouldFailWithMessages foo + ''' + def bar(CharSequence cs) { } + def it + if (it instanceof CharSequence) { + if (it instanceof String) { + it = new Date() + foo(it) + } + bar(it) // it is CharSequence or Date + } + ''', + 'Cannot find matching method','#foo(java.util.Date)', + 'Cannot find matching method','#bar(java.util.Date)' } + // GROOVY-5226, GROOVY-11290 void testShouldNotFailEvenIfVariableIsReassigned() { - assertScript ''' - static String foo(int val) { - 'int' - } - def it + String foobar = 'def foo(int i) { }\ndef bar(CharSequence cs) { }' + + assertScript foobar + ''' + def it = "" if (it instanceof String) { + bar(it) it = 123 foo(it) } ''' + + assertScript foobar + ''' + def it = "" + if (it instanceof CharSequence) { + bar(it) + if (it instanceof String) { + bar(it) + it = 123 + foo(it) + } else { + bar(it) + } + } + ''' } - void testShouldNotFailEvenIfVariableIsReassignedAndInstanceOfIsEmbed() { + // GROOVY-5226, GROOVY-11290 + void testShouldNotFailEvenIfVariableIsReassignedAndMultipleInstanceOf() { assertScript ''' - static String foo(int val) { - 'int' - } - static String foo(Date val) { - 'Date' - } - def it + def foo(int i) { 'int' } + def foo(Date d) { 'Date' } + def it = "" if (it instanceof String) { it = 123 foo(it) diff --git a/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy b/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy index 575cda5f586..786b230199a 100644 --- a/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy +++ b/src/test/groovy/transform/stc/TypeInferenceSTCTest.groovy @@ -243,7 +243,8 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase { ''' } - @NotYetImplemented // GROOVY-10096 + // GROOVY-10096 + @NotYetImplemented void testInstanceOf10() { shouldFailWithMessages ''' class Foo { @@ -283,9 +284,24 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase { ''' } + // GROOVY-11290 + void testInstanceOf12() { + assertScript ''' + def test(List list) { + if (list instanceof List) { + (list*.toLowerCase()).join('') + } + } + + String result = test(['foo', 'bar']) + assert result == 'foobar' + ''' + } + + // GROOVY-5226 void testNestedInstanceOf1() { assertScript ''' - Object o + Object o = "foo" if (o instanceof Object) { if (o instanceof String) { o.toUpperCase() @@ -294,19 +310,42 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase { ''' } + // GROOVY-5226 void testNestedInstanceOf2() { - shouldFailWithMessages ''' - Object o + assertScript ''' + Object o = "foo" if (o instanceof String) { - if (o instanceof Object) { // causes the inferred type of 'o' to be overwritten + if (o instanceof Object) { o.toUpperCase() } } - ''', - 'Cannot find matching method java.lang.Object#toUpperCase()' + ''' } + // GROOVY-11290 void testNestedInstanceOf3() { + assertScript ''' + Object o = null + if (o instanceof Closeable) { + if (o instanceof Cloneable) { + o.close() + } + } + ''' + } + + void testNestedInstanceOf4() { + assertScript ''' + Object o = [1,2] as Number[] + if (o instanceof Object[]) { + if (o instanceof Number[]) { + o[0].intValue() + } + } + ''' + } + + void testNestedInstanceOf5() { assertScript ''' class A { int foo() { 1 } @@ -314,8 +353,16 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase { class B { int foo2() { 2 } } + def o = new A() int result = o instanceof A ? o.foo() : (o instanceof B ? o.foo2() : 3) + assert result == 1 + o = new B() + result = o instanceof A ? o.foo() : (o instanceof B ? o.foo2() : 3) + assert result == 2 + o = new Object() + result = o instanceof A ? o.foo() : (o instanceof B ? o.foo2() : 3) + assert result == 3 ''' }