IDEA-82118 ("Replace 'for each' loop with iterator 'for' loop intention ignores bounds on the Iterable's type parameters, generates invalid code)

plugins/IntentionPowerPak/src/com/siyeh/ipp/forloop/ReplaceForEachLoopWithIteratorForLoopIntention.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2003-2011 Dave Griffith, Bas Leijdekkers
+ * Copyright 2003-2012 Dave Griffith, Bas Leijdekkers
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -35,32 +35,40 @@ public class ReplaceForEachLoopWithIteratorForLoopIntention extends Intention {
   }
 
   @Override
-  public void processIntention(@NotNull PsiElement element)
-    throws IncorrectOperationException {
-    final PsiForeachStatement statement =
-      (PsiForeachStatement)element.getParent();
+  public void processIntention(@NotNull PsiElement element) throws IncorrectOperationException {
+    final PsiForeachStatement statement = (PsiForeachStatement)element.getParent();
     if (statement == null) {
       return;
     }
     final Project project = statement.getProject();
-    final JavaCodeStyleManager codeStyleManager =
-      JavaCodeStyleManager.getInstance(project);
+    final JavaCodeStyleManager codeStyleManager = JavaCodeStyleManager.getInstance(project);
     final PsiExpression iteratedValue = statement.getIteratedValue();
     if (iteratedValue == null) {
       return;
     }
     @NonNls final StringBuilder newStatement = new StringBuilder();
-    final PsiParameter iterationParameter =
-      statement.getIterationParameter();
+    final PsiType iteratedValueType = iteratedValue.getType();
+    if (!(iteratedValueType instanceof PsiClassType)) {
+      return;
+    }
+    final PsiClassType classType = (PsiClassType)iteratedValueType;
+    final PsiParameter iterationParameter = statement.getIterationParameter();
     final PsiType parameterType = iterationParameter.getType();
-    final String iterator =
-      codeStyleManager.suggestUniqueVariableName("iterator",
-                                                 statement, true);
+    final String iterator = codeStyleManager.suggestUniqueVariableName("iterator", statement, true);
     final String typeText = parameterType.getCanonicalText();
     newStatement.append("for(java.util.Iterator");
-    newStatement.append('<');
-    newStatement.append(typeText);
-    newStatement.append("> ");
+    if (classType.hasParameters()) {
+      newStatement.append('<');
+      final PsiType[] parameters = classType.getParameters();
+      if (parameters.length == 1) {
+        newStatement.append(parameters[0].getCanonicalText());
+      } else {
+        newStatement.append(typeText);
+      }
+      newStatement.append("> ");
+    } else {
+      newStatement.append(' ');
+    }
     newStatement.append(iterator);
     newStatement.append(" = ");
     if (iteratedValue instanceof PsiTypeCastExpression) {

plugins/IntentionPowerPak/test/com/siyeh/ipp/forloop/iterator/BareCollectionLoop.java

@@ -0,0 +1,12 @@
+package com.siyeh.ipp.forloop.iterator;
+
+import java.util.Collection;
+
+class BareCollectionLoop {
+
+  void x(Collection c) {
+    fo<caret>r (Object n : c) {
+
+    }
+  }
+}

plugins/IntentionPowerPak/test/com/siyeh/ipp/forloop/iterator/BareCollectionLoop_after.java

@@ -0,0 +1,14 @@
+package com.siyeh.ipp.forloop.iterator;
+
+import java.util.Collection;
+import java.util.Iterator;
+
+class BareCollectionLoop {
+
+  void x(Collection c) {
+      for (Iterator iterator = c.iterator(); iterator.hasNext(); ) {
+          Object n = iterator.next();
+
+      }
+  }
+}

plugins/IntentionPowerPak/test/com/siyeh/ipp/forloop/iterator/BoundedTypes.java

@@ -0,0 +1,11 @@
+package com.siyeh.ipp.forloop.iterator;
+
+import java.util.Collection;
+
+class BoundedTypes {
+  void x(Collection<? extends Number> c) {
+    <caret>for (Number n : c) {
+
+    }
+  }
+}

plugins/IntentionPowerPak/test/com/siyeh/ipp/forloop/iterator/BoundedTypes_after.java

@@ -0,0 +1,13 @@
+package com.siyeh.ipp.forloop.iterator;
+
+import java.util.Collection;
+import java.util.Iterator;
+
+class BoundedTypes {
+  void x(Collection<? extends Number> c) {
+      for (Iterator<? extends Number> iterator = c.iterator(); iterator.hasNext(); ) {
+          Number n = iterator.next();
+
+      }
+  }
+}

plugins/IntentionPowerPak/test/com/siyeh/ipp/forloop/iterator/GenericTypes.java

@@ -0,0 +1,18 @@
+package com.siyeh.ipp.forloop.iterator;
+
+import java.util.Iterator;
+
+class GenericTypes<K, V> implements Iterable<V> {
+
+  @Override
+  public Iterator<V> iterator() {
+    return null;
+  }
+
+  public void test() {
+    final GenericTypes<String, Integer> test = new GenericTypes<String, Integer>();
+    <caret>for (Integer integer : test) {
+
+    }
+  }
+}

plugins/IntentionPowerPak/test/com/siyeh/ipp/forloop/iterator/GenericTypes_after.java

@@ -0,0 +1,19 @@
+package com.siyeh.ipp.forloop.iterator;
+
+import java.util.Iterator;
+
+class GenericTypes<K, V> implements Iterable<V> {
+
+  @Override
+  public Iterator<V> iterator() {
+    return null;
+  }
+
+  public void test() {
+    final GenericTypes<String, Integer> test = new GenericTypes<String, Integer>();
+      for (Iterator<Integer> iterator = test.iterator(); iterator.hasNext(); ) {
+          Integer integer = iterator.next();
+
+      }
+  }
+}

plugins/IntentionPowerPak/testSrc/com/siyeh/ipp/forloop/ReplaceForEachLoopWithIteratorForLoopIntentionTest.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright 2000-2012 JetBrains s.r.o.
+ *
+ * Licensed 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 com.siyeh.ipp.forloop;
+
+import com.siyeh.IntentionPowerPackBundle;
+import com.siyeh.ipp.IPPTestCase;
+
+import java.util.Collection;
+
+public class ReplaceForEachLoopWithIteratorForLoopIntentionTest extends IPPTestCase {
+  public void testBareCollectionLoop() { doTest(); }
+  public void testGenericTypes() { doTest(); }
+  public void testBoundedTypes() { doTest(); }
+
+  @Override
+  protected String getIntentionName() {
+    return IntentionPowerPackBundle.message("replace.for.each.loop.with.iterator.for.loop.intention.name");
+  }
+
+  @Override
+  protected String getRelativePath() {
+    return "forloop/iterator";
+  }
+
+  void x(Collection c) {
+    for (Object n : c) {
+
+    }
+  }
+}