Skip to content

Commit

Permalink
Update all tests to use text blocks
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Stephan202 committed Dec 26, 2023
1 parent 2569067 commit 160580d
Show file tree
Hide file tree
Showing 7 changed files with 450 additions and 52 deletions.
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand All @@ -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<ExpressionTree> INPUT_SOURCE_ACCEPTING_METHOD =
anyOf(
Expand All @@ -77,9 +98,31 @@ public final class ErrorProneTestHelperSourceFormat extends BugChecker
instanceMethod()
.onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput")
.named("addOutputLines");
private static final Supplier<Boolean> 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) {
Expand All @@ -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<? extends ExpressionTree> 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) {
Expand All @@ -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<? extends ExpressionTree> 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<? extends ExpressionTree> 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<String> 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)
Expand All @@ -162,14 +288,29 @@ private static Optional<String> 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 <em>appear</em> that they {@link SourceVersion#supportsTextBlocks(Context) aren't}.
*
* @see <a href="https://github.com/bsideup/jabel">Jabel</a>
*/
private static boolean isJabelEnabled(VisitorState state) {
return MultiTaskListener.instance(state.context).getTaskListeners().stream()
.anyMatch(listener -> listener.toString().contains("com.github.bsideup.jabel"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,12 @@ public static boolean canIntroduceUsage(String className, VisitorState state) {
*
* <p>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);
}

Expand Down
Loading

0 comments on commit 160580d

Please sign in to comment.