From 61120c7d7135784605ee0a30a4e55e8739bc09f0 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sat, 23 Mar 2024 19:34:30 +0100 Subject: [PATCH] WIP --- documentation-support/pom.xml | 9 +++ .../BugPatternTestExtractor.java | 18 ++---- .../errorprone/documentation/Compilation.java | 9 +-- .../bugpatterns/TestHelperSourceFormat.java | 60 +++++-------------- .../TestHelperSourceFormatTest.java | 6 +- .../picnic/errorprone/utils/SourceCode.java | 45 +++++++++++++- pom.xml | 5 ++ 7 files changed, 83 insertions(+), 69 deletions(-) diff --git a/documentation-support/pom.xml b/documentation-support/pom.xml index 373b2ce900..05d1746adc 100644 --- a/documentation-support/pom.xml +++ b/documentation-support/pom.xml @@ -33,6 +33,10 @@ error_prone_test_helpers test + + ${project.groupId} + error-prone-utils + com.fasterxml.jackson.core jackson-annotations @@ -72,6 +76,11 @@ assertj-core test + + org.jetbrains + annotations + test + org.jspecify jspecify diff --git a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java index 7ca51f683a..0810f624a5 100644 --- a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java +++ b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java @@ -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 @@ -189,21 +190,10 @@ private static void extractReplacementTestCases( } } - // XXX: This logic is duplicated in `ErrorProneTestSourceFormat`. Can we do better? private static Optional getSourceCode(MethodInvocationTree tree) { - List 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'); } } diff --git a/documentation-support/src/test/java/tech/picnic/errorprone/documentation/Compilation.java b/documentation-support/src/test/java/tech/picnic/errorprone/documentation/Compilation.java index 452b5d2df5..8e1273e70e 100644 --- a/documentation-support/src/test/java/tech/picnic/errorprone/documentation/Compilation.java +++ b/documentation-support/src/test/java/tech/picnic/errorprone/documentation/Compilation.java @@ -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 @@ -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 @@ -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 options, JavaFileObject javaFileObject) { diff --git a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormat.java b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormat.java index c06a8387ab..603de6b704 100644 --- a/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormat.java +++ b/error-prone-guidelines/src/main/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormat.java @@ -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 { @@ -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")); @@ -100,10 +95,6 @@ public final class TestHelperSourceFormat extends BugChecker .named("addOutputLines"); private static final Supplier 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); @@ -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); } @@ -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("\\", "\\\\") - .replace(TEXT_BLOCK_MARKER, "\"\"\\\"") - + TEXT_BLOCK_MARKER; + .replace(SourceCode.TEXT_BLOCK_DELIMITER, "\"\"\\\"") + + 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(", ")); @@ -268,7 +259,7 @@ private static Optional 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)) .filter(CharMatcher.whitespace()::matchesAllOf); } @@ -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 getConstantSourceCode( - List 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 appear that they {@link SourceVersion#supportsTextBlocks(Context) aren't}. diff --git a/error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormatTest.java b/error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormatTest.java index c0c1a11f2c..317540eb94 100644 --- a/error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormatTest.java +++ b/error-prone-guidelines/src/test/java/tech/picnic/errorprone/guidelines/bugpatterns/TestHelperSourceFormatTest.java @@ -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()) diff --git a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java index a79310a00d..b9dc4e2958 100644 --- a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java +++ b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java @@ -10,6 +10,7 @@ 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; @@ -17,6 +18,7 @@ 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; /** @@ -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() {} @@ -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); } /** @@ -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 joinConstantSourceCodeLines( + List sourceLines) { + StringBuilder source = new StringBuilder(); + + for (ExpressionTree sourceLine : sourceLines) { + if (!source.isEmpty()) { + // XXX: Review whether we can/should use `System.lineSeparator()` here. + source.append('\n'); + } + + String value = ASTHelpers.constValue(sourceLine, String.class); + if (value == null) { + return Optional.empty(); + } + + source.append(value); + } + + return Optional.of(source.toString()); + } + /** * Creates a {@link SuggestedFix} for the deletion of the given {@link Tree}, including any * whitespace that follows it. diff --git a/pom.xml b/pom.xml index 1101e2729b..30e2e8bd68 100644 --- a/pom.xml +++ b/pom.xml @@ -444,6 +444,11 @@ value-annotations 2.10.1 + + org.jetbrains + annotations + 24.1.0 + org.jspecify jspecify