From cef611c2d22e76be5a8b3e1cc73d3774a3a997e9 Mon Sep 17 00:00:00 2001 From: Eric Milles Date: Tue, 15 Oct 2024 14:17:24 -0500 Subject: [PATCH] GROOVY-11305: STC: support for-each loop for implicitly-`Iterable` types --- .../groovy/runtime/DefaultGroovyMethods.java | 50 +++++++++-------- .../sc/StaticCompilationVisitor.java | 14 ++--- .../stc/StaticTypeCheckingVisitor.java | 30 ++++++----- src/test/groovy/bugs/Groovy8169.groovy | 49 ----------------- .../groovy/transform/stc/LoopsSTCTest.groovy | 54 +++++++++++++++---- 5 files changed, 93 insertions(+), 104 deletions(-) delete mode 100644 src/test/groovy/bugs/Groovy8169.groovy diff --git a/src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java b/src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java index 231920eb7d0..86964355833 100644 --- a/src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java +++ b/src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java @@ -8271,38 +8271,49 @@ public static boolean isWhitespace(Character self) { // iterator /** - * Attempts to create an Iterator for the given object by first - * converting it to a Collection. + * Creates an Iterator for the given Object by converting it to a Collection. * - * @param o an object - * @return an Iterator for the given Object. + * @param self an object + * @return an Iterator for the given Object * @see org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation#asCollection(java.lang.Object) * @since 1.0 */ - public static Iterator iterator(final Object o) { - if (o instanceof Iterator) return (Iterator)o; - return DefaultTypeTransformation.asCollection(o).iterator(); + public static Iterator iterator(final Object self) { + if (self instanceof Iterator) return (Iterator) self; + return DefaultTypeTransformation.asCollection(self).iterator(); + } + + /** + * Supports 'duck-typing' when trying to get an Iterator for each element of + * a Collection, some of which may already be an Iterator. + * + * @param self an Iterator object + * @return the Iterator itself + * @since 1.5.0 + */ + public static Iterator iterator(final Iterator self) { + return self; } /** - * Allows an Enumeration to behave like an Iterator. Note that the - * {@link java.util.Iterator#remove() remove()} method is unsupported since the - * underlying Enumeration does not provide a mechanism for removing items. + * Allows an Enumeration to behave like an Iterable. Note that the + * {@link java.util.Iterator#remove() remove()} method is unsupported since + * the underlying Enumeration doesn't provide a mechanism for removing items. * - * @param enumeration an Enumeration object + * @param self an Enumeration object * @return an Iterator for the given Enumeration * @since 1.0 */ - public static Iterator iterator(final Enumeration enumeration) { + public static Iterator iterator(final Enumeration self) { return new Iterator() { @Override public boolean hasNext() { - return enumeration.hasMoreElements(); + return self.hasMoreElements(); } @Override public T next() { - return enumeration.nextElement(); + return self.nextElement(); } @Override @@ -8313,15 +8324,12 @@ public void remove() { } /** - * An identity function for iterators, supporting 'duck-typing' when trying to get an - * iterator for each object within a collection, some of which may already be iterators. + * Returns an Iterator for the given Map's entry set. * - * @param self an iterator object - * @return itself - * @since 1.5.0 + * @since 5.0.0 */ - public static Iterator iterator(Iterator self) { - return self; + public static Iterator> iterator(final Map self) { + return self.entrySet().iterator(); } //-------------------------------------------------------------------------- diff --git a/src/main/java/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java b/src/main/java/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java index 6edf30b6db5..e8b95ad4780 100644 --- a/src/main/java/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java @@ -255,18 +255,12 @@ public void visitConstructorCallExpression(final ConstructorCallExpression call) @Override public void visitForLoop(final ForStatement statement) { super.visitForLoop(statement); - Expression collectionExpression = statement.getCollectionExpression(); + var collectionExpression = statement.getCollectionExpression(); if (!(collectionExpression instanceof ClosureListExpression)) { - ClassNode forLoopVariableType = statement.getVariableType(); - ClassNode collectionType = getType(collectionExpression); - ClassNode componentType; - if (ClassHelper.isWrapperCharacter(ClassHelper.getWrapper(forLoopVariableType)) && ClassHelper.isStringType(collectionType)) { - // we allow auto-coercion here - componentType = forLoopVariableType; - } else { - componentType = inferLoopElementType(collectionType); + if (statement.getVariable().isDynamicTyped()) { // GROOVY-8169 + ClassNode inferredType = getType(statement.getVariable()); + statement.getVariable().setType(inferredType); // GROOVY-5640, GROOVY-5641 } - statement.getVariable().setType(componentType); } } 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 f91b21482c8..2f342b6ae6e 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -2105,21 +2105,25 @@ public void visitForLoop(final ForStatement forLoop) { ClassNode collectionType = getType(collectionExpression); ClassNode forLoopVariableType = forLoop.getVariableType(); ClassNode componentType; - if (isWrapperCharacter(getWrapper(forLoopVariableType)) && isStringType(collectionType)) { - // we allow auto-coercion here - componentType = forLoopVariableType; + if (isStringType(collectionType)) { + if (isWrapperCharacter(getWrapper(forLoopVariableType))) { + // we support auto-coercion for char + componentType = forLoopVariableType; + } else { + componentType = STRING_TYPE; + } } else { - componentType = inferLoopElementType(collectionType); - } - if (getUnwrapper(componentType) == forLoopVariableType) { - // prefer primitive type over boxed type - componentType = forLoopVariableType; + componentType = inferComponentType(collectionType, null); + if (componentType == null) { + componentType = OBJECT_TYPE; + } else if (getUnwrapper(componentType).equals(forLoopVariableType)) { + componentType = forLoopVariableType; // prefer the primitive type + } } if (!checkCompatibleAssignmentTypes(forLoopVariableType, componentType)) { addStaticTypeError("Cannot loop with element of type " + prettyPrintType(forLoopVariableType) + " with collection of type " + prettyPrintType(collectionType), forLoop); } - if (!isDynamicTyped(forLoopVariableType)) { - // user has specified a type, prefer it over the inferred type + if (!isDynamicTyped(forLoopVariableType)) { // user-supplied type componentType = forLoopVariableType; } typeCheckingContext.controlStructureVariables.put(forLoop.getVariable(), componentType); @@ -5785,11 +5789,11 @@ private static Map extractPlaceHolders(final Cla Map result = null; ClassNode[] todo; if (receiver instanceof UnionTypeClassNode) { - todo = ((UnionTypeClassNode) receiver).getDelegates(); + todo = ((UnionTypeClassNode) receiver).getDelegates(); // GROOVY-7275 } else { todo = new ClassNode[] {!isPrimitiveType(declaringClass) ? wrapTypeIfNecessary(receiver) : receiver}; } - for (ClassNode type : todo) { +out: for (ClassNode type : todo) { for (ClassNode current = type; current != null; ) { Map placeHolders = new HashMap<>(); if (current.isGenericsPlaceHolder()) @@ -5824,7 +5828,7 @@ else if (current.getGenericsTypes() != null result = placeHolders; // we are done if we are now in the declaring class - if (currentIsDeclaring) break; + if (currentIsDeclaring) break out; current = getNextSuperClass(current, declaringClass); if (current == null && isClassType(declaringClass)) { diff --git a/src/test/groovy/bugs/Groovy8169.groovy b/src/test/groovy/bugs/Groovy8169.groovy deleted file mode 100644 index 6b82899e47b..00000000000 --- a/src/test/groovy/bugs/Groovy8169.groovy +++ /dev/null @@ -1,49 +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 groovy.transform.CompileStatic -import org.junit.Test - -import static groovy.test.GroovyAssert.assertScript - -@CompileStatic -final class Groovy8169 { - - @Test - void testForLoopVariableRetainsOriginType() { - assertScript ''' - import groovy.transform.* - import org.codehaus.groovy.ast.stmt.* - import static org.codehaus.groovy.ast.ClassHelper.* - - @ASTTest(phase=CLASS_GENERATION, value={ - def loop = node.code.statements.find { it instanceof ForStatement } - assert loop.variable.originType == OBJECT_TYPE - assert loop.variable.type == STRING_TYPE - }) - @CompileStatic - void m() { - List strings = ['one', 'two'] - for (Object s in strings) { - } - } - ''' - } -} diff --git a/src/test/groovy/transform/stc/LoopsSTCTest.groovy b/src/test/groovy/transform/stc/LoopsSTCTest.groovy index e0562a61400..b104af4a14d 100644 --- a/src/test/groovy/transform/stc/LoopsSTCTest.groovy +++ b/src/test/groovy/transform/stc/LoopsSTCTest.groovy @@ -114,21 +114,19 @@ class LoopsSTCTest extends StaticTypeCheckingTestCase { void testForInLoopOnMap1() { assertScript ''' @ASTTest(phase=INSTRUCTION_SELECTION, value={ - lookup('forLoop').each { - assert it instanceof org.codehaus.groovy.ast.stmt.ForStatement - def collection = it.collectionExpression // MethodCallExpression - def inft = collection.getNodeMetaData(INFERRED_TYPE) - assert inft == make(Set) - def entryInft = inft.genericsTypes[0].type - assert entryInft == make(Map.Entry) - assert entryInft.genericsTypes[0].type == STRING_TYPE - assert entryInft.genericsTypes[1].type == Integer_TYPE - } + def loop = lookup('loop')[0] + assert loop instanceof org.codehaus.groovy.ast.stmt.ForStatement + def collectionType = loop.collectionExpression.getNodeMetaData(INFERRED_TYPE) + assert collectionType == SET_TYPE + def elementType = collectionType.genericsTypes[0].type + assert elementType == make(Map.Entry) + assert elementType.genericsTypes[0].type == STRING_TYPE + assert elementType.genericsTypes[1].type == Integer_TYPE }) void test() { def result = "" def sum = 0 - forLoop: + loop: for ( Map.Entry it in [a:1, b:3].entrySet() ) { result += it.getKey() sum += it.getValue() @@ -187,6 +185,23 @@ class LoopsSTCTest extends StaticTypeCheckingTestCase { ''' } + // GROOVY-8169 + void testForInLoopOnList2() { + assertScript ''' + @ASTTest(phase=INSTRUCTION_SELECTION, value={ + def loop = lookup('loop')[0] + assert loop.variable.type == OBJECT_TYPE + assert loop.variable.originType == OBJECT_TYPE + }) + void test() { + List strings = ['one', 'two'] + loop: + for (Object s in strings) { } + } + test() + ''' + } + // GROOVY-8643 void testForInLoopOnArray() { assertScript ''' @@ -295,6 +310,23 @@ class LoopsSTCTest extends StaticTypeCheckingTestCase { ''' } + // GROOVY-11305 + void testForInLoopOnNearlyIterable() { + assertScript ''' + class C { + Iterator iterator() { + ['a','b','c'].iterator() + } + } + def list = [] + def c = new C() + for (item in c) { + list.add(item.toUpperCase()) + } + assert list == ['A','B','C'] + ''' + } + // GROOVY-10651 void testForInLoopOnRawTypeIterable() { assertScript '''