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.

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

Error prone RedundantModifier check supports more interface components #1021

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,17 @@

package com.palantir.baseline.errorprone;

import com.google.errorprone.VisitorState;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.ModifiersTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath;
import java.util.Locale;
import javax.lang.model.element.Modifier;

/** Additional {@link Matcher} factory methods shared by baseline checks. */
final class MoreMatchers {
Expand Down Expand Up @@ -49,5 +57,57 @@ static <T extends Tree> Matcher<T> isSubtypeOf(String baseTypeString) {
Matchers.not(Matchers.kindIs(Tree.Kind.NULL_LITERAL)));
}

/**
* Matches enclosing classes on {@link ClassTree} blocks. This differs from
* {@link Matchers#enclosingClass(Matcher)} which matches the input {@link ClassTree},
* not the enclosing class.
*/
static <T extends ClassTree> Matcher<T> classEnclosingClass(Matcher<ClassTree> matcher) {
return (Matcher<T>) (classTree, state) -> {
TreePath currentPath = state.getPath().getParentPath();
while (currentPath != null) {
Tree leaf = currentPath.getLeaf();
if (leaf instanceof ClassTree) {
return matcher.matches((ClassTree) leaf, state);
}
currentPath = currentPath.getParentPath();
}
return false;
};
}

/**
* Works similarly to {@link Matchers#hasModifier(Modifier)}, but only matches elements
* which explicitly list the modifier. For example, all components nested in an interface
* are public by default, but they don't necessarily use the public keyword.
*/
static <T extends Tree> Matcher<T> hasExplicitModifier(Modifier modifier) {
return (Matcher<T>) (tree, state) -> {
if (tree instanceof ClassTree) {
return containsModifier(((ClassTree) tree).getModifiers(), state, modifier);
}
if (tree instanceof MethodTree) {
return containsModifier(((MethodTree) tree).getModifiers(), state, modifier);
}
if (tree instanceof VariableTree) {
return containsModifier(((VariableTree) tree).getModifiers(), state, modifier);
}
return false;
};
}

private static boolean containsModifier(ModifiersTree tree, VisitorState state, Modifier modifier) {
if (!tree.getFlags().contains(modifier)) {
return false;
}
String source = state.getSourceForNode(tree);
// getSourceForNode returns null when there are no modifiers specified
if (source == null) {
return false;
}
// nested interfaces report a static modifier despite not being present
return source.contains(modifier.name().toLowerCase(Locale.ENGLISH));
}

private MoreMatchers() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
import com.google.errorprone.matchers.Matchers;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.ModifiersTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.VariableTree;
import java.util.Locale;
import javax.lang.model.element.Modifier;

Expand All @@ -45,38 +45,65 @@
severity = BugPattern.SeverityLevel.ERROR,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm a tiny bit concerned that having this at ERROR level might introduce unnecessary friction when people just want to run things like ./gradlew test locally. We're just detecting redundant code, not actually 'wrong' in anywa way... given that excavator can come around and auto-fix, what do you think about putting this as suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I initially used error because this in some part replaces a checkstyle check which we verify premerge.
Given that robots can fix this for us, and it's not a correctness issue, I don't mind moving it to a suggestion, and allowing minor regressions until the robots sweep through.

summary = "Avoid using redundant modifiers")
public final class RedundantModifier extends BugChecker
implements BugChecker.ClassTreeMatcher, BugChecker.MethodTreeMatcher {
implements BugChecker.ClassTreeMatcher, BugChecker.MethodTreeMatcher, BugChecker.VariableTreeMatcher {

private static final Matcher<ClassTree> STATIC_ENUM_OR_INTERFACE = Matchers.allOf(
Matchers.anyOf(Matchers.kindIs(Tree.Kind.ENUM), Matchers.kindIs(Tree.Kind.INTERFACE)),
classHasExplicitModifier(Modifier.STATIC));
MoreMatchers.hasExplicitModifier(Modifier.STATIC));

private static final Matcher<MethodTree> PRIVATE_ENUM_CONSTRUCTOR = Matchers.allOf(
Matchers.methodIsConstructor(),
Matchers.enclosingClass(Matchers.kindIs(Tree.Kind.ENUM)),
methodHasExplicitModifier(Modifier.PRIVATE));
MoreMatchers.hasExplicitModifier(Modifier.PRIVATE));

