Skip to content

Commit

Permalink
WIP
Browse files Browse the repository at this point in the history
  • Loading branch information
Stephan202 committed Mar 23, 2024
1 parent ff32f63 commit 61120c7
Show file tree
Hide file tree
Showing 7 changed files with 83 additions and 69 deletions.
9 changes: 9 additions & 0 deletions documentation-support/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
<artifactId>error_prone_test_helpers</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>error-prone-utils</artifactId>
</dependency>
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-annotations</artifactId>
Expand Down Expand Up @@ -72,6 +76,11 @@
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jetbrains</groupId>
<artifactId>annotations</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jspecify</groupId>
<artifactId>jspecify</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.Optional;
import org.jspecify.annotations.Nullable;
import tech.picnic.errorprone.documentation.BugPatternTestExtractor.TestCases;
import tech.picnic.errorprone.utils.SourceCode;

/**
* An {@link Extractor} that describes how to extract data from classes that test a {@code
Expand Down Expand Up @@ -189,21 +190,10 @@ private static void extractReplacementTestCases(
}
}

// XXX: This logic is duplicated in `ErrorProneTestSourceFormat`. Can we do better?
private static Optional<String> getSourceCode(MethodInvocationTree tree) {
List<? extends ExpressionTree> sourceLines =
tree.getArguments().subList(1, tree.getArguments().size());
StringBuilder source = new StringBuilder();

for (ExpressionTree sourceLine : sourceLines) {
String value = ASTHelpers.constValue(sourceLine, String.class);
if (value == null) {
return Optional.empty();
}
source.append(value).append('\n');
}

return Optional.of(source.toString());
return SourceCode.joinConstantSourceCodeLines(
tree.getArguments().subList(1, tree.getArguments().size()))
.map(s -> s + '\n');
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import javax.tools.Diagnostic;
import javax.tools.JavaCompiler;
import javax.tools.JavaFileObject;
import org.intellij.lang.annotations.Language;

// XXX: Generalize and move this class so that it can also be used by `refaster-compiler`.
// XXX: This class is supported by the `ErrorProneTestHelperSourceFormat` check, but until that
Expand All @@ -23,12 +24,12 @@ public final class Compilation {
private Compilation() {}

public static void compileWithDocumentationGenerator(
Path outputDirectory, String path, String... lines) {
compileWithDocumentationGenerator(outputDirectory.toAbsolutePath().toString(), path, lines);
Path outputDirectory, String path, @Language("JAVA") String source) {
compileWithDocumentationGenerator(outputDirectory.toAbsolutePath().toString(), path, source);
}

public static void compileWithDocumentationGenerator(
String outputDirectory, String path, String... lines) {
String outputDirectory, String path, @Language("JAVA") String source) {
/*
* The compiler options specified here largely match those used by Error Prone's
* `CompilationTestHelper`. A key difference is the stricter linting configuration. When
Expand All @@ -49,7 +50,7 @@ public static void compileWithDocumentationGenerator(
"-Xplugin:DocumentationGenerator -XoutputDirectory=" + outputDirectory,
"-XDdev",
"-XDcompilePolicy=simple"),
FileObjects.forSourceLines(path, lines));
FileObjects.forSourceLines(path, source));
}

private static void compile(ImmutableList<String> options, JavaFileObject javaFileObject) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,27 +53,20 @@
* are not able to) remove imports that become obsolete as a result of applying their suggested
* fix(es).
*/
// 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
// XXX: The check does not flag well-formatted text blocks with insufficient or excess 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 (and when targeting JDK 15+ be "
+ "specified using a single text block)",
"""
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 + "TestHelperSourceFormat",
linkType = CUSTOM,
severity = SUGGESTION,
tags = STYLE)
// XXX: Drop suppression if/when the `avoidTextBlocks` field is dropped.
// XXX: Drop this suppression if/when the `avoidTextBlocks` field is dropped.
@SuppressWarnings("java:S2160" /* Super class equality definition suffices. */)
public final class TestHelperSourceFormat extends BugChecker
implements MethodInvocationTreeMatcher {
Expand All @@ -91,6 +84,8 @@ public final class TestHelperSourceFormat extends BugChecker
.named("addInputLines"),
// XXX: Add tests for `Compilation.compileWithDocumentationGenerator`. Until done, make
// sure to update this matcher if that method's class or name is changed/moved.
// XXX: Alternatively, match any invocation of a method whose last argument is annotated
// `@Language("JAVA")`.
staticMethod()
.onClass("tech.picnic.errorprone.documentation.Compilation")
.named("compileWithDocumentationGenerator"));
Expand All @@ -100,10 +95,6 @@ public final class TestHelperSourceFormat extends BugChecker
.named("addOutputLines");
private static final Supplier<Boolean> IS_JABEL_ENABLED =
VisitorState.memoize(TestHelperSourceFormat::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);

Expand Down Expand Up @@ -139,7 +130,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
}

/* Attempt to format the source code only if it fully consists of constant expressions. */
return getConstantSourceCode(sourceLines)
return SourceCode.joinConstantSourceCodeLines(sourceLines)
.map(source -> flagFormattingIssues(sourceLines, source, isOutputSource, state))
.orElse(Description.NO_MATCH);
}
Expand Down Expand Up @@ -222,18 +213,18 @@ private static String toTextBlockExpression(
String indentation = suggestTextBlockIndentation(tree, state);

// XXX: Verify trailing """ on new line.
return TEXT_BLOCK_MARKER
return SourceCode.TEXT_BLOCK_DELIMITER
+ System.lineSeparator()
+ indentation
+ source
.replace(TEXT_BLOCK_LINE_SEPARATOR, System.lineSeparator() + indentation)
.replace(SourceCode.TEXT_BLOCK_LINE_SEPARATOR, System.lineSeparator() + indentation)
.replace("\\", "\\\\")

