diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java index 93a1285ab62..74d74e393e0 100644 --- 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,7 +646,7 @@ 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()); @@ -647,6 +654,10 @@ private boolean isFieldEligibleForChecking(VariableTree variableTree, VarSymbol private static final long RECORD_FLAG = 1L << 61; + private 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 index 853d37485a1..0201ea7304b 100644 --- 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());