Skip to content

Commit

Permalink
Fix RedundantModifier interpretation of implicit modifiers (#1014)
Browse files Browse the repository at this point in the history 
Fix `RedundantModifier` interpretation of implicit modifiers
  • Loading branch information
@carterkozak @bulldozer-bot
carterkozak authored and bulldozer-bot[bot] committed Nov 1, 2019
1 parent e9be1c0 commit 1ceef96
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 7 deletions.
37 changes: 30 additions & 7 deletions baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantModifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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));
}
}
100 changes: 100 additions & 0 deletions ...ine-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantModifierTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand All @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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());
}
}
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-1014.v2.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: fix
fix:
description: Fix `RedundantModifier` interpretation of implicit modifiers
links:
- https://github.com/palantir/gradle-baseline/pull/1014

0 comments on commit 1ceef96

Please sign in to comment.