Check warning on line 221 in error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormat.java

View workflow job for this annotation

GitHub Actions / pitest

2 different changes can be made to line 221 without causing a test to fail

removed call to replace (covered by 2 tests RemoveChainedCallsMutator) swapped parameters 1 and 2 in call to replace (covered by 2 tests ParamSwapMutator)
.replace(TEXT_BLOCK_MARKER, "\"\"\\\"")
+ TEXT_BLOCK_MARKER;
.replace(SourceCode.TEXT_BLOCK_DELIMITER, "\"\"\\\"")

Check warning on line 222 in error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormat.java

View workflow job for this annotation

GitHub Actions / pitest

2 different changes can be made to line 222 without causing a test to fail

removed call to replace (covered by 2 tests RemoveChainedCallsMutator) swapped parameters 1 and 2 in call to replace (covered by 2 tests ParamSwapMutator)
+ SourceCode.TEXT_BLOCK_DELIMITER;
}

private static String toLineEnumeration(String source, VisitorState state) {
return Splitter.on(TEXT_BLOCK_LINE_SEPARATOR)
return Splitter.on(SourceCode.TEXT_BLOCK_LINE_SEPARATOR)
.splitToStream(source)
.map(state::getConstantExpression)
.collect(joining(", "));
Expand Down Expand Up @@ -268,7 +259,7 @@ private static Optional<String> getIndentation(Tree tree, String source) {
return Optional.empty();
}

return Optional.of(source.substring(finalNewLine + 1, startPos))
return Optional.of(source.substring(finalNewLine + System.lineSeparator().length(), startPos))

Check warning on line 262 in error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormat.java

View workflow job for this annotation

GitHub Actions / pitest

2 different changes can be made to line 262 without causing a test to fail

removed call to substring (covered by 2 tests RemoveChainedCallsMutator) replaced return value with Optional.empty for getIndentation (covered by 2 tests EmptyObjectReturnValsMutator)
.filter(CharMatcher.whitespace()::matchesAllOf);
}

Expand All @@ -282,27 +273,6 @@ private static String formatSourceCode(String source, boolean retainUnusedImport
return FORMATTER.formatSource(withOptionallyRemovedImports);
}

