Fix RedundantModifier interpretation of implicit modifiers

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

@@ -26,6 +26,7 @@
 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 java.util.Locale;
 import javax.lang.model.element.Modifier;
@@ -48,30 +49,30 @@ public final class RedundantModifier extends BugChecker
 
     private static final Matcher<ClassTree> STATIC_ENUM_OR_INTERFACE = Matchers.allOf(
             Matchers.anyOf(Matchers.kindIs(Tree.Kind.ENUM), Matchers.kindIs(Tree.Kind.INTERFACE)),
-            Matchers.hasModifier(Modifier.STATIC));
+            classHasExplicitModifier(Modifier.STATIC));
 
     private static final Matcher<MethodTree> PRIVATE_ENUM_CONSTRUCTOR = Matchers.allOf(
             Matchers.methodIsConstructor(),
             Matchers.enclosingClass(Matchers.kindIs(Tree.Kind.ENUM)),
-            Matchers.hasModifier(Modifier.PRIVATE));
+            methodHasExplicitModifier(Modifier.PRIVATE));
 
     private static final Matcher<MethodTree> STATIC_FINAL_METHOD = Matchers.allOf(
-            Matchers.isStatic(),
-            Matchers.hasModifier(Modifier.FINAL));
+            methodHasExplicitModifier(Modifier.STATIC),
+            methodHasExplicitModifier(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(Matchers.hasModifier(Modifier.PUBLIC), Matchers.hasModifier(Modifier.ABSTRACT)));
+            Matchers.anyOf(methodHasExplicitModifier(Modifier.PUBLIC), methodHasExplicitModifier(Modifier.ABSTRACT)));
 
     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),
-                    Matchers.hasModifier(Modifier.FINAL))),
+                    classHasExplicitModifier(Modifier.FINAL))),
             Matchers.allOf(
-                    Matchers.hasModifier(Modifier.FINAL),
+                    methodHasExplicitModifier(Modifier.FINAL),
                     Matchers.not(Matchers.hasAnnotation(SafeVarargs.class))));
 
     @Override
@@ -115,4 +116,26 @@ 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);
+        if (source == null) {
+            return true; // When source is not available, rely on the modifier flags
+        }
+        // nested interfaces report a static modifier despite not being present
+        return source.contains(modifier.name().toLowerCase(Locale.ENGLISH));
+    }
 }

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

@@ -17,6 +17,7 @@
 package com.palantir.baseline.errorprone;
 
 import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.CompilationTestHelper;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.parallel.Execution;
 import org.junit.jupiter.api.parallel.ExecutionMode;
@@ -49,6 +50,20 @@ void fixEnumConstructor() {
                 ).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
     }
 
+    @Test
+    void allowEnumResult() {
+        helper().addSourceLines(
+                "Test.java",
+                "public enum Test {",
+                "  INSTANCE(\"str\");",
+                "  private final String str;",
+                "  Test(String str) {",
+                "    this.str = str;",
+                "  }",
+                "}"
+        ).doTest();
+    }
+
     @Test
     @SuppressWarnings("checkstyle:RegexpSinglelineJava")
     void fixStaticEnum() {
@@ -71,6 +86,18 @@ void fixStaticEnum() {
                 ).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
     }
 
+    @Test
+    void testAllowPrivateEnum() {
+        helper().addSourceLines(
+                "Enclosing.java",
+                "public class Enclosing {",
+                "  private enum Test {",
+                "    INSTANCE",
+                "  }",
+                "}"
+        ).doTest();
+    }
+
     @Test
     void fixStaticInterface() {
         fix()
@@ -90,6 +117,28 @@ void fixStaticInterface() {
                 ).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
     }
 
+    @Test
+    void allowInterface() {
+        helper().addSourceLines(
+                "Enclosing.java",
+                "public class Enclosing {",
+                "  public interface Test {",
+                "  }",
+                "}"
+        ).doTest();
+    }
+
+    @Test
+    void allowNestedInterface() {
+        helper().addSourceLines(
+                "Enclosing.java",
+                "interface Enclosing {",
+                "  public interface Test {",
+                "  }",
+                "}"
+        ).doTest();
+    }
+
     @Test
     void fixInterfaceMethods() {
         fix()
@@ -117,6 +166,21 @@ void fixInterfaceMethods() {
                 ).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
     }
 
+    @Test
+    void allowValidInterfaceMethods() {
+        helper().addSourceLines(
+                "Enclosing.java",
+                "public class Enclosing {",
+                "  public interface Test {",
+                "    int a();",
+                "    int b();",
+                "    int c();",
+                "    int d();",
+                "  }",
+                "}"
+        ).doTest();
+    }
+
     @Test
     void fixFinalClassModifiers() {
         fix()
@@ -141,6 +205,20 @@ void fixFinalClassModifiers() {
                 ).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
     }
 
+    @Test
+    void allowFinalClass() {
+        helper().addSourceLines(
+                "Test.java",
+                "public final class Test {",
+                "  public void a() {}",
+                "  private void b() {}",
+                "  void c() {}",
+                // SafeVarargs is a special case
+                "  @SafeVarargs public final void d(Object... value) {}",
+                "}"
+        ).doTest();
+    }
+
     @Test
     void fixStaticFinalMethod() {
         fix()
@@ -174,7 +252,29 @@ void fixStaticFinalMethod() {
                 ).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
     }
 
+    @Test
+    void allowStaticMethods() {
+        helper().addSourceLines(
+                "Test.java",
+                "public class Test {",
+                "  public static int a() {",
+                "    return 1;",
+                "  }",
+                "  private static int b() {",
+                "    return 1;",
+                "  }",
+                "  static int c() {",
+                "    return 1;",
+                "  }",
+                "}"
+        ).doTest();
+    }
+
     private BugCheckerRefactoringTestHelper fix() {
         return BugCheckerRefactoringTestHelper.newInstance(new RedundantModifier(), getClass());
     }
+
+    private CompilationTestHelper helper() {
+        return CompilationTestHelper.newInstance(RedundantModifier.class, getClass());
+    }
 }

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

@@ -0,0 +1,5 @@
+type: fix
+fix:
+  description: Fix `RedundantModifier` interpretation of implicit modifiers
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/1014