Fix UnusedVariable false positives for private record parameters

google · Mar 27, 2023 · a6bd7f6 · a6bd7f6
1 parent 861377b
commit a6bd7f6
Show file tree

Hide file tree

Showing 2 changed files with 105 additions and 3 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
@@ -617,6 +617,13 @@ && exemptedFieldBySuperType(getType(variableTree), state)) {
           if (variableTree.getName().contentEquals("this")) {
             return null;
           }
+          // Ignore if parameter is part of canonical record constructor; tree does not seem
+          // to contain usage in that case, but parameter is always used implicitly
+          // For compact canonical constructor parameters don't have record flag so need to
+          // check constructor flags (`symbol.owner`) instead
+          if (hasRecordFlag(symbol) || hasRecordFlag(symbol.owner)) {
+            return null;
+          }
           unusedElements.put(symbol, getCurrentPath());
           if (!isParameterSubjectToAnalysis(symbol)) {
             onlyCheckForReassignments.add(symbol);
@@ -639,14 +646,18 @@ private boolean isFieldEligibleForChecking(VariableTree variableTree, VarSymbol
           && ASTHelpers.hasDirectAnnotationWithSimpleName(variableTree, "Inject")) {
         return true;
       }
-      if ((symbol.flags() & RECORD_FLAG) == RECORD_FLAG) {
+      if (hasRecordFlag(symbol)) {
         return false;
       }
       return canBeRemoved(symbol) && !SPECIAL_FIELDS.contains(symbol.getSimpleName().toString());
     }
 
     private static final long RECORD_FLAG = 1L << 61;
 
+    private static boolean hasRecordFlag(Symbol symbol) {
+      return (symbol.flags() & RECORD_FLAG) == RECORD_FLAG;
+    }
+
     /** Returns whether {@code sym} can be removed without updating call sites in other files. */
     private boolean isParameterSubjectToAnalysis(Symbol sym) {
       checkArgument(sym.getKind() == ElementKind.PARAMETER);

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java
@@ -1385,7 +1385,7 @@ public void simpleRecord() {
     assumeTrue(RuntimeVersion.isAtLeast16());
     helper
         .addSourceLines(
-            "SimpleRecord.java", //
+            "SimpleRecord.java",
             "public record SimpleRecord (Integer foo, Long bar) {}")
         .expectNoDiagnostics()
         .doTest();
@@ -1398,12 +1398,103 @@ public void nestedRecord() {
         .addSourceLines(
             "SimpleClass.java",
             "public class SimpleClass {",
-            "   public record SimpleRecord (Integer foo, Long bar) {}",
+            "  public record SimpleRecord (Integer foo, Long bar) {}",
             "}")
         .expectNoDiagnostics()
         .doTest();
   }
 
+  @Test
+  public void recordWithStaticFields() {
+    assumeTrue(RuntimeVersion.isAtLeast16());
+    helper
+        .addSourceLines(
+            "SimpleClass.java",
+            "public class SimpleClass {",
+            "  public record MyRecord (int foo) {",
+            "    private static int a = 1;",
+            "    private static int b = 1;",
+            "    // BUG: Diagnostic contains: is never read",
+            "    private static int c = 1;",
+            "    ",
+            "    public MyRecord {",
+            "      foo = Math.max(a, foo);",
+            "    }",
+            "  }",
+            "",
+            "  public int b() {",
+            "    return MyRecord.b;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  // Implicit canonical constructor has same access as record (https://docs.oracle.com/javase/specs/jls/se17/html/jls-8.html#jls-8.10.4)
+  // Therefore this case is important to test because UnusedVariable treats parameters of private methods differently
+  @Test
+  public void nestedPrivateRecord() {
+    assumeTrue(RuntimeVersion.isAtLeast16());
+    helper
+        .addSourceLines(
+            "SimpleClass.java",
+            "public class SimpleClass {",
+            "  private record SimpleRecord (Integer foo, Long bar) {}",
+            "}")
+        .expectNoDiagnostics()
+        .doTest();
+  }
+
+  @Test
+  public void nestedPrivateRecordCompactCanonicalConstructor() {
+    assumeTrue(RuntimeVersion.isAtLeast16());
+    helper
+        .addSourceLines(
+            "SimpleClass.java",
+            "public class SimpleClass {",
+            "  private record SimpleRecord (Integer foo, Long bar) {",
+            // Compact canonical constructor implicitly assigns field values at end
+            "    private SimpleRecord {",
+            "      System.out.println(foo);",
+            "    }",
+            "  }",
+            "}")
+        .expectNoDiagnostics()
+        .doTest();
+  }
+
+  @Test
+  public void nestedPrivateRecordNormalCanonicalConstructor() {
+    assumeTrue(RuntimeVersion.isAtLeast16());
+    helper
+        .addSourceLines(
+            "SimpleClass.java",
+            "public class SimpleClass {",
+            "  private record SimpleRecord (Integer foo, Long bar) {",
+            "    private SimpleRecord(Integer foo, Long bar) {",
+            "      this.foo = foo;",
+            "      this.bar = bar;",
+            "    }",
+            "  }",
+            "}")
+        .expectNoDiagnostics()
+        .doTest();
+  }
+
+  @Test
+  public void unusedRecordConstructorParameter() {
+    assumeTrue(RuntimeVersion.isAtLeast16());
+    helper
+        .addSourceLines(
+            "SimpleRecord.java",
+            "public record SimpleRecord (int x) {",
+            "  // BUG: Diagnostic contains: The parameter 'b' is never read",
+            "  private SimpleRecord(int a, int b) {",
+            "    this(a);",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void unusedInRecord() {
     assumeTrue(RuntimeVersion.isAtLeast16());