// XXX: This logic is duplicated in `BugPatternTestExtractor`. Can we do better?
private static Optional<String> getConstantSourceCode(
List<? extends ExpressionTree> sourceLines) {
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);
}

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}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,14 @@ void m() {
.addSourceLines(
"F.java",
""\"
class F {}
""\")
class F {}
""\")
// BUG: Diagnostic contains: Test code should follow the Google Java style (pay attention to
// trailing newlines)
.addSourceLines(
"G.java",
""\"
class G {}""\")
class G {}""\")
.doTest();
BugCheckerRefactoringTestHelper.newInstance(RefasterAnyOfUsage.class, getClass())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@
import com.google.common.collect.Streams;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.util.ASTHelpers;
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;
import com.sun.tools.javac.util.Position;
import java.util.List;
import java.util.Optional;

/**
Expand All @@ -27,8 +29,14 @@ 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 = "\"\"\"";
/** The Java syntax that indicates the start and end of a text block. */
public static final String TEXT_BLOCK_DELIMITER = "\"\"\"";

/**
* The string separating lines in a Java text block, independently of the platform on which the
* code is compiled.
*/
public static final String TEXT_BLOCK_LINE_SEPARATOR = "\n";

private SourceCode() {}

Expand All @@ -48,7 +56,7 @@ public static boolean isTextBlock(ExpressionTree tree, VisitorState state) {

/* 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);
return src != null && src.startsWith(TEXT_BLOCK_DELIMITER);

Check warning on line 59 in error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 59 without causing a test to fail

removed conditional - replaced equality check with true (covered by 1 tests RemoveConditionalMutator_EQUAL_IF)
}

/**
Expand All @@ -65,6 +73,37 @@ public static String treeToString(Tree tree, VisitorState state) {
return src != null ? src : tree.toString();
}

/**
* Constructs a multi-line string by joining the given source code lines, if they all represent
* compile-time constants.
*
* @param sourceLines The source code lines of interest.
* @return A non-empty optional iff all source code lines represent compile-time constants.
*/
// XXX: Test!
// XXX: This method doesn't add a trailing newline for the benefit of one caller, but that's
// perhaps a bit awkward.
public static Optional<String> joinConstantSourceCodeLines(
List<? extends ExpressionTree> sourceLines) {
StringBuilder source = new StringBuilder();

for (ExpressionTree sourceLine : sourceLines) {
if (!source.isEmpty()) {

Check warning on line 91 in error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java

View workflow job for this annotation

GitHub Actions / pitest

2 different changes can be made to line 91 without causing a test to fail

removed conditional - replaced equality check with false (no tests cover this line RemoveConditionalMutator_EQUAL_ELSE) removed conditional - replaced equality check with true (no tests cover this line RemoveConditionalMutator_EQUAL_IF)
// XXX: Review whether we can/should use `System.lineSeparator()` here.
source.append('\n');
}

String value = ASTHelpers.constValue(sourceLine, String.class);
if (value == null) {

Check warning on line 97 in error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java

View workflow job for this annotation

GitHub Actions / pitest

2 different changes can be made to line 97 without causing a test to fail

removed conditional - replaced equality check with false (no tests cover this line RemoveConditionalMutator_EQUAL_ELSE) removed conditional - replaced equality check with true (no tests cover this line RemoveConditionalMutator_EQUAL_IF)
return Optional.empty();
}

source.append(value);
}

return Optional.of(source.toString());

Check warning on line 104 in error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 104 without causing a test to fail

replaced return value with Optional.empty for joinConstantSourceCodeLines (no tests cover this line EmptyObjectReturnValsMutator)
}

/**
* Creates a {@link SuggestedFix} for the deletion of the given {@link Tree}, including any
* whitespace that follows it.
Expand Down
5 changes: 5 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,11 @@
<artifactId>value-annotations</artifactId>
<version>2.10.1</version>
</dependency>
<dependency>
<groupId>org.jetbrains</groupId>
<artifactId>annotations</artifactId>
<version>24.1.0</version>
</dependency>
<dependency>
<groupId>org.jspecify</groupId>
<artifactId>jspecify</artifactId>
Expand Down

0 comments on commit 61120c7

Please sign in to comment.