Error prone RedundantModifier check supports more interface components

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreMatchers.java

@@ -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 {
@@ -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() {}
 }

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantModifier.java

@@ -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;
 
@@ -45,38 +45,65 @@
         severity = BugPattern.SeverityLevel.ERROR,
         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)
@@ -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.")
+                    .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.")
@@ -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;
     }
 }

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantModifierTest.java

@@ -145,7 +145,7 @@ void allowNestedInterface() {
         helper().addSourceLines(
                 "Enclosing.java",
                 "interface Enclosing {",
-                "  public interface Test {",
+                "  interface Test {",
                 "  }",
                 "}"
         ).doTest();
@@ -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());
     }

changelog/@unreleased/pr-1021.v2.yml

@@ -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