SONARJAVA-5485 S1481: Report on unnamed variables in try with resources (#5090)

Co-authored-by: erwan-serandour <erwan.serandour@sonarsource.com>

Author: tomasz-tylenda-sonarsource

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

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S1481",
   "hasTruePositives": true,
-  "falseNegatives": 7,
+  "falseNegatives": 10,
   "falsePositives": 0
-}
+}

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

@@ -41,7 +41,15 @@ public void f(int unusedParameter, Object o) {
     } catch (Exception e) {
     }
 
-    try (Stream foo = Stream.of()) { // Compliant
+    try (Stream stream1 = Stream.of(); // Noncompliant {{Remove this unused "stream1" local variable.}} [[quickfixes=qf_tr1]]
+//              ^^^^^^^
+//              fix@qf_tr1 {{Replace unused local variable with _}}
+//              edit@qf_tr1 [[sc=10;ec=24]] {{var _}}
+         Stream _ = Stream.of();
+         Stream stream3 = Stream.of()) { // Noncompliant {{Remove this unused "stream3" local variable.}} [[quickfixes=qf_tr2]]
+//              ^^^^^^^
+//              fix@qf_tr2 {{Replace unused local variable with _}}
+//              edit@qf_tr2 [[sc=10;ec=24]] {{var _}}
     } catch (Exception _) {
     }
 

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

@@ -1,5 +1,7 @@
 package checks.unused;
 
+import java.util.stream.Stream;
+
 public class UnusedLocalVariableCheck_java22 {
   private UnusedLocalVariableCheck_java22() {}
 
@@ -21,4 +23,18 @@ public static int count(int[] elements) {
 
     return count;
   }
+
+  public static void tryWithResources() {
+    try (Stream foo = Stream.of()) { // Noncompliant {{Remove this unused "foo" local variable.}} [[quickfixes=qf_tr1]]
+//              ^^^
+//              fix@qf_tr1 {{Replace unused local variable with _}}
+//              edit@qf_tr1 [[sc=10;ec=20]] {{var _}}
+    } catch (Exception e) {
+    }
+
+    Stream bar = Stream.of();
+    try(bar) {
+    } catch (Exception e) {
+    }
+  }
 }

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

@@ -0,0 +1,30 @@
+package checks.unused;
+
+
+public class UnusedLocalVariableCheck_withLambda {
+
+  public void subjectTryWithResources() {
+    try (org.assertj.core.api.AutoCloseableSoftAssertions softlyTry = new org.assertj.core.api.AutoCloseableSoftAssertions()) { // Noncompliant
+    }
+  }
+
+  public void subjectEnhancedFor() {
+    for (org.assertj.core.api.AutoCloseableSoftAssertions softlyFor : new org.assertj.core.api.AutoCloseableSoftAssertions[]{}) { // Noncompliant
+    }
+  }
+
+  // Commenting out decoy methods (or renaming the arguments in lambdas)
+  // changes the outcome of the tests without semantics.
+
+  public void decoyTryWithResources() {
+    org.assertj.core.api.SoftAssertions.assertSoftly(softlyTry -> {
+      softlyTry.assertThat(5).isLessThan(3);
+    });
+  }
+
+  public void decoyEnhancedFor() {
+    org.assertj.core.api.SoftAssertions.assertSoftly(softlyFor -> {
+      softlyFor.assertThat(5).isLessThan(3);
+    });
+  }
+}

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

@@ -29,7 +29,6 @@
 import org.sonar.java.reporting.JavaQuickFix;
 import org.sonar.java.reporting.JavaTextEdit;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
-import org.sonar.plugins.java.api.JavaVersion;
 import org.sonar.plugins.java.api.semantic.Symbol;
 import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
 import org.sonar.plugins.java.api.tree.CaseLabelTree;
@@ -76,7 +75,7 @@ public void leaveNode(Tree tree) {
       IdentifierTree simpleName = variable.simpleName();
       if (!simpleName.isUnnamedVariable()) {
         boolean unresolved = UNRESOLVED_IDENTIFIERS_AND_SWITCH_CASE_VISITOR.isUnresolved(simpleName.name());
-        if (!unresolved && isProperLocalVariable(variable) && isUnused(variable.symbol()) && !isMandatoryForeachVariable(context.getJavaVersion(), variable)) {
+        if (!unresolved && isProperLocalVariable(variable) && isUnused(variable.symbol()) && canBeReplaced(variable)) {
           QuickFixHelper.newIssue(context)
             .forRule(this)
             .onTree(simpleName)
@@ -90,11 +89,12 @@ public void leaveNode(Tree tree) {
   }
 
   /**
-   * Before Java 22 it was not possible to remove the variable in a foreach statement even it is unused.
+   * Before Java 22 it was not possible to remove the variable in a foreach statement or try with resources even it is unused.
    * For instance in {@code for (String element : list) {}}, it is only since Java 22 that it can be rewritten {@code for (var _ : list) {}}.
    */
-  private static boolean isMandatoryForeachVariable(JavaVersion javaVersion, VariableTree variable) {
-    return variable.parent() instanceof ForEachStatement && !javaVersion.isJava22Compatible();
+  private boolean canBeReplaced(VariableTree variable) {
+    return context.getJavaVersion().isJava22Compatible()
+      || (!isForeachVariable(variable) && !isTryResource(variable));
   }
 
   private static boolean isUnused(Symbol symbol) {
@@ -119,7 +119,6 @@ private static boolean isProperLocalVariable(VariableTree variable) {
     return symbol.isLocalVariable()
       && !symbol.isParameter()
       && !isDefinedInCatchClause(variable)
-      && !isTryResource(variable)
       && !UNRESOLVED_IDENTIFIERS_AND_SWITCH_CASE_VISITOR.isSwitchPatternVariable(variable);
   }
 
@@ -131,9 +130,13 @@ private static boolean isTryResource(VariableTree variable) {
     return variable.parent().is(Tree.Kind.LIST) && variable.parent().parent().is(Tree.Kind.TRY_STATEMENT);
   }
 
+  private static boolean isForeachVariable(VariableTree variable) {
+    return variable.parent() instanceof ForEachStatement;
+  }
+
   private static List<JavaQuickFix> computeQuickFix(VariableTree variable) {
-    if (variable.parent() instanceof ForEachStatement) {
-      return makeQuickFixReplacingWithUnnamedVariable(variable);
+    if (isForeachVariable(variable) || isTryResource(variable)) {
+      return List.of(makeQuickFixReplacingWithUnnamedVariable(variable));
     }
     return getQuickFixTextSpan(variable).map(textSpan -> Collections.singletonList(
         JavaQuickFix.newQuickFix("Remove unused local variable")
@@ -143,11 +146,14 @@ private static List<JavaQuickFix> computeQuickFix(VariableTree variable) {
     ).orElseGet(Collections::emptyList);
   }
 
-  private static List<JavaQuickFix> makeQuickFixReplacingWithUnnamedVariable(VariableTree variable) {
-    var textSpan = AnalyzerMessage.textSpanBetween(variable.firstToken(), variable.lastToken());
-    return List.of(JavaQuickFix.newQuickFix("Replace unused local variable with _")
-      .addTextEdit(JavaTextEdit.replaceTextSpan(textSpan, "var _"))
-      .build());
+  private static JavaQuickFix makeQuickFixReplacingWithUnnamedVariable(VariableTree variable) {
+    return JavaQuickFix.newQuickFix("Replace unused local variable with _")
+      // This works with both enhanced for loop and try-with-resources.
+      // In the latter case we keep the initializer:
+      //   `for(int elem: elems)`  turns into  `for(var _: elems)`
+      //   `try(Resource res = initializer())`  turns into  `try(var _ = initializer ())`
+      .addTextEdit(JavaTextEdit.replaceBetweenTree(variable.type(), true, variable.simpleName(), true, "var _"))
+      .build();
   }
 
   private static Optional<AnalyzerMessage.TextSpan> getQuickFixTextSpan(VariableTree variable) {

java-checks/src/test/java/org/sonar/java/checks/unused/UnusedLocalVariableCheckTest.java

@@ -59,4 +59,27 @@ void test_non_compiling() {
       .verifyIssues();
   }
 
+  @Test
+  void test_with_lambda() {
+    CheckVerifier.newVerifier()
+      .onFile(TestUtils.mainCodeSourcesPath("checks/unused/UnusedLocalVariableCheck_withLambda.java"))
+      .withCheck(new UnusedLocalVariableCheck())
+      .withJavaVersion(22)
+      .verifyIssues();
+  }
+
+  /**
+   * Test for false negative when a name is used in a lambda expression.
+   * See SONARJAVA-5504 for details.
+   */
+  @Test
+  void test_with_lambda_without_semantics() {
+    CheckVerifier.newVerifier()
+      .onFile(TestUtils.mainCodeSourcesPath("checks/unused/UnusedLocalVariableCheck_withLambda.java"))
+      .withCheck(new UnusedLocalVariableCheck())
+      .withJavaVersion(22)
+      .withoutSemantic()
+      // False negatives.
+      .verifyNoIssues();
+  }
 }