SONARJAVA-4277 Rework issue message, add secondaries, and clean imple…

…mentation of S1142 (#4330)

java-checks-test-sources/src/main/files/non-compiling/checks/MethodWithExcessiveReturnsCheck.java

@@ -11,14 +11,14 @@ class A {
     return;
   }
 
-  void foo() { // Noncompliant [[sc=8;ec=11]] {{Reduce the number of returns of this method 4, down to the maximum allowed 3.}}
+  void foo() { // Noncompliant [[sc=8;ec=11]] {{This method has 4 returns, which is more than the 3 allowed.}}
     return;
     return;
     return;
     return;
   }
 
-  boolean foo2() { // Noncompliant [[sc=11;ec=15]] {{Reduce the number of returns of this method 4, down to the maximum allowed 3.}}
+  boolean foo2() { // Noncompliant [[sc=11;ec=15;secondary=22,23,24,25]] {{This method has 4 returns, which is more than the 3 allowed.}}
     return true;
     return false;
     return true;

java-checks-test-sources/src/main/java/checks/MethodWithExcessiveReturnsCheck.java

@@ -8,19 +8,19 @@ boolean foo1() { // Compliant
    return false;
   }
 
-  boolean foo2() { // Noncompliant [[sc=11;ec=15]] {{Reduce the number of returns of this method 4, down to the maximum allowed 3.}}
+  boolean foo2() { // Noncompliant [[sc=11;ec=15;secondary=12,13,14,15]] {{This method has 4 returns, which is more than the 3 allowed.}}
     if (false) return true;
     if (false) return false;
     if (false) return true;
     return false;
   }
 
-  void foo3() { // Noncompliant {{Reduce the number of returns of this method 4, down to the maximum allowed 3.}}
+  void foo3() { // Noncompliant {{This method has 4 returns, which is more than the 3 allowed.}}
     if (false) return;
     if (false) return;
 
     new MethodWithExcessiveReturnsCheck() {
-      public void f() { // Noncompliant {{Reduce the number of returns of this method 5, down to the maximum allowed 3.}}
+      public void f() { // Noncompliant [[sc=19;ec=20;secondary=24,25,26,27,28]] {{This method has 5 returns, which is more than the 3 allowed.}}
         if (false) return;
         if (false) return;
         if (false) return;
@@ -71,7 +71,7 @@ public boolean equals(MethodWithExcessiveReturnsCheck obj) { // Noncompliant bec
   }
 
   interface I {
-    default void method() { // Noncompliant {{Reduce the number of returns of this method 5, down to the maximum allowed 3.}}
+    default void method() { // Noncompliant {{This method has 5 returns, which is more than the 3 allowed.}}
       if (false) return;
       if (false) return;
       if (false) return;

java-checks-test-sources/src/main/java/checks/MethodWithExcessiveReturnsCheckCustom.java

@@ -20,7 +20,7 @@ void foo3() {
     if (false) return;
 
     new MethodWithExcessiveReturnsCheckCustom() {
-      public void f() { // Noncompliant {{Reduce the number of returns of this method 5, down to the maximum allowed 4.}}
+      public void f() { // Noncompliant {{This method has 5 returns, which is more than the 4 allowed.}}
         if (false) return;
         if (false) return;
         if (false) return;
@@ -46,7 +46,7 @@ Object foo4(List<Integer> bar) {
       if(false) return o2;
       return o1;
     });
-    bar.sort((o1, o2)-> { // Noncompliant [[sc=22;ec=24]] {{Reduce the number of returns of this method 6, down to the maximum allowed 4.}}
+    bar.sort((o1, o2)-> { // Noncompliant [[sc=22;ec=24;secondary=50,51,52,53,54,55]]]] {{This method has 6 returns, which is more than the 4 allowed.}}
       if(false) return o2;
       if(false) return o1;
       if(false) return o2;
@@ -58,7 +58,7 @@ Object foo4(List<Integer> bar) {
   }
 
   interface I {
-    default void method(boolean a) { // Noncompliant {{Reduce the number of returns of this method 5, down to the maximum allowed 4.}}
+    default void method(boolean a) { // Noncompliant {{This method has 5 returns, which is more than the 4 allowed.}}
       if(a) return;
       if(a) return;
       if(a) return;

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

@@ -19,71 +19,88 @@
  */
 package org.sonar.java.checks;
 
-import java.util.Arrays;
+import java.util.Collections;
 import java.util.Deque;
 import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
 import org.sonar.check.Rule;
 import org.sonar.check.RuleProperty;
 import org.sonar.java.checks.helpers.MethodTreeUtils;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.JavaFileScannerContext;
 import org.sonar.plugins.java.api.tree.LambdaExpressionTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
+import org.sonar.plugins.java.api.tree.ReturnStatementTree;
 import org.sonar.plugins.java.api.tree.Tree;
 
 @Rule(key = "S1142")
 public class MethodWithExcessiveReturnsCheck extends IssuableSubscriptionVisitor {
 
+  private static final String ISSUE_MESSAGE = "This method has %d returns, which is more than the %d allowed.";
+
   private static final int DEFAULT_MAX = 3;
 
   @RuleProperty(description = "Maximum allowed return statements per method", defaultValue = "" + DEFAULT_MAX)
   public int max = DEFAULT_MAX;
 
-  private final Map<Tree, Integer> returnStatementCounter = new HashMap<>();
-  private final Deque<Tree> methods = new LinkedList<>();
+  private final Map<Tree, List<Tree>> returnStatements = new HashMap<>();
+  private final Deque<Tree> methodsOrLambdas = new LinkedList<>();
 
   @Override
   public void leaveFile(JavaFileScannerContext context) {
-    returnStatementCounter.clear();
+    returnStatements.clear();
+    methodsOrLambdas.clear();
   }
 
   @Override
   public List<Tree.Kind> nodesToVisit() {
-    return Arrays.asList(Tree.Kind.RETURN_STATEMENT, Tree.Kind.METHOD, Tree.Kind.LAMBDA_EXPRESSION);
+    return List.of(Tree.Kind.RETURN_STATEMENT, Tree.Kind.METHOD, Tree.Kind.LAMBDA_EXPRESSION);
   }
 
   @Override
   public void visitNode(Tree tree) {
     if (tree.is(Tree.Kind.RETURN_STATEMENT)) {
-      returnStatementCounter.compute(methods.peek(), (k, v) -> (v == null) ? 1 : (v + 1));
+      returnStatements.computeIfAbsent(methodsOrLambdas.peek(), k -> new LinkedList<>())
+        .add(((ReturnStatementTree) tree).returnKeyword());
     } else {
-      methods.push(tree);
+      methodsOrLambdas.push(tree);
     }
   }
 
   @Override
   public void leaveNode(Tree tree) {
-    Tree reportTree = null;
-    if (tree.is(Tree.Kind.METHOD)) {
-      MethodTree method = (MethodTree) tree;
-      if (MethodTreeUtils.isEqualsMethod(method)) {
-        methods.pop();
-      } else {
-        reportTree = method.simpleName();
-      }
+    if (!tree.is(Tree.Kind.RETURN_STATEMENT)) {
+      reportTree(tree).ifPresent(reportTree -> report(tree, reportTree));
+      returnStatements.remove(tree);
+      methodsOrLambdas.pop();
+    }
+  }
 
-    } else if (tree.is(Tree.Kind.LAMBDA_EXPRESSION)) {
-      reportTree = ((LambdaExpressionTree) tree).arrowToken();
+  private static Optional<Tree> reportTree(Tree methodOrLambda) {
+    if (methodOrLambda.is(Tree.Kind.LAMBDA_EXPRESSION)) {
+      return Optional.of(((LambdaExpressionTree) methodOrLambda).arrowToken());
     }
-    if (reportTree != null) {
-      int count = returnStatementCounter.getOrDefault(tree, 0);
-      if (count > max) {
-        reportIssue(reportTree, "Reduce the number of returns of this method " + count + ", down to the maximum allowed " + max + ".");
-      }
-      methods.pop();
+    MethodTree method = (MethodTree) methodOrLambda;
+    if (!MethodTreeUtils.isEqualsMethod(method)) {
+      return Optional.of(method.simpleName());
+    }
+    // equals can have many returns and it's OK
+    return Optional.empty();
+  }
+
+  private void report(Tree currentTree, Tree reportTree) {
+    List<Tree> returns = returnStatements.getOrDefault(currentTree, Collections.emptyList());
+    int count = returns.size();
+    if (count > max) {
+      String message = String.format(ISSUE_MESSAGE, count, max);
+      List<JavaFileScannerContext.Location> secondaries = returns.stream()
+        .map(token -> new JavaFileScannerContext.Location("return", token))
+        .collect(Collectors.toList());
+      reportIssue(reportTree, message, secondaries, null);
     }
   }
 }

java-checks/src/test/java/org/sonar/java/checks/MethodWithExcessiveReturnsCheckTest.java

@@ -22,15 +22,15 @@
 import org.junit.jupiter.api.Test;
 import org.sonar.java.checks.verifier.CheckVerifier;
 
+import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath;
 import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath;
-import static org.sonar.java.checks.verifier.TestUtils.testSourcesPath;
 
 class MethodWithExcessiveReturnsCheckTest {
 
   @Test
   void detected() {
     CheckVerifier.newVerifier()
-      .onFile(testSourcesPath("checks/MethodWithExcessiveReturnsCheck.java"))
+      .onFile(mainCodeSourcesPath("checks/MethodWithExcessiveReturnsCheck.java"))
       .withCheck(new MethodWithExcessiveReturnsCheck())
       .verifyIssues();
   }
@@ -48,7 +48,7 @@ void custom() {
     MethodWithExcessiveReturnsCheck check = new MethodWithExcessiveReturnsCheck();
     check.max = 4;
     CheckVerifier.newVerifier()
-      .onFile(testSourcesPath("checks/MethodWithExcessiveReturnsCheckCustom.java"))
+      .onFile(mainCodeSourcesPath("checks/MethodWithExcessiveReturnsCheckCustom.java"))
       .withCheck(check)
       .verifyIssues();
   }