Skip to content

Commit

Permalink
Make BooleanParameter match constructors too.
Browse files Browse the repository at this point in the history
And add a couple of fairly common terms to the list of parameter names
to ignore.

RELNOTES: Make BooleanParameter match constructors.

MOE_MIGRATED_REVID=211982804
  • Loading branch information
graememorgan authored and cushon committed Sep 13, 2018
1 parent 9da085c commit ad60a03
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 12 deletions.
Expand Up @@ -28,6 +28,7 @@
import com.google.errorprone.BugPattern.ProvidesFix; import com.google.errorprone.BugPattern.ProvidesFix;
import com.google.errorprone.VisitorState; import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
import com.google.errorprone.bugpatterns.argumentselectiondefects.NamedParameterComment; import com.google.errorprone.bugpatterns.argumentselectiondefects.NamedParameterComment;
import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Description;
Expand All @@ -37,6 +38,8 @@
import com.google.errorprone.util.ErrorProneTokens; import com.google.errorprone.util.ErrorProneTokens;
import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.Tree.Kind;
import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol;
Expand All @@ -51,36 +54,48 @@
summary = "Use parameter comments to document ambiguous literals", summary = "Use parameter comments to document ambiguous literals",
severity = SUGGESTION, severity = SUGGESTION,
providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION) providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION)
public class BooleanParameter extends BugChecker implements MethodInvocationTreeMatcher { public class BooleanParameter extends BugChecker
implements MethodInvocationTreeMatcher, NewClassTreeMatcher {

private static final ImmutableSet<String> EXCLUDED_NAMES =
ImmutableSet.of("default", "defValue", "value");


@Override @Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
List<? extends ExpressionTree> arguments = tree.getArguments(); handleArguments(tree, tree.getArguments(), state);
if (arguments.size() < 2) { return NO_MATCH;
}

@Override
public Description matchNewClass(NewClassTree tree, VisitorState state) {
handleArguments(tree, tree.getArguments(), state);
return NO_MATCH;
}

private void handleArguments(
Tree tree, List<? extends ExpressionTree> arguments, VisitorState state) {
if (arguments.size() < 2 && tree instanceof MethodInvocationTree) {
// single-argument methods are often self-documentating // single-argument methods are often self-documentating
return NO_MATCH; return;
} }
if (arguments.stream().noneMatch(BooleanParameter::isBooleanLiteral)) { if (arguments.stream().noneMatch(BooleanParameter::isBooleanLiteral)) {
return NO_MATCH; return;
} }
MethodSymbol sym = ASTHelpers.getSymbol(tree); MethodSymbol sym = (MethodSymbol) ASTHelpers.getSymbol(tree);
if (NamedParameterComment.containsSyntheticParameterName(sym)) { if (NamedParameterComment.containsSyntheticParameterName(sym)) {
return NO_MATCH; return;
} }
int start = ((JCTree) tree).getStartPosition(); int start = ((JCTree) tree).getStartPosition();
int end = state.getEndPosition(getLast(tree.getArguments())); int end = state.getEndPosition(getLast(arguments));
String source = state.getSourceCode().subSequence(start, end).toString(); String source = state.getSourceCode().subSequence(start, end).toString();
Deque<ErrorProneToken> tokens = Deque<ErrorProneToken> tokens =
new ArrayDeque<>(ErrorProneTokens.getTokens(source, state.context)); new ArrayDeque<>(ErrorProneTokens.getTokens(source, state.context));
forEachPair( forEachPair(
sym.getParameters().stream(), sym.getParameters().stream(),
arguments.stream(), arguments.stream(),
(p, c) -> checkParameter(p, c, start, tokens, state)); (p, c) -> checkParameter(p, c, start, tokens, state));
return NO_MATCH;
} }


private static final ImmutableSet<String> BLACKLIST = ImmutableSet.of("value");

private void checkParameter( private void checkParameter(
VarSymbol paramSym, VarSymbol paramSym,
ExpressionTree a, ExpressionTree a,
Expand All @@ -95,7 +110,7 @@ private void checkParameter(
// single-character parameter names aren't helpful // single-character parameter names aren't helpful
return; return;
} }
if (BLACKLIST.contains(name)) { if (EXCLUDED_NAMES.contains(name)) {
return; return;
} }
while (!tokens.isEmpty() while (!tokens.isEmpty()
Expand Down
Expand Up @@ -33,6 +33,7 @@ public void refactoring() {
.addInputLines( .addInputLines(
"in/Test.java", "in/Test.java",
"class Test {", "class Test {",
" Test(boolean foo) {}",
" void f(boolean foo) {}", " void f(boolean foo) {}",
" void f(boolean foo, boolean bar) {}", " void f(boolean foo, boolean bar) {}",
" void g(boolean p, boolean q) {}", " void g(boolean p, boolean q) {}",
Expand All @@ -44,11 +45,13 @@ public void refactoring() {
" f(false, false);", " f(false, false);",
" g(false, false); // single-char", " g(false, false); // single-char",
" h(false, false); // synthetic", " h(false, false); // synthetic",
" new Test(false);",
" }", " }",
"}") "}")
.addOutputLines( .addOutputLines(
"out/Test.java", "out/Test.java",
"class Test {", "class Test {",
" Test(boolean foo) {}",
" void f(boolean foo) {}", " void f(boolean foo) {}",
" void f(boolean foo, boolean bar) {}", " void f(boolean foo, boolean bar) {}",
" void g(boolean p, boolean q) {}", " void g(boolean p, boolean q) {}",
Expand All @@ -60,6 +63,7 @@ public void refactoring() {
" f(/* foo= */ false, /* bar= */ false);", " f(/* foo= */ false, /* bar= */ false);",
" g(false, false); // single-char", " g(false, false); // single-char",
" h(false, false); // synthetic", " h(false, false); // synthetic",
" new Test(/* foo= */ false);",
" }", " }",
"}") "}")
.doTest(TEXT_MATCH); .doTest(TEXT_MATCH);
Expand Down

0 comments on commit ad60a03

Please sign in to comment.