From 1ceef966f1295405ac7f9931e47760094e901e77 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 1 Nov 2019 16:52:24 -0400 Subject: [PATCH] Fix `RedundantModifier` interpretation of implicit modifiers (#1014) Fix `RedundantModifier` interpretation of implicit modifiers --- .../errorprone/RedundantModifier.java | 37 +++++-- .../errorprone/RedundantModifierTest.java | 100 ++++++++++++++++++ changelog/@unreleased/pr-1014.v2.yml | 5 + 3 files changed, 135 insertions(+), 7 deletions(-) create mode 100644 changelog/@unreleased/pr-1014.v2.yml diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantModifier.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantModifier.java index 482d05a08..6f8f271e5 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantModifier.java +++ b/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 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 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 STATIC_FINAL_METHOD = Matchers.allOf( - Matchers.isStatic(), - Matchers.hasModifier(Modifier.FINAL)); + methodHasExplicitModifier(Modifier.STATIC), + methodHasExplicitModifier(Modifier.FINAL)); private static final Matcher 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 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 methodHasExplicitModifier(Modifier modifier) { + return (Matcher) (methodTree, state) -> + containsModifier(methodTree.getModifiers(), state, modifier); + } + + private static Matcher classHasExplicitModifier(Modifier modifier) { + return (Matcher) (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)); + } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantModifierTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantModifierTest.java index 34b531224..a2b838212 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantModifierTest.java +++ b/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()); + } } diff --git a/changelog/@unreleased/pr-1014.v2.yml b/changelog/@unreleased/pr-1014.v2.yml new file mode 100644 index 000000000..d1ac7315a --- /dev/null +++ b/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