diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java index 4bdc29593ae..06f860c68a0 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructor.java @@ -15,7 +15,6 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; -import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.MultiMatcher; import com.google.errorprone.util.ASTHelpers; @@ -24,6 +23,7 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; import java.util.List; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** A {@link BugChecker} that flags redundant {@code @Autowired} constructor annotations. */ @AutoService(BugChecker.class) @@ -62,6 +62,6 @@ public Description matchClass(ClassTree tree, VisitorState state) { * leave flagging the unused import to Error Prone's `RemoveUnusedImports` check. */ AnnotationTree annotation = Iterables.getOnlyElement(annotations); - return describeMatch(annotation, SuggestedFix.delete(annotation)); + return describeMatch(annotation, SourceCode.deleteWithTrailingWhitespace(annotation, state)); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java index a6706cd7a84..0de1b28c89c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java @@ -14,7 +14,6 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; -import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; @@ -22,6 +21,7 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; import java.util.Optional; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** A {@link BugChecker} that flags empty methods that seemingly can simply be deleted. */ @AutoService(BugChecker.class) @@ -55,7 +55,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) { return Description.NO_MATCH; } - return describeMatch(tree, SuggestedFix.delete(tree)); + return describeMatch(tree, SourceCode.deleteWithTrailingWhitespace(tree, state)); } private static boolean isInPossibleTestHelperClass(VisitorState state) { diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/SourceCode.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/SourceCode.java index 9691a8a2f20..065d023efbc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/SourceCode.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/SourceCode.java @@ -1,7 +1,12 @@ package tech.picnic.errorprone.bugpatterns.util; +import static com.sun.tools.javac.util.Position.NOPOS; + +import com.google.common.base.CharMatcher; import com.google.errorprone.VisitorState; +import com.google.errorprone.fixes.SuggestedFix; import com.sun.source.tree.Tree; +import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; /** * A collection of Error Prone utility methods for dealing with the source code representation of @@ -9,6 +14,9 @@ */ // XXX: Can we locate this code in a better place? Maybe contribute it upstream? public final class SourceCode { + /** The complement of {@link CharMatcher#whitespace()}. */ + private static final CharMatcher NON_WHITESPACE_MATCHER = CharMatcher.whitespace().negate(); + private SourceCode() {} /** @@ -24,4 +32,31 @@ public static String treeToString(Tree tree, VisitorState state) { String src = state.getSourceForNode(tree); return src != null ? src : tree.toString(); } + + /** + * Creates a {@link SuggestedFix} for the deletion of the given {@link Tree}, including any + * whitespace that follows it. + * + *

Removing trailing whitespace may prevent the introduction of an empty line at the start of a + * code block; such empty lines are not removed when formatting the code using Google Java Format. + * + * @param tree The AST node of interest. + * @param state A {@link VisitorState} describing the context in which the given {@link Tree} is + * found. + * @return A non-{@code null} {@link SuggestedFix} similar to one produced by {@link + * SuggestedFix#delete(Tree)}. + */ + public static SuggestedFix deleteWithTrailingWhitespace(Tree tree, VisitorState state) { + CharSequence sourceCode = state.getSourceCode(); + int endPos = state.getEndPosition(tree); + if (sourceCode == null || endPos == NOPOS) { + /* We can't identify the trailing whitespace; delete just the tree. */ + return SuggestedFix.delete(tree); + } + + int actualEnd = NON_WHITESPACE_MATCHER.indexIn(sourceCode, endPos); + return actualEnd == -1 + ? SuggestedFix.delete(tree) + : SuggestedFix.replace(((DiagnosticPosition) tree).getStartPosition(), actualEnd, ""); + } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructorTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructorTest.java index 459e765b72c..ad63927d6aa 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructorTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/AutowiredConstructorTest.java @@ -94,13 +94,11 @@ void replacement() { "", "interface Container {", " class A {", - "", " @Deprecated", " A() {}", " }", "", " class B {", - "", " B(String x) {}", " }", "}") diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EmptyMethodTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EmptyMethodTest.java index e1fdbc360e6..e7a4bc6f10f 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EmptyMethodTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EmptyMethodTest.java @@ -76,8 +76,18 @@ void replacement() { " void instanceMethod() {}", "", " static void staticMethod() {}", + "", + " static void staticMethodWithComment() {", + " /* Foo. */", + " }", + "}") + .addOutputLines( + "A.java", + "final class A {", + " static void staticMethodWithComment() {", + " /* Foo. */", + " }", "}") - .addOutputLines("A.java", "final class A {}") .doTest(TestMode.TEXT_MATCH); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/SourceCodeTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/SourceCodeTest.java new file mode 100644 index 00000000000..54aed84e131 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/SourceCodeTest.java @@ -0,0 +1,198 @@ +package tech.picnic.errorprone.bugpatterns.util; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import javax.lang.model.element.Name; +import org.junit.jupiter.api.Test; + +final class SourceCodeTest { + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass()); + + @Test + void deleteWithTrailingWhitespaceAnnotations() { + refactoringTestHelper + .addInputLines("AnnotationToBeDeleted.java", "@interface AnnotationToBeDeleted {}") + .expectUnchanged() + .addInputLines( + "AnotherAnnotationToBeDeleted.java", "@interface AnotherAnnotationToBeDeleted {}") + .expectUnchanged() + .addInputLines( + "AnnotationDeletions.java", + "", + "interface AnnotationDeletions {", + " class SoleAnnotation {", + " @AnnotationToBeDeleted", + " void m() {}", + " }", + "", + " class FirstAnnotation {", + " @AnnotationToBeDeleted", + " @Deprecated", + " void m() {}", + " }", + "", + " class MiddleAnnotation {", + " @Deprecated", + " @AnnotationToBeDeleted", + " @SuppressWarnings(\"foo\")", + " void m() {}", + " }", + "", + " class LastAnnotation {", + " @Deprecated", + " @AnnotationToBeDeleted", + " void m() {}", + " }", + "", + " class MultipleAnnotations {", + " @AnnotationToBeDeleted", + " @AnotherAnnotationToBeDeleted", + " @Deprecated", + " void m() {}", + " }", + "}") + .addOutputLines( + "AnnotationDeletions.java", + "", + "interface AnnotationDeletions {", + " class SoleAnnotation {", + " void m() {}", + " }", + "", + " class FirstAnnotation {", + " @Deprecated", + " void m() {}", + " }", + "", + " class MiddleAnnotation {", + " @Deprecated", + " @SuppressWarnings(\"foo\")", + " void m() {}", + " }", + "", + " class LastAnnotation {", + " @Deprecated", + " void m() {}", + " }", + "", + " class MultipleAnnotations {", + " @Deprecated", + " void m() {}", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void deleteWithTrailingWhitespaceMethods() { + refactoringTestHelper + .addInputLines( + "MethodDeletions.java", + "", + "interface MethodDeletions {", + " class SoleMethod {", + " void methodToBeDeleted() {}", + " }", + "", + " class FirstMethod {", + " void methodToBeDeleted() {}", + "", + " void finalMethod() {}", + " }", + "", + " class MiddleMethod {", + " void initialMethod() {}", + "", + " void methodToBeDeleted() {}", + "", + " void finalMethod() {}", + " }", + "", + " class LastMethod {", + " void initialMethod() {}", + "", + " void methodToBeDeleted() {}", + " }", + "", + " class MultipleMethods {", + " void method1ToBeDeleted() {}", + "", + " void method2ToBeDeleted() {}", + "", + " void middleMethod() {}", + "", + " void method3ToBeDeleted() {}", + "", + " void method4ToBeDeleted() {}", + " }", + "}") + .addOutputLines( + "MethodDeletions.java", + "", + "interface MethodDeletions {", + " class SoleMethod {}", + "", + " class FirstMethod {", + " void finalMethod() {}", + " }", + "", + " class MiddleMethod {", + " void initialMethod() {}", + "", + " void finalMethod() {}", + " }", + "", + " class LastMethod {", + " void initialMethod() {}", + " }", + "", + " class MultipleMethods {", + " void middleMethod() {}", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + /** + * Uses {@link SourceCode#deleteWithTrailingWhitespace(Tree, VisitorState)} to suggest the + * deletion of annotations and methods with a name containing {@value DELETION_MARKER}. + */ + @BugPattern(severity = ERROR, summary = "Interacts with `SourceCode` for testing purposes") + public static final class TestChecker extends BugChecker + implements AnnotationTreeMatcher, MethodTreeMatcher { + private static final long serialVersionUID = 1L; + private static final String DELETION_MARKER = "ToBeDeleted"; + + @Override + public Description matchAnnotation(AnnotationTree tree, VisitorState state) { + return match( + tree, + ASTHelpers.getAnnotationMirror(tree).getAnnotationType().asElement().getSimpleName(), + state); + } + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + return match(tree, tree.getName(), state); + } + + private Description match(Tree tree, Name name, VisitorState state) { + return name.toString().contains(DELETION_MARKER) + ? describeMatch(tree, SourceCode.deleteWithTrailingWhitespace(tree, state)) + : Description.NO_MATCH; + } + } +}