SONARJAVA-5598 S3063: Fix FPs on usage in expressions, getChars, and fields (#5200)

Author: tomasz-tylenda-sonarsource

java-checks-test-sources/default/src/main/java/checks/unused/UnusedStringBuilderCheckSample.java

@@ -28,6 +28,28 @@ public String usedButInitializedSeparately() {
     return sb.toString();
   }
 
+  public String usedInExpression() {
+    StringBuilder inExpr = new StringBuilder();
+    inExpr.append("two ");
+    inExpr.append("three ");
+    return "one" + inExpr + "four";
+  }
+
+  public void usedInException() {
+    StringBuilder exceptionMessage = new StringBuilder();
+    exceptionMessage.append("message");
+    throw new IllegalStateException("message:" + exceptionMessage);
+  }
+
+  public char[] usedGetChars() {
+    StringBuilder buf = new StringBuilder();
+    buf.append("Hello ");
+    buf.append("World!");
+    char[] chars = new char[5];
+    buf.getChars(6, 11, chars, 0);
+    return chars;
+  }
+
   public void unused() {
     StringBuilder sb = new StringBuilder(); // Noncompliant {{Consume or remove this unused StringBuilder}}
 //                ^^
@@ -156,8 +178,8 @@ void appendHello() {
   }
 
   static class UnusedFieldPrivate {
-    private StringBuilder stringBuilder = new StringBuilder(); // Noncompliant
-//                        ^^^^^^^^^^^^^
+    // FN: Ideally we should report this one, but there are some FPs when analyzing fields, so we skip them.
+    private StringBuilder stringBuilder = new StringBuilder();
 
     void appendHello() {
       stringBuilder.append("Hello");
@@ -174,7 +196,7 @@ void appendHello() {
   }
 
   static class UsedFieldPrivate {
-    private StringBuilder stringBuilder = new StringBuilder(); // Compliant
+    private StringBuilder stringBuilder = new StringBuilder();
 
     void appendHello() {
       stringBuilder.append("Hello");

java-checks-test-sources/default/src/main/java/checks/unused/UnusedStringBuilderCheckSample2.java

@@ -0,0 +1,4 @@
+package checks.unused;
+
+public class UnusedStringBuilderCheckSample2 {
+}

java-checks/src/main/java/org/sonar/java/checks/unused/UnusedStringBuilderCheck.java

@@ -26,6 +26,7 @@
 import org.sonar.plugins.java.api.semantic.Symbol;
 import org.sonar.plugins.java.api.semantic.Type;
 import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
+import org.sonar.plugins.java.api.tree.BinaryExpressionTree;
 import org.sonar.plugins.java.api.tree.ExpressionTree;
 import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree;
 import org.sonar.plugins.java.api.tree.NewClassTree;
@@ -38,7 +39,7 @@
  */
 @Rule(key = "S3063")
 public class UnusedStringBuilderCheck extends IssuableSubscriptionVisitor {
-  private static final Set<String> TERMINAL_METHODS = Set.of("toString", "substring");
+  private static final Set<String> TERMINAL_METHODS = Set.of("getChars", "substring", "toString");
 
   @Override
   public List<Tree.Kind> nodesToVisit() {
@@ -51,10 +52,9 @@ public void visitNode(Tree tree) {
       Symbol symbol = variableTree.symbol();
       String typeName = getStringBuilderOrStringBuffer(symbol.type());
 
-      // Exclude non-local variables with non-private visibility,
-      // because they can be changed in a way that is hard to track.
+      // Exclude non-local variables, because they can be changed in a way that is hard to track.
       if (typeName != null && isInitializedByConstructor(variableTree.initializer()) &&
-        isLocalOrPrivate(symbol) &&
+        symbol.isLocalVariable() &&
         symbol.usages().stream().noneMatch(UnusedStringBuilderCheck::isUsedInAssignment) &&
         symbol.usages().stream().noneMatch(UnusedStringBuilderCheck::isConsumed)) {
         reportIssue(variableTree.simpleName(), "Consume or remove this unused %s".formatted(typeName));
@@ -83,10 +83,6 @@ private static boolean isInitializedByConstructor(@Nullable ExpressionTree initi
     return initializer instanceof NewClassTree;
   }
 
-  private static boolean isLocalOrPrivate(Symbol symbol) {
-    return symbol.isLocalVariable() || symbol.isPrivate();
-  }
-
   /**
    * True if the argument is the LHS of an assignment.
    *
@@ -114,7 +110,7 @@ private static boolean isConsumed(Tree tree) {
           .filter(UnusedStringBuilderCheck::isConsumed)
           .isPresent();
       }
-    } else if (parent instanceof ReturnStatementTree || parent instanceof ArgumentListTreeImpl) {
+    } else if (parent instanceof ReturnStatementTree || parent instanceof ArgumentListTreeImpl || parent instanceof BinaryExpressionTree) {
       return true;
     }
     return false;