Skip to content

Commit

Permalink
Build test code using JDK 17
Browse files Browse the repository at this point in the history
This enables usage of text blocks, which in combination with IntelliJ
IDEA's _language injection_ functionality greatly simplify writing
`BugChecker` tests.
  • Loading branch information
Stephan202 committed Nov 13, 2022
1 parent 79ca98d commit 7ab17e5
Show file tree
Hide file tree
Showing 9 changed files with 424 additions and 48 deletions.
1 change: 1 addition & 0 deletions .mvn/jvm.config
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED
--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
Expand Down
18 changes: 6 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -176,17 +176,11 @@ Some other commands one may find relevant:
the current working directory does not contain unstaged or uncommited
changes.

When running the project's tests in IntelliJ IDEA, you might see the following
error:

```
java: exporting a package from system module jdk.compiler is not allowed with --release
```

If this happens, go to _Settings -> Build, Execution, Deployment -> Compiler ->
Java Compiler_ and deselect the option _Use '--release' option for
cross-compilation (Java 9 and later)_. See [IDEA-288052][idea-288052] for
details.
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

Expand All @@ -213,7 +207,7 @@ guidelines][contributing].
[github-actions-build-badge]: https://github.com/PicnicSupermarket/error-prone-support/actions/workflows/build.yaml/badge.svg
[github-actions-build-master]: https://github.com/PicnicSupermarket/error-prone-support/actions/workflows/build.yaml?query=branch%3Amaster
[google-java-format]: https://github.com/google/google-java-format
[idea-288052]: https://youtrack.jetbrains.com/issue/IDEA-288052
[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 @@ -5,12 +5,15 @@
import static com.google.errorprone.BugPattern.StandardTags.STYLE;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.instanceMethod;
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;
Expand All @@ -26,9 +29,9 @@
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 java.util.List;
import java.util.Optional;
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/**
* A {@link BugChecker} that flags improperly formatted Error Prone test code.
Expand All @@ -44,20 +47,27 @@
* 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.
// 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.
@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)
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 String FLAG_IGNORE_MALFORMED_CODE =
"ErrorProneTestHelperSourceFormat:IgnoreMalformedCode";
private static final Formatter FORMATTER = new Formatter();
private static final Matcher<ExpressionTree> INPUT_SOURCE_ACCEPTING_METHOD =
anyOf(
Expand All @@ -71,9 +81,27 @@ public final class ErrorProneTestHelperSourceFormat extends BugChecker
instanceMethod()
.onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput")
.named("addOutputLines");
// XXX: Proper name for this?
private static final String TEXT_BLOCK_MARKER = "\"\"\"";
private static final String DEFAULT_TEXT_BLOCK_INDENTATION = " ".repeat(12);

/** Instantiates a new {@link ErrorProneTestHelperSourceFormat} instance. */
public ErrorProneTestHelperSourceFormat() {}
private final boolean avoidTextBlocks;
private final boolean ignoreMalformedCode;

/** Instantiates a default {@link ErrorProneTestHelperSourceFormat} instance. */
public ErrorProneTestHelperSourceFormat() {
this(ErrorProneFlags.empty());
}

/**
* Instantiates a customized {@link ErrorProneTestHelperSourceFormat}.
*
* @param flags Any provided command line flags.
*/
public ErrorProneTestHelperSourceFormat(ErrorProneFlags flags) {
avoidTextBlocks = flags.getBoolean(FLAG_AVOID_TEXT_BLOCKS).orElse(Boolean.FALSE);
ignoreMalformedCode = flags.getBoolean(FLAG_IGNORE_MALFORMED_CODE).orElse(Boolean.FALSE);
}

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
Expand All @@ -88,53 +116,130 @@ 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(state) ? gjfResult : gjfResult.stripTrailing();
} catch (FormatterException e) {
return buildDescription(methodInvocation)
.setMessage(String.format("Source code is malformed: %s", e.getMessage()))
.build();
return ignoreMalformedCode
? Description.NO_MATCH
: buildDescription(methodInvocation)
.setMessage(String.format("Source code is malformed: %s", e.getMessage()))
.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('\n')
.splitToStream(formatted)
.map(state::getConstantExpression)
.collect(joining(", "))));
return buildDescription(methodInvocation)
.setMessage(
isFormatted || (hasNewlineMismatch && hasStringLiteralMismatch)
? 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(
(startPos == NOPOS || endPos == NOPOS)
? SuggestedFix.emptyFix()
: SuggestedFix.replace(
startPos,
endPos,
canUseTextBlocks(state)
? toTextBlockExpression(methodInvocation, formatted, state)
: toLineEnumeration(formatted, state)))
.build();
}

private boolean shouldUpdateStringLiteralFormat(
List<? extends ExpressionTree> sourceLines, VisitorState state) {
return canUseTextBlocks(state)
? sourceLines.size() > 1 || !SourceCode.isTextBlock(sourceLines.get(0), state)
: sourceLines.stream().anyMatch(tree -> SourceCode.isTextBlock(tree, state));
}

private boolean canUseTextBlocks(VisitorState state) {
return !avoidTextBlocks && SourceCode.isTextBlockSupported(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
+ '\n'
+ indentation
+ source
.replace("\n", '\n' + indentation)
.replace("\\", "\\\\")
.replace(TEXT_BLOCK_MARKER, "\"\"\\\"")
+ TEXT_BLOCK_MARKER;
}

private static String toLineEnumeration(String source, VisitorState state) {
return Splitter.on('\n')
.splitToStream(source)
.map(state::getConstantExpression)
.collect(joining(", "));
}

// XXX: This doesn't seem fully correct; see `EmptyMethodTest#replacement`.
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))
.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('\n', 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 @@ -152,12 +257,16 @@ private static Optional<String> getConstantSourceCode(
StringBuilder source = new StringBuilder();

for (ExpressionTree sourceLine : sourceLines) {
if (source.length() > 0) {
source.append('\n');
}

Object value = ASTHelpers.constValue(sourceLine);
if (value == null) {
return Optional.empty();
}

source.append(value).append('\n');
source.append(value);
}

return Optional.of(source.toString());
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,53 @@
package tech.picnic.errorprone.bugpatterns.util;

import com.google.errorprone.VisitorState;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.jvm.Target;

/**
* 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 {
// XXX: Proper name for this?
private static final String TEXT_BLOCK_MARKER = "\"\"\"";

private SourceCode() {}

/**
* Tells whether the targeted Java language level supports text blocks.
*
* @param state A {@link VisitorState} from which the targeted Java language level can be derived.
* @return {@code true} iff text block expressions are supported.
*/
// XXX: Add tests!
public static boolean isTextBlockSupported(VisitorState state) {
// XXX: String comparison is for JDK 11 compatibility. Is there a better way?
return Target.instance(state.context).toString().compareTo("JDK1_15") >= 0;
// return Target.instance(state.context).compareTo(Target.JDK1_15) >= 0;
}

/**
* 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 @@ -69,6 +69,9 @@ void identification() {
.doTest();
}

// XXX: Drop this suppression once
// https://github.com/PicnicSupermarket/error-prone-support/pull/347 is merged.
@SuppressWarnings("RegexpMultiline" /* Check may introduce empty line at start of code block. */)
@Test
void replacement() {
refactoringTestHelper
Expand Down
Loading

0 comments on commit 7ab17e5

Please sign in to comment.