Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix UnusedVariable false positives for private record parameters #4049

Closed
wants to merge 1 commit into from
Closed

Fix UnusedVariable false positives for private record parameters #4049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -645,14 +652,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 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);
Expand Down
95 changes: 94 additions & 1 deletion core/src/test/java/com/google/errorprone/bugpatterns/UnusedVariableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down