private static final Matcher<MethodTree> STATIC_FINAL_METHOD = Matchers.allOf(
methodHasExplicitModifier(Modifier.STATIC),
methodHasExplicitModifier(Modifier.FINAL));
MoreMatchers.hasExplicitModifier(Modifier.STATIC),
MoreMatchers.hasExplicitModifier(Modifier.FINAL));

private static final Matcher<MethodTree> UNNECESSARY_INTERFACE_METHOD_MODIFIERS = Matchers.allOf(
Matchers.enclosingClass(Matchers.kindIs(Tree.Kind.INTERFACE)),
Matchers.not(Matchers.isStatic()),
Matchers.not(Matchers.hasModifier(Modifier.DEFAULT)),
Matchers.anyOf(methodHasExplicitModifier(Modifier.PUBLIC), methodHasExplicitModifier(Modifier.ABSTRACT)));
Matchers.anyOf(
MoreMatchers.hasExplicitModifier(Modifier.PUBLIC),
MoreMatchers.hasExplicitModifier(Modifier.ABSTRACT)));

private static final Matcher<MethodTree> INTERFACE_STATIC_METHOD_MODIFIERS = Matchers.allOf(
Matchers.enclosingClass(Matchers.kindIs(Tree.Kind.INTERFACE)),
Matchers.isStatic(),
MoreMatchers.hasExplicitModifier(Modifier.PUBLIC));

private static final Matcher<VariableTree> INTERFACE_FIELD_MODIFIERS = Matchers.allOf(
Matchers.enclosingClass(Matchers.kindIs(Tree.Kind.INTERFACE)),
Matchers.isStatic(),
Matchers.anyOf(
MoreMatchers.hasExplicitModifier(Modifier.PUBLIC),
MoreMatchers.hasExplicitModifier(Modifier.STATIC),
MoreMatchers.hasExplicitModifier(Modifier.FINAL)));

private static final Matcher<ClassTree> INTERFACE_NESTED_CLASS_MODIFIERS = Matchers.allOf(
MoreMatchers.classEnclosingClass(Matchers.kindIs(Tree.Kind.INTERFACE)),
Matchers.anyOf(
MoreMatchers.hasExplicitModifier(Modifier.PUBLIC),
MoreMatchers.hasExplicitModifier(Modifier.STATIC)));

