-
Notifications
You must be signed in to change notification settings - Fork 745
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.
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
account for java records when checking for unused variables #2515
Conversation
Fixes #1648 |
core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
Outdated
Show resolved
Hide resolved
@cushon Should be good for another pass. Test added to cover missing case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix, and the reminder. I'll get this merged soon
// Don't complain if the parent of this variable is a Record | ||
if(allUsageSites.stream().anyMatch(e -> e.getParentPath().getLeaf().getKind()== Tree.Kind.RECORD)){ | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this could miss actual unused variables inside record declarations. I'm going to try skipping the synthetic fields for record components with something like the following, but please let me know if you see any issues with that alternative:
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 3c347ccc9..26a13ced2 100644
--- a/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
+++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnusedVariable.java
@@ -85,6 +85,7 @@ import com.sun.source.util.SimpleTreeVisitor;
import com.sun.source.util.TreePath;
import com.sun.source.util.TreePathScanner;
import com.sun.source.util.TreeScanner;
+import com.sun.tools.javac.code.Flags;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.TypeSymbol;
@@ -610,10 +611,15 @@ public final class UnusedVariable extends BugChecker implements CompilationUnitT
&& ASTHelpers.hasDirectAnnotationWithSimpleName(variableTree, "Inject")) {
return true;
}
+ if ((symbol.flags() & RECORD_FLAG) == RECORD_FLAG) {
+ return false;
+ }
return variableTree.getModifiers().getFlags().contains(Modifier.PRIVATE)
&& !SPECIAL_FIELDS.contains(symbol.getSimpleName().toString());
}
+ private static final long RECORD_FLAG = 1L << 61;
+
/** Returns whether {@code sym} can be removed without updating call sites in other files. */
private boolean isParameterSubjectToAnalysis(Symbol sym) {
checkArgument(sym.getKind() == ElementKind.PARAMETER);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can't really be unused though right? Since they automatically get a public accessor method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Record components can't be unused, but I wanted to make it clearer that stuff like the following was still handled:
public record R (Integer foo, Long bar) {
void f() {
int x = foo; // unused
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. good catch.
No description provided.