From 24065a9ba499c207c28fde8db5a1bd0f02aa9754 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Thu, 17 Aug 2023 13:09:17 -0700 Subject: [PATCH] Fix UnusedVariable false positives for private record parameters Fixes #2713 It looks like the tree emitted by the compiler is incomplete for the canonical constructor of a record, which is why the parameters were erroneously flagged as unused. Apparently this only affected `private` record classes because for them the implicit canonical constructor is `private` as well, and the `UnusedVariable` check treats parameters of `private` methods and constructors differently. https://github.com/google/error-prone/issues/2713#issuecomment-1079038446 showed an example where this also affected record fields, however I was unable to reproduce that. Maybe they were using an older Error Prone version which does not include https://github.com/google/error-prone/commit/47da3af7490e6dbb214af58ea6460ae5823f50e3, their sample code is incomplete, or my test setup was incorrect. Edit: It looks like they were using an older Error Prone version, see https://github.com/google/error-prone/issues/2713#issuecomment-1489916690. Fixes #3837 COPYBARA_INTEGRATE_REVIEW=https://github.com/google/error-prone/pull/3837 from Marcono1234:record-unused-param-false-positives 453577851e17586b463e1b2894b3fa46d5ee1a53 PiperOrigin-RevId: 557914152 --- .../bugpatterns/UnusedVariable.java | 13 ++- .../bugpatterns/UnusedVariableTest.java | 95 ++++++++++++++++++- 2 files changed, 106 insertions(+), 2 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 index 99b4bc659f4..cd41fac6081 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java @@ -623,6 +623,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.owner)) { + return null; + } unusedElements.put(symbol, getCurrentPath()); if (!isParameterSubjectToAnalysis(symbol)) { onlyCheckForReassignments.add(symbol); @@ -645,7 +652,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()); @@ -653,6 +660,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 6a172a4e305..90abe1733e9 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java @@ -1398,12 +1398,105 @@ 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());