SONARJAVA-1217 Refine raised issues

When the iterable is an identifier raise an issue only if its owner
is a method to skip fields.

When the iterable is not an identifier raise issues only for
consecutives foreachs.

Author: mpaladin

its/ruling/src/test/resources/expected/squid-S3047.json

@@ -5,15 +5,6 @@
 'project:java/awt/Window.java':[
 1614,
 ],
-'project:java/lang/ApplicationShutdownHooks.java':[
-104,
-],
-'project:java/lang/ProcessEnvironment.java':[
-279,
-],
-'project:javax/swing/colorchooser/ColorPanel.java':[
-96,
-],
 'project:javax/swing/plaf/synth/SynthDesktopPaneUI.java':[
 219,
 ],

java-checks/src/main/java/org/sonar/java/checks/LoopsOnSameSetCheck.java

@@ -37,11 +37,10 @@
 import org.sonar.squidbridge.annotations.SqaleConstantRemediation;
 import org.sonar.squidbridge.annotations.SqaleSubCharacteristic;
 
+import javax.annotation.Nullable;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 @Rule(
   key = "S3047",
@@ -61,41 +60,42 @@ public List<Tree.Kind> nodesToVisit() {
   @Override
   public void visitNode(Tree tree) {
     Map<Symbol, Integer> forEachSymbols = new HashMap<>();
-    Set<Tree> forEachIterables = new HashSet<>();
+    Tree previousForeachIterable = null;
     for (Tree item : ((BlockTree) tree).body()) {
       if (item.is(Tree.Kind.FOR_EACH_STATEMENT)) {
-        checkForEach(forEachSymbols, forEachIterables, (ForEachStatement) item);
+        ForEachStatement forEachStatement = (ForEachStatement) item;
+        checkForEach(forEachSymbols, previousForeachIterable, forEachStatement);
+        previousForeachIterable = forEachStatement.expression();
       } else {
+        previousForeachIterable = null;
         item.accept(new InvalidatorVisitor(forEachSymbols));
       }
     }
   }
 
-  private void checkForEach(Map<Symbol, Integer> forEachSymbols, Set<Tree> forEachIterables, ForEachStatement item) {
+  private void checkForEach(Map<Symbol, Integer> forEachSymbols, @Nullable Tree previousIterable, ForEachStatement item) {
     ExpressionTree expressionTree = ExpressionsHelper.skipParentheses(item.expression());
     if (expressionTree.is(Tree.Kind.IDENTIFIER)) {
       checkForEachIdentifier(forEachSymbols, (IdentifierTree) expressionTree);
-    } else {
-      checkForEachExpression(forEachIterables, expressionTree);
+    } else if (previousIterable != null) {
+      checkForEachExpression(previousIterable, expressionTree);
     }
   }
 
-  private void checkForEachExpression(Set<Tree> forEachIterables, ExpressionTree expressionTree) {
-    for (Tree subTree : forEachIterables) {
-      if (SyntacticEquivalence.areEquivalent(expressionTree, subTree)) {
-        addIssue(expressionTree, FirstSyntaxTokenFinder.firstSyntaxToken(subTree).line());
-        return;
-      }
+  private void checkForEachExpression(Tree forEachIterable, ExpressionTree expressionTree) {
+    if (SyntacticEquivalence.areEquivalent(expressionTree, forEachIterable)) {
+      addIssue(expressionTree, FirstSyntaxTokenFinder.firstSyntaxToken(forEachIterable).line());
     }
-    forEachIterables.add(expressionTree);
   }
 
   private void checkForEachIdentifier(Map<Symbol, Integer> forEachSymbols, IdentifierTree node) {
     Symbol symbol = node.symbol();
-    if (forEachSymbols.containsKey(symbol)) {
-      addIssue(node, forEachSymbols.get(symbol));
-    } else {
-      forEachSymbols.put(symbol, ((JavaTree) node).getLine());
+    if (symbol.owner().isMethodSymbol()) {
+      if (forEachSymbols.containsKey(symbol)) {
+        addIssue(node, forEachSymbols.get(symbol));
+      } else {
+        forEachSymbols.put(symbol, ((JavaTree) node).getLine());
+      }
     }
   }
 

java-checks/src/test/files/checks/LoopsOnSameSetCheck.java

@@ -7,17 +7,22 @@ private List<String> getList() {
     return null;
   }
 
+  private List<String> globalStrings;
+
   public void doSomethingToAList(List<String> strings, List<String> strings2) {
     for (String str : strings);
-    for (String str : strings); // Noncompliant {{Combine this loop with the one that starts on line 11.}}
+    foo();
+    for (String str : strings); // Noncompliant {{Combine this loop with the one that starts on line 13.}}
     strings = null;
     for (String str : strings); // strings get reassigned
     for (String str : getList());
-    for (String str : getList()); // Noncompliant {{Combine this loop with the one that starts on line 15.}}
+    for (String str : getList()); // Noncompliant {{Combine this loop with the one that starts on line 18.}}
+    foo();
+    for (String str : getList()); // no issue if something in between for loops
     for (String str : foo());
     List<String> list = null;
     for (String str : list);
-    for (String str : list); // Noncompliant {{Combine this loop with the one that starts on line 19.}}
+    for (String str : list); // Noncompliant {{Combine this loop with the one that starts on line 24.}}
     if (list.size() > 10) {
       list.remove(0);
     }
@@ -35,5 +40,9 @@ void foo() {
         }
       }
     };
+    for (String str : globalStrings);
+    modifyGlobalStrings();
+    // no issues for fields
+    for (String str : globalStrings);
   }
 }