Improve Tree deletion suggestions

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));
   }
 }

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java

@@ -14,14 +14,14 @@
 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;
 import com.sun.source.tree.ClassTree;
 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) {

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/SourceCode.java

@@ -1,14 +1,21 @@
 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
  * AST nodes.
  */
-// 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 +31,32 @@ 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.
+   *
+   * <p>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 whitespaceEndPos = NON_WHITESPACE_MATCHER.indexIn(sourceCode, endPos);
+    return SuggestedFix.replace(
+        ((DiagnosticPosition) tree).getStartPosition(),
+        whitespaceEndPos == -1 ? sourceCode.length() : whitespaceEndPos,
+        "");
+  }
 }

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) {}",
             "  }",
             "}")

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);
   }
 }

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/SourceCodeTest.java

@@ -0,0 +1,199 @@
+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);
+  }
+
+  /**
+   * A {@link BugChecker} that 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;
+    }
+  }
+}