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 missing parameter name detection, autofix unused to avoid human intervention #1729

Merged
merged 2 commits into from
Apr 15, 2021
Merged

Fix missing parameter name detection, autofix unused to avoid human intervention #1729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82b2c17
Fix missing parameter name detection, autofix unused
carterkozak Apr 14, 2021
964055c
O(n^2) beats O(n) for small values of n
carterkozak Apr 14, 2021
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
136 changes: 95 additions & 41 deletions baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ConsistentOverrides.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,12 @@
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import org.immutables.value.Value.Immutable;
Expand All @@ -50,61 +47,118 @@
summary = "Method overrides should have variable names consistent with the super-method when there "
+ "are multiple parameters with the same type to avoid incorrectly binding values to variables.")
public final class ConsistentOverrides extends BugChecker implements MethodTreeMatcher {
private static final Pattern UNKNOWN_ARG_NAME = Pattern.compile("^args\\d+$");

@Override
public Description matchMethod(MethodTree tree, VisitorState state) {
Symbol sym = ASTHelpers.getSymbol(tree);
if (!(sym instanceof MethodSymbol) || sym.isStatic()) {
MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree);
List<? extends VariableTree> methodParameters = tree.getParameters();
if (methodSymbol == null
|| methodSymbol.isStatic()
|| methodSymbol.isPrivate()
|| methodParameters.size() <= 1
|| !hasDuplicateTypes(methodParameters, state)) {
return Description.NO_MATCH;
}

MethodSymbol methodSymbol = (MethodSymbol) sym;
if (methodSymbol.params().size() <= 1) {
return Description.NO_MATCH;
}

List<? extends VariableTree> paramTrees = tree.getParameters();
getNonParameterizedSuperMethod(methodSymbol, state.getTypes())
.filter(ConsistentOverrides::hasMeaningfulArgNames)
.ifPresent(superMethod -> {
Map<Type, List<ParamEntry>> superParamsByType = IntStream.range(0, paramTrees.size())
.mapToObj(i -> ParamEntry.of(superMethod.params().get(i), i))
.collect(Collectors.groupingBy(ParamEntry::type));

superParamsByType.values().forEach(params -> {
if (params.size() <= 1) {
return;
}

for (ParamEntry expectedParam : params) {
int index = expectedParam.index();
String param = methodSymbol.params.get(index).name.toString();
if (equivalentNames(param, expectedParam.name())) {
continue;
.filter(ConsistentOverrides::retainedParameterNames)
.ifPresent(superMethod -> IntStream.range(0, methodParameters.size())
.mapToObj(i -> ParamEntry.of(superMethod.params().get(i), i))
.collect(Collectors.groupingBy(ParamEntry::type))
.values()
.forEach(params -> {
if (params.size() <= 1) {
return;
}

state.reportMatch(buildDescription(tree)
.addFix(SuggestedFixes.renameVariable(
paramTrees.get(index), expectedParam.name(), state))
.build());
}
});
});
for (ParamEntry expectedParam : params) {
int index = expectedParam.index();
// Use the parameter VariableTree name which always retains name information
VariableTree parameter = methodParameters.get(index);
String parameterName = parameter.getName().toString();
String expectedParameterName = expectedParam.name();
if (!isMeaninglessParameterName(expectedParameterName)
&& !equivalentNames(parameterName, expectedParameterName)) {
state.reportMatch(buildDescription(tree)
.addFix(SuggestedFixes.renameVariable(
methodParameters.get(index),
retainUnderscore(parameterName, expectedParameterName),
state))
.build());
}
}
}));

return Description.NO_MATCH;
}

/**
* {@code O(n^2)} check for duplicate types. Methods tend to have very few arguments so
* performance is better than using a {@link java.util.Set} because n is generally less than 100.
*/
private static boolean hasDuplicateTypes(List<? extends VariableTree> methodParameters, VisitorState state) {
for (int i = 0; i < methodParameters.size() - 1; i++) {
Type firstType = ASTHelpers.getType(methodParameters.get(i));
for (int j = i + 1; j < methodParameters.size(); j++) {
if (state.getTypes().isSameType(firstType, ASTHelpers.getType(methodParameters.get(j)))) {
return true;
}
}
}
return false;
}

// Match the original names underscore prefix to appease StrictUnusedVariable
private static String retainUnderscore(String originalName, String newName) {
boolean originalUnderscore = originalName.startsWith("_");
boolean newUnderscore = newName.startsWith("_");
if (originalUnderscore == newUnderscore) {
return newName;
}
if (!originalUnderscore) {
return newName.substring(1);
} else {
return "_" + newName;
}
}

private static boolean equivalentNames(String actual, String expected) {
return actual.equals(expected)
|| (actual.charAt(0) == '_' && actual.equals("_" + expected))
|| (expected.charAt(0) == '_' && expected.equals("_" + actual));
// Handle StrictUnusedVariable underscore prefixes in both directions
|| (actual.charAt(0) == '_' && actual.length() == expected.length() + 1 && actual.endsWith(expected))
|| (expected.charAt(0) == '_' && expected.length() == actual.length() + 1 && expected.endsWith(actual));
}

// If any parameters have names that don't match 'arg\d+', names are retained.
private static boolean retainedParameterNames(MethodSymbol methodSymbol) {
for (VarSymbol parameter : methodSymbol.params()) {
if (!isUnknownParameterName(parameter.name)) {
return true;
}
}
return methodSymbol.params().isEmpty();
}

private static boolean isMeaninglessParameterName(CharSequence input) {
// Single character names are unhelpful for readers, and should be overridden in implementations
return input.length() <= 1
// Implementation may not retain parameter names, we shouldn't create churn in this case.
|| isUnknownParameterName(input);
}

private static boolean hasMeaningfulArgNames(MethodSymbol methodSymbol) {
return !methodSymbol.params().stream()
.allMatch(symbol -> symbol.name.length() == 1
|| UNKNOWN_ARG_NAME.matcher(symbol.name).matches());
// Returns true if code was compiled without javac '-parameters'
private static boolean isUnknownParameterName(CharSequence input) {
int length = input.length();
if (length > 3 && input.charAt(0) == 'a' && input.charAt(1) == 'r' && input.charAt(2) == 'g') {
for (int i = 3; i < length; i++) {
char current = input.charAt(i);
if (!Character.isDigit(current)) {
return false;
}
}
return true;
}
return false;
}

private static Optional<MethodSymbol> getNonParameterizedSuperMethod(MethodSymbol methodSymbol, Types types) {
Expand Down
62 changes: 61 additions & 1 deletion ...e-error-prone/src/test/java/com/palantir/baseline/errorprone/ConsistentOverridesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ void ignores_unhelpfulNames() {
}

@Test
void ignores_unused_variables() {
void allows_unused_variables() {
fix().addInputLines(
"Test.java",
"import " + List.class.getCanonicalName() + ";",
Expand All @@ -114,6 +114,35 @@ void ignores_unused_variables() {
.doTest();
}

@Test
void allows_unused_variable_names() {
fix().addInputLines(
"Test.java",
"import " + List.class.getCanonicalName() + ";",
"class Test {",
" interface Foo {",
" void doStuff(String foo, String bar);",
" }",
" class DefaultFoo implements Foo {",
" @Override",
" public void doStuff(String foo, String _baz) {}",
" }",
"}")
.addOutputLines(
"Test.java",
"import " + List.class.getCanonicalName() + ";",
"class Test {",
" interface Foo {",
" void doStuff(String foo, String bar);",
" }",
" class DefaultFoo implements Foo {",
" @Override",
" public void doStuff(String foo, String _bar) {}",
" }",
"}")
.doTest();
}

@Test
void allows_unused_variables_to_be_used() {
fix().addInputLines(
Expand Down Expand Up @@ -215,6 +244,37 @@ void extended_interface() {
.doTest();
}

@Test
void interface_has_one_meaningless_name() {
fix().addInputLines(
"Test.java",
"class Test {",
" interface Foo {",
" boolean doStuff(String a, String bar);",
" }",
" class DefaultFoo implements Foo {",
" @Override",
" public boolean doStuff(String bang, String foo) {",
" return bang.equals(foo);",
" }",
" }",
"}")
.addOutputLines(
"Test.java",
"class Test {",
" interface Foo {",
" boolean doStuff(String a, String bar);",
" }",
" class DefaultFoo implements Foo {",
" @Override",
" public boolean doStuff(String bang, String bar) {",
" return bang.equals(bar);",
" }",
" }",
"}")
.doTest();
}

private RefactoringValidator fix() {
return RefactoringValidator.of(new ConsistentOverrides(), getClass());
}
Expand Down