Skip to content

Commit

Permalink
Improve Tree deletion suggestions (#347)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Stephan202 committed Nov 21, 2022
1 parent 1b6356a commit 98185b9
Show file tree
Hide file tree
Showing 6 changed files with 250 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
Expand Down Expand Up @@ -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));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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() {}

/**
Expand All @@ -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,
"");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,11 @@ void replacement() {
"",
"interface Container {",
" class A {",
"",
" @Deprecated",
" A() {}",
" }",
"",
" class B {",
"",
" B(String x) {}",
" }",
"}")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
}

0 comments on commit 98185b9

Please sign in to comment.