SONARJAVA-3829 S2629 should not report when log level is enabled (#4877)

its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json

@@ -1316,7 +1316,7 @@
   {
     "ruleKey": "S2629",
     "hasTruePositives": true,
-    "falseNegatives": 58,
+    "falseNegatives": 60,
     "falsePositives": 0
   },
   {

its/autoscan/src/test/resources/autoscan/diffs/diff_S2629.json

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S2629",
   "hasTruePositives": true,
-  "falseNegatives": 58,
+  "falseNegatives": 60,
   "falsePositives": 0
-}
+}

java-checks-test-sources/default/src/main/java/checks/LazyArgEvaluationCheckSampleFPs.java

@@ -0,0 +1,49 @@
+package checks;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.Marker;
+import org.slf4j.MarkerFactory;
+
+class FalsePositivesFromTheCommunity {
+
+  private static final Marker myMarker = MarkerFactory.getMarker("MY_MARKER");
+  private static final Logger logger = LoggerFactory.getLogger(FalsePositivesFromTheCommunity.class);
+
+  Object doSomething() {
+    return null;
+  }
+
+  // https://community.sonarsource.com/t/false-positive-on-java-s2629/42091
+  void foo() {
+
+    logger.debug(myMarker, "message1: {}.", doSomething()); // Compliant - because we don't care about small performance loss in exceptional paths
+
+    if (logger.isDebugEnabled(myMarker)) {
+      logger.debug(myMarker, "message2: {}.", doSomething() + "yolo"); // Compliant as method(s) invoked conditionally - WAS FP due to missing MARKER param handling
+      logger.debug(myMarker, "message2a: {}.", doSomething()); // Compliant
+    }
+  }
+
+  // https://community.sonarsource.com/t/s2629-despite-using-isinfoenabled/120810
+  void foo2() {
+
+    logger.debug(myMarker, "message3: {}.", doSomething()); // Compliant - because we don't care about small performance loss in exceptional paths
+    logger.debug(myMarker, "message4: {}.", doSomething() + "yolo"); // Noncompliant {{Invoke method(s) only conditionally. Use the built-in formatting to construct this argument.}}
+
+    // this return makes the later debug statements in this method compliant
+    if (!logger.isDebugEnabled(myMarker)) {
+      return;
+    }
+    logger.debug(myMarker, "message4: {}.", doSomething() + "yolo"); // Compliant as method(s) invoked conditionally - IS FP
+    logger.debug(myMarker, "message4a: {}.", doSomething()); // Compliant
+  }
+
+  void fooBar() {
+
+    // test without return statement
+    logger.debug(myMarker, "message3: {}.", doSomething()); // Compliant - because we don't care about small performance loss in exceptional paths
+    logger.debug(myMarker, "message4: {}.", doSomething() + "yolo"); // Noncompliant {{Invoke method(s) only conditionally. Use the built-in formatting to construct this argument.}}
+  }
+
+}

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

@@ -41,6 +41,7 @@
 import org.sonar.plugins.java.api.tree.MethodInvocationTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
 import org.sonar.plugins.java.api.tree.NewClassTree;
+import org.sonar.plugins.java.api.tree.ReturnStatementTree;
 import org.sonar.plugins.java.api.tree.Tree;
 
 import static org.sonar.plugins.java.api.semantic.MethodMatchers.ANY;
@@ -81,6 +82,7 @@ private static class SLF4J {
       .ofSubTypes(LOGGER)
       .names(testMethodNames(METHOD_NAMES))
       .addWithoutParametersMatcher()
+      .addParametersMatcher(MARKER)
       .build();
   }
 
@@ -185,6 +187,7 @@ private static String[] testMethodNames(String[] lowerCaseNames) {
 
   private JavaFileScannerContext context;
   private Deque<Tree> treeStack = new ArrayDeque<>();
+  private boolean foundReturnStatementInsideLevelTest = false;
 
   @Override
   public void scanFile(JavaFileScannerContext context) {
@@ -197,7 +200,11 @@ public void scanFile(JavaFileScannerContext context) {
 
   @Override
   public void visitMethodInvocation(MethodInvocationTree tree) {
-    if (LAZY_ARG_METHODS.matches(tree) && !insideCatchStatement() && !insideLevelTest() && !argsUsingSuppliers(tree)) {
+    if (LAZY_ARG_METHODS.matches(tree) &&
+      !insideCatchStatement() &&
+      !insideLevelTest() &&
+      !foundReturnStatementInsideLevelTest &&
+      !argsUsingSuppliers(tree)) {
       onMethodInvocationFound(tree);
     }
   }
@@ -217,6 +224,14 @@ public void visitIfStatement(IfStatementTree ifTree) {
     }
   }
 
+  @Override
+  public void visitReturnStatement(ReturnStatementTree tree) {
+    if (insideLevelTest()) {
+      // record when a return is detected within a log test
+      foundReturnStatementInsideLevelTest = true;
+    }
+  }
+
   @Override
   public void visitCatch(CatchTree tree) {
     stackAndContinue(tree, super::visitCatch);
@@ -226,6 +241,8 @@ public void visitCatch(CatchTree tree) {
   public void visitMethod(MethodTree tree) {
     // we put method trees on stack to be able to detect log statements in anonymous classes
     stackAndContinue(tree, super::visitMethod);
+    // once a method is exited, then this field cannot be true (if it was set to true)
+    foundReturnStatementInsideLevelTest = false;
   }
 
   private boolean insideLevelTest() {

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

@@ -35,6 +35,14 @@ void test() {
       .verifyIssues();
   }
 
+  @Test
+  void test_fps() {
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath("checks/LazyArgEvaluationCheckSampleFPs.java"))
+      .withCheck(new LazyArgEvaluationCheck())
+      .verifyIssues();
+  }
+
   @Test
   void test_non_compiling() {
     CheckVerifier.newVerifier()