From 07cc7a855e1eb702ce182bcab899f8eec418ca1b Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 4 Nov 2019 14:42:46 -0500 Subject: [PATCH] Error prone RedundantModifier check supports more interface components ```diff public interface Iface { - public static void a() {} + static void a() {} } ``` ```diff public interface Iface { - public static final int VALUE = 1; + int VALUE = 1; } ``` --- .../errorprone/RedundantModifier.java | 41 ++++++++++++++++- .../errorprone/RedundantModifierTest.java | 44 +++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) 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 53fb370036..080173bbcd 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 @@ -28,6 +28,8 @@ 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,7 +47,7 @@ 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 STATIC_ENUM_OR_INTERFACE = Matchers.allOf( Matchers.anyOf(Matchers.kindIs(Tree.Kind.ENUM), Matchers.kindIs(Tree.Kind.INTERFACE)), @@ -66,6 +68,19 @@ public final class RedundantModifier extends BugChecker Matchers.not(Matchers.hasModifier(Modifier.DEFAULT)), Matchers.anyOf(methodHasExplicitModifier(Modifier.PUBLIC), methodHasExplicitModifier(Modifier.ABSTRACT))); + private static final Matcher INTERFACE_STATIC_METHOD_MODIFIERS = Matchers.allOf( + Matchers.enclosingClass(Matchers.kindIs(Tree.Kind.INTERFACE)), + Matchers.isStatic(), + methodHasExplicitModifier(Modifier.PUBLIC)); + + private static final Matcher INTERFACE_FIELD_MODIFIERS = Matchers.allOf( + Matchers.enclosingClass(Matchers.kindIs(Tree.Kind.INTERFACE)), + Matchers.isStatic(), + Matchers.anyOf( + variableHasExplicitModifier(Modifier.PUBLIC), + variableHasExplicitModifier(Modifier.STATIC), + variableHasExplicitModifier(Modifier.FINAL))); + private static final Matcher UNNECESSARY_FINAL_METHOD_ON_FINAL_CLASS = Matchers.allOf( Matchers.not(Matchers.isStatic()), Matchers.enclosingClass(Matchers.allOf( @@ -108,6 +123,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,6 +138,19 @@ public Description matchMethod(MethodTree tree, VisitorState state) { return Description.NO_MATCH; } + @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(); + } + return Description.NO_MATCH; + } + private static Matcher methodHasExplicitModifier(Modifier modifier) { return (Matcher) (methodTree, state) -> containsModifier(methodTree.getModifiers(), state, modifier); @@ -127,6 +161,11 @@ private static Matcher classHasExplicitModifier(Modifier modifier) { containsModifier(classTree.getModifiers(), state, modifier); } + private static Matcher variableHasExplicitModifier(Modifier modifier) { + return (Matcher) (variableTree, state) -> + containsModifier(variableTree.getModifiers(), state, modifier); + } + private static boolean containsModifier(ModifiersTree tree, VisitorState state, Modifier modifier) { if (!tree.getFlags().contains(modifier)) { return false; 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 37d714dfc9..2eba766213 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 @@ -282,6 +282,50 @@ 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); + } + private RefactoringValidator fix() { return RefactoringValidator.of(new RedundantModifier(), getClass()); }