From 13a1cd2f4b2b7010ef1f1d01a2097d3890d0a79b Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 14 Aug 2022 14:36:15 +0200 Subject: [PATCH] Update all tests to use text blocks All `CompilationTestHelper` and `BugCheckerRefactoringTestHelper` test code is now specified using a single text block. These changes were automated thanks to the new text block support added to the `ErrorProneTestHelperSourceFormat` check. --- README.md | 7 + .../ErrorProneTestHelperSourceFormat.java | 205 +++++++++++++++--- .../bugpatterns/util/SourceCode.java | 23 ++ .../bugpatterns/util/ThirdPartyLibrary.java | 6 +- .../ErrorProneTestHelperSourceFormatTest.java | 198 +++++++++++++++-- .../bugpatterns/util/SourceCodeTest.java | 47 ++++ to-11.sh | 16 ++ 7 files changed, 450 insertions(+), 52 deletions(-) create mode 100755 to-11.sh diff --git a/README.md b/README.md index 8ebf4d54cd..30d656e723 100644 --- a/README.md +++ b/README.md @@ -233,6 +233,12 @@ Other highly relevant commands: against _all_ code in the current working directory. For more information check the [PIT Maven plugin][pitest-maven]. +The `BugChecker` implementations provided by this project are tested using +Error Prone's `CompilationTestHelper` and `BugCheckerRefactoringTestHelper` +classes. These utilities accept text blocks containing inline Java source code. +To ease modification of this inline source code, consider using IntelliJ IDEA's +[language injection][idea-language-injection] feature. + ## 💡 How it works This project provides additional [`BugChecker`][error-prone-bugchecker] @@ -268,6 +274,7 @@ channel; please see our [security policy][security] for details. [github-actions-build-badge]: https://github.com/PicnicSupermarket/error-prone-support/actions/workflows/build.yml/badge.svg [github-actions-build-master]: https://github.com/PicnicSupermarket/error-prone-support/actions/workflows/build.yml?query=branch:master&event=push [google-java-format]: https://github.com/google/google-java-format +[idea-language-injection]: https://www.jetbrains.com/help/idea/using-language-injections.html [license-badge]: https://img.shields.io/github/license/PicnicSupermarket/error-prone-support [license]: https://github.com/PicnicSupermarket/error-prone-support/blob/master/LICENSE.md [maven-central-badge]: https://img.shields.io/maven-central/v/tech.picnic.error-prone-support/error-prone-support?color=blue diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormat.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormat.java index 99e1d92350..7654b71e6e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormat.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormat.java @@ -6,19 +6,24 @@ import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.sun.tools.javac.util.Position.NOPOS; import static java.util.stream.Collectors.joining; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.base.CharMatcher; import com.google.common.base.Splitter; import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.suppliers.Supplier; import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.SourceVersion; import com.google.googlejavaformat.java.Formatter; import com.google.googlejavaformat.java.FormatterException; import com.google.googlejavaformat.java.ImportOrderer; @@ -27,9 +32,12 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; -import com.sun.tools.javac.util.Position; +import com.sun.tools.javac.api.MultiTaskListener; +import com.sun.tools.javac.util.Context; import java.util.List; import java.util.Optional; +import javax.inject.Inject; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** * A {@link BugChecker} that flags improperly formatted Error Prone test code. @@ -45,20 +53,33 @@ * are not able to) remove imports that become obsolete as a result of applying their suggested * fix(es). */ -// XXX: Once we target JDK 17 (optionally?) suggest text block fixes. +// XXX: The check does not flag well-formatted text blocks with insufficient indentation. Cover this +// using an generic check or wait for Google Java Format support (see +// https://github.com/google/google-java-format/issues/883#issuecomment-1404336418). +// XXX: The check does not flag well-formatted text blocks with excess indentation. +// XXX: ^ Validate this claim. // XXX: GJF guesses the line separator to be used by inspecting the source. When using text blocks // this may cause the current unconditional use of `\n` not to be sufficient when building on // Windows; TBD. +// XXX: Forward compatibility: ignore "malformed" code in tests that, based on an +// `@DisabledForJreRange` or `@EnableForJreRange` annotation, target a Java runtime greater than the +// current runtime. @AutoService(BugChecker.class) @BugPattern( - summary = "Test code should follow the Google Java style", + summary = + "Test code should follow the Google Java style (and when targeting JDK 15+ be " + + "specified using a single text block)", link = BUG_PATTERNS_BASE_URL + "ErrorProneTestHelperSourceFormat", linkType = CUSTOM, severity = SUGGESTION, tags = STYLE) +// XXX: Drop suppression if/when the `avoidTextBlocks` field is dropped. +@SuppressWarnings("java:S2160" /* Super class equality definition suffices. */) public final class ErrorProneTestHelperSourceFormat extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; + private static final String FLAG_AVOID_TEXT_BLOCKS = + "ErrorProneTestHelperSourceFormat:AvoidTextBlocks"; private static final Formatter FORMATTER = new Formatter(); private static final Matcher INPUT_SOURCE_ACCEPTING_METHOD = anyOf( @@ -77,9 +98,31 @@ public final class ErrorProneTestHelperSourceFormat extends BugChecker instanceMethod() .onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput") .named("addOutputLines"); + private static final Supplier IS_JABEL_ENABLED = + VisitorState.memoize(ErrorProneTestHelperSourceFormat::isJabelEnabled); + // XXX: Proper name for this? + // XXX: Something about tabs. + private static final String TEXT_BLOCK_MARKER = "\"\"\""; + private static final String TEXT_BLOCK_LINE_SEPARATOR = "\n"; + private static final String DEFAULT_TEXT_BLOCK_INDENTATION = " ".repeat(12); + private static final String METHOD_SELECT_ARGUMENT_RELATIVE_INDENTATION = " ".repeat(8); - /** Instantiates a new {@link ErrorProneTestHelperSourceFormat} instance. */ - public ErrorProneTestHelperSourceFormat() {} + private final boolean avoidTextBlocks; + + /** Instantiates a default {@link ErrorProneTestHelperSourceFormat} instance. */ + public ErrorProneTestHelperSourceFormat() { + this(ErrorProneFlags.empty()); + } + + /** + * Instantiates a customized {@link ErrorProneTestHelperSourceFormat}. + * + * @param flags Any provided command line flags. + */ + @Inject + ErrorProneTestHelperSourceFormat(ErrorProneFlags flags) { + avoidTextBlocks = flags.getBoolean(FLAG_AVOID_TEXT_BLOCKS).orElse(Boolean.FALSE); + } @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { @@ -95,22 +138,23 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return buildDescription(tree).setMessage("No source code provided").build(); } - int startPos = ASTHelpers.getStartPosition(sourceLines.get(0)); - int endPos = state.getEndPosition(sourceLines.get(sourceLines.size() - 1)); - /* Attempt to format the source code only if it fully consists of constant expressions. */ return getConstantSourceCode(sourceLines) - .map(source -> flagFormattingIssues(startPos, endPos, source, isOutputSource, state)) + .map(source -> flagFormattingIssues(sourceLines, source, isOutputSource, state)) .orElse(Description.NO_MATCH); } private Description flagFormattingIssues( - int startPos, int endPos, String source, boolean retainUnusedImports, VisitorState state) { - Tree methodInvocation = state.getPath().getLeaf(); + List sourceLines, + String source, + boolean retainUnusedImports, + VisitorState state) { + MethodInvocationTree methodInvocation = (MethodInvocationTree) state.getPath().getLeaf(); String formatted; try { - formatted = formatSourceCode(source, retainUnusedImports).trim(); + String gjfResult = formatSourceCode(source, retainUnusedImports); + formatted = canUseTextBlocks(sourceLines, state) ? gjfResult : gjfResult.stripTrailing(); } catch ( @SuppressWarnings("java:S1166" /* Stack trace not relevant. */) FormatterException e) { @@ -119,31 +163,113 @@ private Description flagFormattingIssues( .build(); } - if (source.trim().equals(formatted)) { + boolean isFormatted = source.equals(formatted); + boolean hasStringLiteralMismatch = shouldUpdateStringLiteralFormat(sourceLines, state); + + if (isFormatted && !hasStringLiteralMismatch) { return Description.NO_MATCH; } - if (startPos == Position.NOPOS || endPos == Position.NOPOS) { - /* - * We have insufficient source information to emit a fix, so we only flag the fact that the - * code isn't properly formatted. - */ - return describeMatch(methodInvocation); - } + int startPos = ASTHelpers.getStartPosition(sourceLines.get(0)); + int endPos = state.getEndPosition(sourceLines.get(sourceLines.size() - 1)); + boolean hasNewlineMismatch = + !isFormatted && source.stripTrailing().equals(formatted.stripTrailing()); /* - * The code isn't properly formatted; replace all lines with the properly formatted - * alternatives. + * The source code is not properly formatted and/or not specified using a single text block. + * Report the more salient of the violations, and suggest a fix if sufficient source information + * is available. */ - return describeMatch( - methodInvocation, - SuggestedFix.replace( - startPos, - endPos, - Splitter.on(System.lineSeparator()) - .splitToStream(formatted) - .map(state::getConstantExpression) - .collect(joining(", ")))); + boolean isTextBlockUsageIssue = isFormatted || (hasNewlineMismatch && hasStringLiteralMismatch); + boolean canSuggestFix = startPos != NOPOS && endPos != NOPOS; + return buildDescription(methodInvocation) + .setMessage( + isTextBlockUsageIssue + ? String.format( + "Test code should %sbe specified using a single text block", + avoidTextBlocks ? "not " : "") + : String.format( + "Test code should follow the Google Java style%s", + hasNewlineMismatch ? " (pay attention to trailing newlines)" : "")) + .addFix( + canSuggestFix + ? SuggestedFix.replace( + startPos, + endPos, + canUseTextBlocks(sourceLines, state) + ? toTextBlockExpression(methodInvocation, formatted, state) + : toLineEnumeration(formatted, state)) + : SuggestedFix.emptyFix()) + .build(); + } + + private boolean shouldUpdateStringLiteralFormat( + List sourceLines, VisitorState state) { + return canUseTextBlocks(sourceLines, state) + ? (sourceLines.size() > 1 || !SourceCode.isTextBlock(sourceLines.get(0), state)) + : sourceLines.stream().anyMatch(tree -> SourceCode.isTextBlock(tree, state)); + } + + private boolean canUseTextBlocks(List sourceLines, VisitorState state) { + return !avoidTextBlocks + && (SourceVersion.supportsTextBlocks(state.context) + || IS_JABEL_ENABLED.get(state) + || sourceLines.stream().anyMatch(line -> SourceCode.isTextBlock(line, state))); + } + + private static String toTextBlockExpression( + MethodInvocationTree tree, String source, VisitorState state) { + String indentation = suggestTextBlockIndentation(tree, state); + + // XXX: Verify trailing """ on new line. + return TEXT_BLOCK_MARKER + + System.lineSeparator() + + indentation + + source + .replace(TEXT_BLOCK_LINE_SEPARATOR, System.lineSeparator() + indentation) + .replace("\\", "\\\\") + .replace(TEXT_BLOCK_MARKER, "\"\"\\\"") + + TEXT_BLOCK_MARKER; + } + + private static String toLineEnumeration(String source, VisitorState state) { + return Splitter.on(TEXT_BLOCK_LINE_SEPARATOR) + .splitToStream(source) + .map(state::getConstantExpression) + .collect(joining(", ")); + } + + // XXX: This makes certain assumptions; document these. + private static String suggestTextBlockIndentation( + MethodInvocationTree target, VisitorState state) { + CharSequence sourceCode = state.getSourceCode(); + if (sourceCode == null) { + return DEFAULT_TEXT_BLOCK_INDENTATION; + } + + String source = sourceCode.toString(); + return getIndentation(target.getArguments().get(1), source) + .or(() -> getIndentation(target.getArguments().get(0), source)) + .or( + () -> + getIndentation(target.getMethodSelect(), source) + .map(METHOD_SELECT_ARGUMENT_RELATIVE_INDENTATION::concat)) + .orElse(DEFAULT_TEXT_BLOCK_INDENTATION); + } + + private static Optional getIndentation(Tree tree, String source) { + int startPos = ASTHelpers.getStartPosition(tree); + if (startPos == NOPOS) { + return Optional.empty(); + } + + int finalNewLine = source.lastIndexOf(System.lineSeparator(), startPos); + if (finalNewLine < 0) { + return Optional.empty(); + } + + return Optional.of(source.substring(finalNewLine + 1, startPos)) + .filter(CharMatcher.whitespace()::matchesAllOf); } private static String formatSourceCode(String source, boolean retainUnusedImports) @@ -162,14 +288,29 @@ private static Optional getConstantSourceCode( StringBuilder source = new StringBuilder(); for (ExpressionTree sourceLine : sourceLines) { + if (source.length() > 0) { + source.append(TEXT_BLOCK_LINE_SEPARATOR); + } + String value = ASTHelpers.constValue(sourceLine, String.class); if (value == null) { return Optional.empty(); } - source.append(value).append(System.lineSeparator()); + source.append(value); } return Optional.of(source.toString()); } + + /** + * Tells whether Jabel appears to be enabled, indicating that text blocks are supported, even if + * may appear that they {@link SourceVersion#supportsTextBlocks(Context) aren't}. + * + * @see Jabel + */ + private static boolean isJabelEnabled(VisitorState state) { + return MultiTaskListener.instance(state.context).getTaskListeners().stream() + .anyMatch(listener -> listener.toString().contains("com.github.bsideup.jabel")); + } } 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 1fd8a17f70..c1351bcc07 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,6 +12,7 @@ import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.util.ErrorProneToken; import com.google.errorprone.util.ErrorProneTokens; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.util.JCDiagnostic.DiagnosticPosition; @@ -26,8 +27,30 @@ public final class SourceCode { /** The complement of {@link CharMatcher#whitespace()}. */ private static final CharMatcher NON_WHITESPACE_MATCHER = CharMatcher.whitespace().negate(); + // XXX: Proper name for this? + private static final String TEXT_BLOCK_MARKER = "\"\"\""; + private SourceCode() {} + /** + * Tells whether the given expression is a text block. + * + * @param tree The AST node of interest. + * @param state A {@link VisitorState} describing the context in which the given {@link + * ExpressionTree} is found. + * @return {@code true} iff the given expression is a text block. + */ + // XXX: Add tests! + public static boolean isTextBlock(ExpressionTree tree, VisitorState state) { + if (tree.getKind() != Tree.Kind.STRING_LITERAL) { + return false; + } + + /* If the source code is unavailable then we assume that this literal is _not_ a text block. */ + String src = state.getSourceForNode(tree); + return src != null && src.startsWith(TEXT_BLOCK_MARKER); + } + /** * Returns a string representation of the given {@link Tree}, preferring the original source code * (if available) over its prettified representation. diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java index 51678cd335..778e999850 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibrary.java @@ -75,8 +75,12 @@ private static boolean canIntroduceUsage(String className, VisitorState state) { * *

