From 26fbd194c81c91f1632c996010f89d7bd6200cef Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 1 Nov 2019 19:22:34 +0000 Subject: [PATCH 1/3] Add generated changelog entries --- changelog/@unreleased/pr-1014.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-1014.v2.yml 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 From b99e9b2647e29a2940dde90263c4cb831af6f7cd Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 1 Nov 2019 15:22:34 -0400 Subject: [PATCH 2/3] Fix `RedundantModifier` interpretation of implicit modifiers Updated RedundantModifier to check explicit modifiers only, added test coverage for all output. --- .../errorprone/RedundantModifier.java | 24 +++-- .../errorprone/RedundantModifierTest.java | 92 +++++++++++++++++++ 2 files changed, 109 insertions(+), 7 deletions(-) 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..408bafa3d 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 @@ -48,30 +48,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 +115,14 @@ public Description matchMethod(MethodTree tree, VisitorState state) { } return Description.NO_MATCH; } + + private static Matcher methodHasExplicitModifier(Modifier modifier) { + return (Matcher) (methodTree, state) -> + methodTree.getModifiers().getFlags().contains(modifier); + } + + private static Matcher classHasExplicitModifier(Modifier modifier) { + return (Matcher) (classTree, state) -> + classTree.getModifiers().getFlags().contains(modifier); + } } 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..15f558974 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,20 @@ void fixStaticInterface() { ).doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } + @Test + void allowInterface() { + helper().addSourceLines( + "Test.java", + "public enum Test {", + " INSTANCE(\"str\");", + " private final String str;", + " Test(String str) {", + " this.str = str;", + " }", + "}" + ).doTest(); + } + @Test void fixInterfaceMethods() { fix() @@ -117,6 +158,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 +197,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 +244,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()); + } } From 53d9c7c9426ca0d27f7bd805fdaf0dabd6f42322 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 1 Nov 2019 15:48:27 -0400 Subject: [PATCH 3/3] fix nested interfaces --- .../errorprone/RedundantModifier.java | 17 ++++++++++++++-- .../errorprone/RedundantModifierTest.java | 20 +++++++++++++------ 2 files changed, 29 insertions(+), 8 deletions(-) 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 408bafa3d..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; @@ -118,11 +119,23 @@ public Description matchMethod(MethodTree tree, VisitorState state) { private static Matcher methodHasExplicitModifier(Modifier modifier) { return (Matcher) (methodTree, state) -> - methodTree.getModifiers().getFlags().contains(modifier); + containsModifier(methodTree.getModifiers(), state, modifier); } private static Matcher classHasExplicitModifier(Modifier modifier) { return (Matcher) (classTree, state) -> - classTree.getModifiers().getFlags().contains(modifier); + 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 15f558974..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 @@ -120,12 +120,20 @@ void fixStaticInterface() { @Test void allowInterface() { helper().addSourceLines( - "Test.java", - "public enum Test {", - " INSTANCE(\"str\");", - " private final String str;", - " Test(String str) {", - " this.str = str;", + "Enclosing.java", + "public class Enclosing {", + " public interface Test {", + " }", + "}" + ).doTest(); + } + + @Test + void allowNestedInterface() { + helper().addSourceLines( + "Enclosing.java", + "interface Enclosing {", + " public interface Test {", " }", "}" ).doTest();