Skip to content

Commit

Permalink
Suggest canonical modifier usage for Refaster template definitions (#254
Browse files Browse the repository at this point in the history 
)
  • Loading branch information
@Stephan202
Stephan202 authored Sep 29, 2022
1 parent 2ba7bf9 commit 397f9c3
Show file tree
Hide file tree
Showing 5 changed files with 324 additions and 5 deletions.
111 changes: 111 additions & 0 deletions ...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiers.java
View file
Original file line number Diff line number Diff line change
@@ -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<Tree> BEFORE_TEMPLATE_METHOD = hasAnnotation(BeforeTemplate.class);
private static final Matcher<Tree> AFTER_TEMPLATE_METHOD = hasAnnotation(AfterTemplate.class);
private static final Matcher<Tree> PLACEHOLDER_METHOD = hasAnnotation(Placeholder.class);
private static final Matcher<Tree> 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<Modifier> modifiersToAdd = EnumSet.noneOf(Modifier.class);
Set<Modifier> 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<Tree> matcher, VisitorState state) {
return tree.getMembers().stream().anyMatch(member -> matcher.matches(member, state));
}
}
4 changes: 2 additions & 2 deletions ...rc/main/java/tech/picnic/errorprone/refastertemplates/ImmutableListMultimapTemplates.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,8 @@ ImmutableListMultimap<K, V> 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<E, K, V> {
@Placeholder(allowsIdentity = true)
Expand Down
4 changes: 2 additions & 2 deletions ...one-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/OptionalTemplates.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ Optional<T> 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<T> {
static final class MapOptionalToBoolean<T> {
@BeforeTemplate
boolean before(Optional<T> optional, Function<? super T, Boolean> predicate) {
return optional.map(predicate).orElse(Refaster.anyOf(false, Boolean.FALSE));
Expand Down Expand Up @@ -315,7 +315,7 @@ Optional<R> after(
}

/** Prefer {@link Optional#or(Supplier)} over more verbose alternatives. */
abstract static class OptionalOrOtherOptional<T> {
static final class OptionalOrOtherOptional<T> {
@BeforeTemplate
@SuppressWarnings("NestedOptionals" /* Auto-fix for the `NestedOptionals` check. */)
Optional<T> before(Optional<T> optional1, Optional<T> optional2) {
Expand Down
2 changes: 1 addition & 1 deletion ...ne-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/WebClientTemplates.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
208 changes: 208 additions & 0 deletions ...ntrib/src/test/java/tech/picnic/errorprone/bugpatterns/RefasterTemplateModifiersTest.java
View file
Original file line number Diff line number Diff line change
@@ -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<I, O> {",
" @Placeholder",
" abstract O someFunction(I input);",
"",
" @BeforeTemplate",
" String before(I input) {",
" return String.valueOf(someFunction(input));",
" }",
"",
" abstract static class Inner<I, O> {",
" @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<I, O> {",
" @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<I, O> {",
" @Placeholder",
" abstract O someFunction(I input);",
"",
" @BeforeTemplate",
" String before(I input) {",
" return String.valueOf(someFunction(input));",
" }",
" }",
"}")
.doTest(TestMode.TEXT_MATCH);
}
}

0 comments on commit 397f9c3

Please sign in to comment.