diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiers.java new file mode 100644 index 0000000000..825b94873a --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiers.java @@ -0,0 +1,111 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.NONE; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.STYLE; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.hasAnnotation; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.refaster.annotation.AfterTemplate; +import com.google.errorprone.refaster.annotation.BeforeTemplate; +import com.google.errorprone.refaster.annotation.Placeholder; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import java.util.EnumSet; +import java.util.Set; +import javax.lang.model.element.Modifier; + +/** + * A {@link BugChecker} which suggests a canonical set of modifiers for Refaster class and method + * definitions. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Refaster class and method definitions should specify a canonical set of modifiers", + linkType = NONE, + severity = SUGGESTION, + tags = STYLE) +public final class RefasterTemplateModifiers extends BugChecker + implements ClassTreeMatcher, MethodTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher BEFORE_TEMPLATE_METHOD = hasAnnotation(BeforeTemplate.class); + private static final Matcher AFTER_TEMPLATE_METHOD = hasAnnotation(AfterTemplate.class); + private static final Matcher PLACEHOLDER_METHOD = hasAnnotation(Placeholder.class); + private static final Matcher REFASTER_METHOD = + anyOf(BEFORE_TEMPLATE_METHOD, AFTER_TEMPLATE_METHOD, PLACEHOLDER_METHOD); + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + if (!hasMatchingMember(tree, BEFORE_TEMPLATE_METHOD, state)) { + /* This class does not contain a Refaster template. */ + return Description.NO_MATCH; + } + + SuggestedFix fix = suggestCanonicalModifiers(tree, state); + return fix.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fix); + } + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (!REFASTER_METHOD.matches(tree, state)) { + return Description.NO_MATCH; + } + + return SuggestedFixes.removeModifiers( + tree, + state, + Modifier.FINAL, + Modifier.PRIVATE, + Modifier.PROTECTED, + Modifier.PUBLIC, + Modifier.STATIC, + Modifier.SYNCHRONIZED) + .map(fix -> describeMatch(tree, fix)) + .orElse(Description.NO_MATCH); + } + + private static SuggestedFix suggestCanonicalModifiers(ClassTree tree, VisitorState state) { + Set modifiersToAdd = EnumSet.noneOf(Modifier.class); + Set modifiersToRemove = + EnumSet.of(Modifier.PRIVATE, Modifier.PROTECTED, Modifier.PUBLIC, Modifier.SYNCHRONIZED); + + if (!hasMatchingMember(tree, PLACEHOLDER_METHOD, state)) { + /* + * Templates without a `@Placeholder` method should be `final`. Note that Refaster enforces + * that `@Placeholder` methods are `abstract`, so templates _with_ such a method will + * naturally be `abstract` and non-`final`. + */ + modifiersToAdd.add(Modifier.FINAL); + modifiersToRemove.add(Modifier.ABSTRACT); + } + + if (ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class) != null) { + /* Nested classes should be `static`. */ + modifiersToAdd.add(Modifier.STATIC); + } + + SuggestedFix.Builder fix = SuggestedFix.builder(); + SuggestedFixes.addModifiers(tree, tree.getModifiers(), state, modifiersToAdd) + .ifPresent(fix::merge); + SuggestedFixes.removeModifiers(tree.getModifiers(), state, modifiersToRemove) + .ifPresent(fix::merge); + return fix.build(); + } + + private static boolean hasMatchingMember( + ClassTree tree, Matcher matcher, VisitorState state) { + return tree.getMembers().stream().anyMatch(member -> matcher.matches(member, state)); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableListMultimapTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableListMultimapTemplates.java index 0174bae0b1..e1da7db817 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableListMultimapTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/ImmutableListMultimapTemplates.java @@ -141,8 +141,8 @@ ImmutableListMultimap after( } /** - * Don't map a a stream's elements to map entries, only to subsequently collect them into an - * {@link ImmutableListMultimap}. The collection can be performed directly. + * Don't map stream's elements to map entries, only to subsequently collect them into an {@link + * ImmutableListMultimap}. The collection can be performed directly. */ abstract static class StreamOfMapEntriesToImmutableListMultimap { @Placeholder(allowsIdentity = true) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java index 1e216ed121..ac11f9d939 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java @@ -153,7 +153,7 @@ Optional after(T input) { * Prefer {@link Optional#filter(Predicate)} over {@link Optional#map(Function)} when converting * an {@link Optional} to a boolean. */ - abstract static class MapOptionalToBoolean { + static final class MapOptionalToBoolean { @BeforeTemplate boolean before(Optional optional, Function predicate) { return optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE)); @@ -315,7 +315,7 @@ Optional after( } /** Prefer {@link Optional#or(Supplier)} over more verbose alternatives. */ - abstract static class OptionalOrOtherOptional { + static final class OptionalOrOtherOptional { @BeforeTemplate @SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */) Optional before(Optional optional1, Optional optional2) { diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/WebClientTemplates.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/WebClientTemplates.java index f22dc49c54..57b5cb850b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/WebClientTemplates.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/WebClientTemplates.java @@ -174,7 +174,7 @@ RequestBodyUriSpec after(WebClient webClient) { } /** Don't unnecessarily use {@link RequestHeadersUriSpec#uri(Function)}. */ - abstract static class RequestHeadersUriSpecUri { + static final class RequestHeadersUriSpecUri { @BeforeTemplate RequestHeadersSpec before( RequestHeadersUriSpec requestHeadersUriSpec, diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiersTest.java new file mode 100644 index 0000000000..574f4081b8 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiersTest.java @@ -0,0 +1,208 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class RefasterTemplateModifiersTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(RefasterTemplateModifiers.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(RefasterTemplateModifiers.class, getClass()); + + @Test + void identification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "", + "final class A {", + " @BeforeTemplate", + " String before(String str) {", + " return str;", + " }", + "", + " String nonRefasterMethod(String str) {", + " return str;", + " }", + "", + " static final class Inner {", + " @BeforeTemplate", + " String before(String str) {", + " return str;", + " }", + " }", + "}") + .addSourceLines( + "B.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "import com.google.errorprone.refaster.annotation.Placeholder;", + "", + "abstract class B {", + " @Placeholder", + " abstract O someFunction(I input);", + "", + " @BeforeTemplate", + " String before(I input) {", + " return String.valueOf(someFunction(input));", + " }", + "", + " abstract static class Inner {", + " @Placeholder", + " abstract O someFunction(I input);", + "", + " @BeforeTemplate", + " String before(I input) {", + " return String.valueOf(someFunction(input));", + " }", + " }", + "}") + .addSourceLines( + "C.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "", + "// BUG: Diagnostic contains:", + "class C {", + " @BeforeTemplate", + " // BUG: Diagnostic contains:", + " final String beforeFinal(String str) {", + " return str;", + " }", + "", + " @BeforeTemplate", + " // BUG: Diagnostic contains:", + " private String beforePrivate(String str) {", + " return str;", + " }", + "", + " @BeforeTemplate", + " // BUG: Diagnostic contains:", + " public String beforePublic(String str) {", + " return str;", + " }", + "", + " @BeforeTemplate", + " // BUG: Diagnostic contains:", + " static String beforeStatic(String str) {", + " return str;", + " }", + "", + " @BeforeTemplate", + " // BUG: Diagnostic contains:", + " synchronized String beforeSynchronized(String str) {", + " return str;", + " }", + "", + " // BUG: Diagnostic contains:", + " abstract static class AbstractInner {", + " @BeforeTemplate", + " String before(String str) {", + " return str;", + " }", + " }", + "", + " // BUG: Diagnostic contains:", + " static class NonFinalInner {", + " @BeforeTemplate", + " String before(String str) {", + " return str;", + " }", + " }", + "", + " // BUG: Diagnostic contains:", + " final class NonStaticInner {", + " @BeforeTemplate", + " String before(String str) {", + " return str;", + " }", + " }", + "}") + .addSourceLines( + "D.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "", + "// BUG: Diagnostic contains:", + "abstract class D {", + " @BeforeTemplate", + " // BUG: Diagnostic contains:", + " protected String beforeProtected(String str) {", + " return str;", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + refactoringTestHelper + .addInputLines( + "A.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "", + "class A {", + " @BeforeTemplate", + " private static String before(String str) {", + " return str;", + " }", + "}") + .addOutputLines( + "A.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "", + "final class A {", + " @BeforeTemplate", + " String before(String str) {", + " return str;", + " }", + "}") + .addInputLines( + "B.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "import com.google.errorprone.refaster.annotation.Placeholder;", + "", + "final class B {", + " abstract class WithoutPlaceholder {", + " @BeforeTemplate", + " protected synchronized String before(String str) {", + " return str;", + " }", + " }", + "", + " abstract class WithPlaceholder {", + " @Placeholder", + " public abstract O someFunction(I input);", + "", + " @BeforeTemplate", + " public final String before(I input) {", + " return String.valueOf(someFunction(input));", + " }", + " }", + "}") + .addOutputLines( + "B.java", + "import com.google.errorprone.refaster.annotation.BeforeTemplate;", + "import com.google.errorprone.refaster.annotation.Placeholder;", + "", + "final class B {", + " static final class WithoutPlaceholder {", + " @BeforeTemplate", + " String before(String str) {", + " return str;", + " }", + " }", + "", + " abstract static class WithPlaceholder {", + " @Placeholder", + " abstract O someFunction(I input);", + "", + " @BeforeTemplate", + " String before(I input) {", + " return String.valueOf(someFunction(input));", + " }", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +}