From 48383773184b8dd2204c71f5fa87934d7b6863dc Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 13 Nov 2022 12:59:03 +0100 Subject: [PATCH 1/3] Improve `Tree` deletion suggestions When suggesting to remove a method or method annotation, also remove any trailing whitespace. This avoids the possible introduction of an empty line right at the start of a code block. --- .../bugpatterns/AutowiredConstructor.java | 4 +- .../errorprone/bugpatterns/EmptyMethod.java | 4 +- .../bugpatterns/util/SourceCode.java | 35 ++++ .../bugpatterns/AutowiredConstructorTest.java | 2 - .../bugpatterns/EmptyMethodTest.java | 12 +- .../bugpatterns/util/SourceCodeTest.java | 198 ++++++++++++++++++ 6 files changed, 248 insertions(+), 7 deletions(-) create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/SourceCodeTest.java 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 4bdc29593a..06f860c68a 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 a6706cd7a8..0de1b28c89 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 9691a8a2f2..065d023efb 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 459e765b72..ad63927d6a 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 e1fdbc360e..e7a4bc6f10 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 0000000000..54aed84e13 --- /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; + } + } +} From bcf76bd9010f693e56a7481d307ed47b8be71220 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 19 Nov 2022 11:04:45 +0100 Subject: [PATCH 2/3] Address feedback --- .../tech/picnic/errorprone/bugpatterns/util/SourceCode.java | 1 - .../picnic/errorprone/bugpatterns/util/SourceCodeTest.java | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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 065d023efb..ab40083198 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 @@ -12,7 +12,6 @@ * 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(); 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 index 54aed84e13..56668a1506 100644 --- 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 @@ -167,8 +167,9 @@ void deleteWithTrailingWhitespaceMethods() { } /** - * Uses {@link SourceCode#deleteWithTrailingWhitespace(Tree, VisitorState)} to suggest the - * deletion of annotations and methods with a name containing {@value DELETION_MARKER}. + * 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 From bdb60eec320907a4e75d4011e5594ef884ca096b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 20 Nov 2022 15:21:12 +0100 Subject: [PATCH 3/3] Tweak --- .../picnic/errorprone/bugpatterns/util/SourceCode.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) 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 ab40083198..dcdf755d3b 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 @@ -53,9 +53,10 @@ public static SuggestedFix deleteWithTrailingWhitespace(Tree tree, VisitorState return SuggestedFix.delete(tree); } - int actualEnd = NON_WHITESPACE_MATCHER.indexIn(sourceCode, endPos); - return actualEnd == -1 - ? SuggestedFix.delete(tree) - : SuggestedFix.replace(((DiagnosticPosition) tree).getStartPosition(), actualEnd, ""); + int whitespaceEndPos = NON_WHITESPACE_MATCHER.indexIn(sourceCode, endPos); + return SuggestedFix.replace( + ((DiagnosticPosition) tree).getStartPosition(), + whitespaceEndPos == -1 ? sourceCode.length() : whitespaceEndPos, + ""); } }