From d256e097e675fb649e767af4dc0ae48c5897f8b0 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Mon, 27 Mar 2023 04:35:36 +0200 Subject: [PATCH 1/2] Fix UnusedVariable false positives for private record parameters --- .../bugpatterns/UnusedVariable.java | 13 ++- .../bugpatterns/UnusedVariableTest.java | 95 ++++++++++++++++++- 2 files changed, 105 insertions(+), 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 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()); From 453577851e17586b463e1b2894b3fa46d5ee1a53 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sun, 13 Aug 2023 14:16:07 +0200 Subject: [PATCH 2/2] Address review feedback --- .../java/com/google/errorprone/bugpatterns/UnusedVariable.java | 2 +- .../com/google/errorprone/bugpatterns/UnusedVariableTest.java | 2 +- 2 files changed, 2 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 674a9214b8e..cd41fac6081 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java @@ -627,7 +627,7 @@ && exemptedFieldBySuperType(getType(variableTree), state)) { // 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)) { + if (hasRecordFlag(symbol.owner)) { return null; } unusedElements.put(symbol, getCurrentPath()); 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 004ab6db757..aaa01a89922 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();