private static final Matcher<MethodTree> UNNECESSARY_FINAL_METHOD_ON_FINAL_CLASS = Matchers.allOf(
Matchers.not(Matchers.isStatic()),
Matchers.enclosingClass(Matchers.allOf(
Matchers.kindIs(Tree.Kind.CLASS),
classHasExplicitModifier(Modifier.FINAL))),
MoreMatchers.hasExplicitModifier(Modifier.FINAL))),
Matchers.allOf(
methodHasExplicitModifier(Modifier.FINAL),
MoreMatchers.hasExplicitModifier(Modifier.FINAL),
Matchers.not(Matchers.hasAnnotation(SafeVarargs.class))));

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
if (INTERFACE_NESTED_CLASS_MODIFIERS.matches(tree, state)) {
return buildDescription(tree)
.setMessage("Types nested in interfaces are public and static by default.")
.addFix(SuggestedFixes.removeModifiers(tree, state, Modifier.PUBLIC, Modifier.STATIC))
.build();
}
if (STATIC_ENUM_OR_INTERFACE.matches(tree, state)) {
return buildDescription(tree)
.setMessage(tree.getKind().name().toLowerCase(Locale.ENGLISH)
Expand Down Expand Up @@ -108,6 +135,12 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
.addFix(SuggestedFixes.removeModifiers(tree, state, Modifier.PUBLIC, Modifier.ABSTRACT))
.build();
}
if (INTERFACE_STATIC_METHOD_MODIFIERS.matches(tree, state)) {
return buildDescription(tree)
.setMessage("Interface components are public by default. The 'public' modifier is unnecessary.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting that java9 lets people define private methods in interfaces! https://bugs.openjdk.java.net/browse/JDK-8071453

This should be all fine though, because we're explicitly only detecting and fixing the public keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I designed this with JEP 213 in mind.

.addFix(SuggestedFixes.removeModifiers(tree, state, Modifier.PUBLIC))
.build();
}
if (UNNECESSARY_FINAL_METHOD_ON_FINAL_CLASS.matches(tree, state)) {
return buildDescription(tree)
.setMessage("Redundant 'final' modifier on an instance method of a final class.")
Expand All @@ -117,26 +150,16 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
return Description.NO_MATCH;
}

private static Matcher<MethodTree> methodHasExplicitModifier(Modifier modifier) {
return (Matcher<MethodTree>) (methodTree, state) ->
containsModifier(methodTree.getModifiers(), state, modifier);
}

private static Matcher<ClassTree> classHasExplicitModifier(Modifier modifier) {
return (Matcher<ClassTree>) (classTree, state) ->
containsModifier(classTree.getModifiers(), state, modifier);
}

private static boolean containsModifier(ModifiersTree tree, VisitorState state, Modifier modifier) {
if (!tree.getFlags().contains(modifier)) {
return false;
}
String source = state.getSourceForNode(tree);
// getSourceForNode returns null when there are no modifiers specified
if (source == null) {
return false;
@Override
public Description matchVariable(VariableTree tree, VisitorState state) {
if (INTERFACE_FIELD_MODIFIERS.matches(tree, state)) {
return buildDescription(tree)
.setMessage("Interface fields are public, static, and final by default. "
+ "These modifiers are unnecessary to specify.")
.addFix(SuggestedFixes.removeModifiers(
tree, state, Modifier.PUBLIC, Modifier.STATIC, Modifier.FINAL))
.build();
}
// nested interfaces report a static modifier despite not being present
return source.contains(modifier.name().toLowerCase(Locale.ENGLISH));
return Description.NO_MATCH;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ void allowNestedInterface() {
helper().addSourceLines(
"Enclosing.java",
"interface Enclosing {",
" public interface Test {",
" interface Test {",
" }",
"}"
).doTest();
Expand Down Expand Up @@ -282,6 +282,75 @@ void allowStaticMethods() {
).doTest();
}

@Test
void fixInterfacePublicStaticMethod() {
fix()
.addInputLines(
"Test.java",
"public interface Test {",
" public static void a() {}",
"}"
)
.addOutputLines(
"Test.java",
"public interface Test {",
" static void a() {}",
"}"
).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

@Test
void fixInterfaceStaticFieldModifiers() {
fix()
.addInputLines(
"Test.java",
"public interface Test {",
" int VALUE_0 = 0;",
" public static final int VALUE_1 = 1;",
" public final int VALUE_2 = 2;",
" public static int VALUE_3 = 3;",
" final int VALUE_4 = 4;",
" static int VALUE_5 = 5;",
"}"
)
.addOutputLines(
"Test.java",
"public interface Test {",
" int VALUE_0 = 0;",
" int VALUE_1 = 1;",
" int VALUE_2 = 2;",
" int VALUE_3 = 3;",
" int VALUE_4 = 4;",
" int VALUE_5 = 5;",
"}"
).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

@Test
void fixInterfaceNestedPublicClass() {
fix()
.addInputLines(
"Test.java",
"public interface Test {",
" public static final class Class0 {}",
" public class Class1 {}",
" static class Class2 {}",
" final class Class3 {}",
" public interface Interface0 {}",
"}"
)
.addOutputLines(
"Test.java",
"public interface Test {",
" final class Class0 {}",
" class Class1 {}",
" class Class2 {}",
" final class Class3 {}",
" interface Interface0 {}",
"}"
).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

private RefactoringValidator fix() {
return RefactoringValidator.of(new RedundantModifier(), getClass());
}
Expand Down
6 changes: 6 additions & 0 deletions changelog/@unreleased/pr-1021.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
type: improvement
improvement:
description: Error prone RedundantModifier check supports interface static methods
and fields.
links:
- https://github.com/palantir/gradle-baseline/pull/1021