The {@link VisitorState}'s symbol table is consulted first. If the type has not yet been * loaded, then an attempt is made to do so. + * + * @param className The type of interest. + * @param state The context under consideration. + * @return {@code true} iff the indicated type is on the classpath. */ - private static boolean isKnownClass(String className, VisitorState state) { + public static boolean isKnownClass(String className, VisitorState state) { return state.getTypeFromString(className) != null || canLoadClass(className, state); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatTest.java index 46be750780..e6655d2042 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatTest.java @@ -4,8 +4,15 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledForJreRange; +import org.junit.jupiter.api.condition.JRE; final class ErrorProneTestHelperSourceFormatTest { + // XXX: Add tests cases for `ErrorProneTestHelperSourceFormat:IgnoreMalformedCode`. + // XXX: Consider reducing the `@DisabledForJreRange(max = JRE.JAVA_14)` test scope by moving the + // text blocks to smaller test methods. + + @DisabledForJreRange(max = JRE.JAVA_14) @Test void identification() { CompilationTestHelper.newInstance(ErrorProneTestHelperSourceFormat.class, getClass()) @@ -17,13 +24,70 @@ void identification() { "import tech.picnic.errorprone.bugpatterns.EmptyMethod;", "", "class A {", - " private final CompilationTestHelper compilationTestHelper =", - " CompilationTestHelper.newInstance(EmptyMethod.class, getClass());", - " private final BugCheckerRefactoringTestHelper refactoringTestHelper =", - " BugCheckerRefactoringTestHelper.newInstance(EmptyMethod.class, getClass());", + " void m() {", + " CompilationTestHelper.newInstance(EmptyMethod.class, getClass())", + " // BUG: Diagnostic contains: No source code provided", + " .addSourceLines(\"A.java\")", + " // BUG: Diagnostic contains: Source code is malformed:", + " .addSourceLines(\"B.java\", \"class B {\")", + " // BUG: Diagnostic contains: Test code should be specified using a single text block", + " .addSourceLines(\"C.java\", \"class C {}\")", + " // Malformed code, but not compile-time constant, so not flagged.", + " .addSourceLines(\"D.java\", \"class D {\" + getClass())", + " // BUG: Diagnostic contains: Test code should follow the Google Java style", + " .addSourceLines(\"E.java\", \"class E { }\")", + " // Well-formed code, so not flagged.", + " .addSourceLines(\"F.java\", \"\"\"", + " class F {}", + " \"\"\")", + " // BUG: Diagnostic contains: Test code should follow the Google Java style (pay attention to", + " // trailing newlines)", + " .addSourceLines(\"G.java\", \"\"\"", + " class G {}\"\"\")", + " .doTest();", "", + " BugCheckerRefactoringTestHelper.newInstance(EmptyMethod.class, getClass())", + " // BUG: Diagnostic contains: Test code should follow the Google Java style", + " .addInputLines(\"in/A.java\", \"class A { }\")", + " // BUG: Diagnostic contains: Test code should follow the Google Java style", + " .addOutputLines(\"out/A.java\", \"class A { }\")", + " // BUG: Diagnostic contains: Test code should follow the Google Java style", + " .addInputLines(", + " \"in/B.java\",", + " \"\"\"", + " import java.util.Map;", + "", + " class B {}", + " \"\"\")", + " // Unused import, but in an output file, so not flagged.", + " .addOutputLines(", + " \"out/B.java\",", + " \"\"\"", + " import java.util.Map;", + "", + " class B {}", + " \"\"\")", + " .doTest(TestMode.TEXT_MATCH);", + " }", + "}") + .doTest(); + } + + @DisabledForJreRange(max = JRE.JAVA_14) + @Test + void identificationAvoidTextBlocks() { + CompilationTestHelper.newInstance(ErrorProneTestHelperSourceFormat.class, getClass()) + .setArgs("-XepOpt:ErrorProneTestHelperSourceFormat:AvoidTextBlocks=true") + .addSourceLines( + "A.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;", + "import com.google.errorprone.CompilationTestHelper;", + "import tech.picnic.errorprone.bugpatterns.EmptyMethod;", + "", + "class A {", " void m() {", - " compilationTestHelper", + " CompilationTestHelper.newInstance(EmptyMethod.class, getClass())", " // BUG: Diagnostic contains: No source code provided", " .addSourceLines(\"A.java\")", " // BUG: Diagnostic contains: Source code is malformed:", @@ -34,9 +98,16 @@ void identification() { " .addSourceLines(\"D.java\", \"class D {\" + getClass())", " // BUG: Diagnostic contains: Test code should follow the Google Java style", " .addSourceLines(\"E.java\", \"class E { }\")", + " // BUG: Diagnostic contains: Test code should not be specified using a single text block", + " .addSourceLines(\"F.java\", \"\"\"", + " class F {}", + " \"\"\")", + " // BUG: Diagnostic contains: Test code should follow the Google Java style (pay attention to", + " // trailing newlines)", + " .addSourceLines(\"G.java\", \"class G {}\", \"\")", " .doTest();", "", - " refactoringTestHelper", + " BugCheckerRefactoringTestHelper.newInstance(EmptyMethod.class, getClass())", " // BUG: Diagnostic contains: Test code should follow the Google Java style", " .addInputLines(\"A.java\", \"class A { }\")", " // BUG: Diagnostic contains: Test code should follow the Google Java style", @@ -51,11 +122,14 @@ void identification() { .doTest(); } + // XXX: Add `replacement` test. + @DisabledForJreRange(max = JRE.JAVA_14) @Test void replacement() { /* * Verifies that import sorting and code formatting is performed unconditionally, while unused * imports are removed unless part of a `BugCheckerRefactoringTestHelper` expected output file. + * Also verifies that text blocks are properly indented. */ BugCheckerRefactoringTestHelper.newInstance(ErrorProneTestHelperSourceFormat.class, getClass()) .addInputLines( @@ -66,13 +140,104 @@ void replacement() { "import tech.picnic.errorprone.bugpatterns.EmptyMethod;", "", "class A {", - " private final CompilationTestHelper compilationTestHelper =", - " CompilationTestHelper.newInstance(EmptyMethod.class, getClass());", - " private final BugCheckerRefactoringTestHelper refactoringTestHelper =", - " BugCheckerRefactoringTestHelper.newInstance(EmptyMethod.class, getClass());", + " void m() {", + " CompilationTestHelper.newInstance(EmptyMethod.class, getClass())", + " .addSourceLines(", + " \"A.java\",", + " \"\"\"", + " import java.util.Map;", + " import java.util.Collection;", + " import java.util.List;", + "", + " interface A extends List, Map { }\"\"\")", + " .addSourceLines(\"B.java\", \"class B {}\")", + " .doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance(EmptyMethod.class, getClass())", + " .addInputLines(", + " \"in/A.java\",", + " \"\"\"", + " import java.util.Map;", + " import java.util.Collection;", + " import java.util.List;", + "", + " interface A extends List, Map { }\"\"\")", + " .addOutputLines(", + " \"out/A.java\",", + " \"\"\"", + " import java.util.Map;", + " import java.util.Collection;", + " import java.util.List;", + "", + " interface A extends List, Map { }\"\"\")", + " .doTest(TestMode.TEXT_MATCH);", + " }", + "}") + .addOutputLines( + "out/A.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;", + "import com.google.errorprone.CompilationTestHelper;", + "import tech.picnic.errorprone.bugpatterns.EmptyMethod;", + "", + "class A {", + " void m() {", + " CompilationTestHelper.newInstance(EmptyMethod.class, getClass())", + " .addSourceLines(", + " \"A.java\",", + " \"\"\"", + " import java.util.List;", + " import java.util.Map;", "", + " interface A extends List, Map {}", + " \"\"\")", + " .addSourceLines(\"B.java\", \"\"\"", + " class B {}", + " \"\"\")", + " .doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance(EmptyMethod.class, getClass())", + " .addInputLines(", + " \"in/A.java\",", + " \"\"\"", + " import java.util.List;", + " import java.util.Map;", + "", + " interface A extends List, Map {}", + " \"\"\")", + " .addOutputLines(", + " \"out/A.java\",", + " \"\"\"", + " import java.util.Collection;", + " import java.util.List;", + " import java.util.Map;", + "", + " interface A extends List, Map {}", + " \"\"\")", + " .doTest(TestMode.TEXT_MATCH);", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void replacementAvoidTextBlocks() { + /* + * Verifies that import sorting and code formatting is performed unconditionally, while unused + * imports are removed unless part of a `BugCheckerRefactoringTestHelper` expected output file. + */ + BugCheckerRefactoringTestHelper.newInstance(ErrorProneTestHelperSourceFormat.class, getClass()) + .setArgs("-XepOpt:ErrorProneTestHelperSourceFormat:AvoidTextBlocks=true") + .addInputLines( + "in/A.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;", + "import com.google.errorprone.CompilationTestHelper;", + "import tech.picnic.errorprone.bugpatterns.EmptyMethod;", + "", + "class A {", " void m() {", - " compilationTestHelper", + " CompilationTestHelper.newInstance(EmptyMethod.class, getClass())", " .addSourceLines(", " \"A.java\",", " \"import java.util.Map;\",", @@ -82,7 +247,7 @@ void replacement() { " \"interface A extends List, Map { }\")", " .doTest();", "", - " refactoringTestHelper", + " BugCheckerRefactoringTestHelper.newInstance(EmptyMethod.class, getClass())", " .addInputLines(", " \"A.java\",", " \"import java.util.Map;\",", @@ -108,13 +273,8 @@ void replacement() { "import tech.picnic.errorprone.bugpatterns.EmptyMethod;", "", "class A {", - " private final CompilationTestHelper compilationTestHelper =", - " CompilationTestHelper.newInstance(EmptyMethod.class, getClass());", - " private final BugCheckerRefactoringTestHelper refactoringTestHelper =", - " BugCheckerRefactoringTestHelper.newInstance(EmptyMethod.class, getClass());", - "", " void m() {", - " compilationTestHelper", + " CompilationTestHelper.newInstance(EmptyMethod.class, getClass())", " .addSourceLines(", " \"A.java\",", " \"import java.util.List;\",", @@ -123,7 +283,7 @@ void replacement() { " \"interface A extends List, Map {}\")", " .doTest();", "", - " refactoringTestHelper", + " BugCheckerRefactoringTestHelper.newInstance(EmptyMethod.class, getClass())", " .addInputLines(", " \"A.java\",", " \"import java.util.List;\",", 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 131da0ba38..4c21be431e 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 @@ -5,21 +5,52 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.BugPattern; +import com.google.errorprone.CompilationTestHelper; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; +import com.sun.source.tree.ReturnTree; import com.sun.source.tree.Tree; import javax.lang.model.element.Name; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledForJreRange; +import org.junit.jupiter.api.condition.JRE; final class SourceCodeTest { + @DisabledForJreRange(max = JRE.JAVA_14) + @Test + void isTextBlock() { + CompilationTestHelper.newInstance(TextBlockFlagger.class, getClass()) + .addSourceLines( + "A.java", + "class A {", + " String negative1() {", + " return toString();", + " }", + "", + " String negative2() {", + " return \"foo\";", + " }", + "", + " String positive1() {", + " // BUG: Diagnostic contains:", + " return \"\"\"", + " foo", + " \"\"\";", + " }", + "}") + .doTest(); + } + @Test void deleteWithTrailingWhitespaceAnnotations() { BugCheckerRefactoringTestHelper.newInstance( @@ -228,6 +259,22 @@ UnwrapMethodInvocationDroppingWhitespaceAndCommentsTestChecker.class, getClass() .doTest(TestMode.TEXT_MATCH); } + /** + * A {@link BugChecker} that delegates to {@link SourceCode#isTextBlock(ExpressionTree, + * VisitorState)}. + */ + @BugPattern(summary = "Interacts with `SourceCode` for testing purposes", severity = ERROR) + public static final class TextBlockFlagger extends BugChecker implements ReturnTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchReturn(ReturnTree tree, VisitorState state) { + return SourceCode.isTextBlock(tree.getExpression(), state) + ? describeMatch(tree) + : Description.NO_MATCH; + } + } + /** * A {@link BugChecker} that uses {@link SourceCode#deleteWithTrailingWhitespace(Tree, * VisitorState)} to suggest the deletion of annotations and methods with a name containing diff --git a/to-11.sh b/to-11.sh new file mode 100755 index 0000000000..e51d9c4116 --- /dev/null +++ b/to-11.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash + +set -e -u -o pipefail + +# XXX: Clean this up. +# XXX: Drop this class. +mvn clean test-compile fmt:format \ + -s "$(dirname "${0}")/settings.xml" \ + -T 1.0C \ + -Perror-prone \ + -Perror-prone-fork \ + -Ppatch \ + -Pself-check \ + -Derror-prone.patch-checks=ErrorProneTestHelperSourceFormat \ + -Derror-prone.self-check-args='-XepOpt:ErrorProneTestHelperSourceFormat:AvoidTextBlocks=true -XepDisableAllChecks' \ + -Dverification.skip