From 01b30d767b4033e05c1dbd033bfeacbdde96ef2d Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Tue, 24 Oct 2023 13:57:37 +0200 Subject: [PATCH] Introduce `BugPatternTestExtractor` for documentation generation (#494) This new `Extractor` implementation collects identification and replacement source code from `BugChecker` unit tests. While there: - Refactor the existing `Extractor` setup such that instances are service-loaded and need to implement only a single method, thereby avoiding the need to align logic between multiple source code locations. - Extend the validation performed by the `Compilation` test helper class. - Extend the `ErrorProneTestHelperSourceFormat` check to support source code passed to the `Compilation` test helper class. --- .../documentation/BugPatternExtractor.java | 52 +- .../BugPatternTestExtractor.java | 229 +++++++++ .../DocumentationGeneratorTaskListener.java | 36 +- .../errorprone/documentation/Extractor.java | 24 +- .../documentation/ExtractorType.java | 36 -- .../BugPatternExtractorTest.java | 69 +-- .../BugPatternTestExtractorTest.java | 468 ++++++++++++++++++ .../errorprone/documentation/Compilation.java | 45 +- ...ocumentationGeneratorTaskListenerTest.java | 62 +++ ...orTest-bugpattern-CompleteBugChecker.json} | 0 ...torTest-bugpattern-MinimalBugChecker.json} | 0 ...rn-UndocumentedSuppressionBugPattern.json} | 0 ...dBugCheckerRefactoringTestHelpersTest.json | 24 + ...sWithCustomCheckerPackageAndNamesTest.json | 24 + ...leBugCheckerRefactoringTestHelperTest.json | 20 + ...st-MultiFileCompilationTestHelperTest.json | 18 + ...leBugCheckerRefactoringTestHelperTest.json | 15 + ...etArgsFixChooserAndCustomTestModeTest.json | 15 + ...t-SingleFileCompilationTestHelperTest.json | 14 + ...eCompilationTestHelperWithSetArgsTest.json | 14 + .../ErrorProneTestHelperSourceFormat.java | 14 +- 21 files changed, 1033 insertions(+), 146 deletions(-) create mode 100644 documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java delete mode 100644 documentation-support/src/main/java/tech/picnic/errorprone/documentation/ExtractorType.java create mode 100644 documentation-support/src/test/java/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest.java rename documentation-support/src/test/resources/tech/picnic/errorprone/documentation/{bugpattern-documentation-complete.json => BugPatternExtractorTest-bugpattern-CompleteBugChecker.json} (100%) rename documentation-support/src/test/resources/tech/picnic/errorprone/documentation/{bugpattern-documentation-minimal.json => BugPatternExtractorTest-bugpattern-MinimalBugChecker.json} (100%) rename documentation-support/src/test/resources/tech/picnic/errorprone/documentation/{bugpattern-documentation-undocumented-suppression.json => BugPatternExtractorTest-bugpattern-UndocumentedSuppressionBugPattern.json} (100%) create mode 100644 documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-CompilationAndBugCheckerRefactoringTestHelpersTest.json create mode 100644 documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest.json create mode 100644 documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-MultiFileBugCheckerRefactoringTestHelperTest.json create mode 100644 documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-MultiFileCompilationTestHelperTest.json create mode 100644 documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileBugCheckerRefactoringTestHelperTest.json create mode 100644 documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestModeTest.json create mode 100644 documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileCompilationTestHelperTest.json create mode 100644 documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileCompilationTestHelperWithSetArgsTest.json diff --git a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternExtractor.java b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternExtractor.java index 071b3f91d0..4f21588744 100644 --- a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternExtractor.java +++ b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternExtractor.java @@ -5,17 +5,19 @@ import static java.util.Objects.requireNonNull; import com.google.auto.common.AnnotationMirrors; +import com.google.auto.service.AutoService; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; import com.google.errorprone.annotations.Immutable; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; import com.sun.tools.javac.code.Attribute; import com.sun.tools.javac.code.Symbol.ClassSymbol; -import com.sun.tools.javac.util.Context; +import java.util.Optional; import javax.lang.model.element.AnnotationValue; import tech.picnic.errorprone.documentation.BugPatternExtractor.BugPatternDocumentation; @@ -23,29 +25,39 @@ * An {@link Extractor} that describes how to extract data from a {@code @BugPattern} annotation. */ @Immutable -final class BugPatternExtractor implements Extractor { +@AutoService(Extractor.class) +@SuppressWarnings("rawtypes" /* See https://github.com/google/auto/issues/870. */) +public final class BugPatternExtractor implements Extractor { + /** Instantiates a new {@link BugPatternExtractor} instance. */ + public BugPatternExtractor() {} + @Override - public BugPatternDocumentation extract(ClassTree tree, Context context) { - ClassSymbol symbol = ASTHelpers.getSymbol(tree); - BugPattern annotation = symbol.getAnnotation(BugPattern.class); - requireNonNull(annotation, "BugPattern annotation must be present"); - - return new AutoValue_BugPatternExtractor_BugPatternDocumentation( - symbol.getQualifiedName().toString(), - annotation.name().isEmpty() ? tree.getSimpleName().toString() : annotation.name(), - ImmutableList.copyOf(annotation.altNames()), - annotation.link(), - ImmutableList.copyOf(annotation.tags()), - annotation.summary(), - annotation.explanation(), - annotation.severity(), - annotation.disableable(), - annotation.documentSuppression() ? getSuppressionAnnotations(tree) : ImmutableList.of()); + public String identifier() { + return "bugpattern"; } @Override - public boolean canExtract(ClassTree tree) { - return ASTHelpers.hasDirectAnnotationWithSimpleName(tree, BugPattern.class.getSimpleName()); + public Optional tryExtract(ClassTree tree, VisitorState state) { + ClassSymbol symbol = ASTHelpers.getSymbol(tree); + BugPattern annotation = symbol.getAnnotation(BugPattern.class); + if (annotation == null) { + return Optional.empty(); + } + + return Optional.of( + new AutoValue_BugPatternExtractor_BugPatternDocumentation( + symbol.getQualifiedName().toString(), + annotation.name().isEmpty() ? tree.getSimpleName().toString() : annotation.name(), + ImmutableList.copyOf(annotation.altNames()), + annotation.link(), + ImmutableList.copyOf(annotation.tags()), + annotation.summary(), + annotation.explanation(), + annotation.severity(), + annotation.disableable(), + annotation.documentSuppression() + ? getSuppressionAnnotations(tree) + : ImmutableList.of())); } /** 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 new file mode 100644 index 0000000000..cf7f7a0b0e --- /dev/null +++ b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/BugPatternTestExtractor.java @@ -0,0 +1,229 @@ +package tech.picnic.errorprone.documentation; + +import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static java.util.function.Predicate.not; + +import com.google.auto.service.AutoService; +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.VisitorState; +import com.google.errorprone.annotations.Immutable; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.util.TreeScanner; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import org.jspecify.annotations.Nullable; +import tech.picnic.errorprone.documentation.BugPatternTestExtractor.TestCases; + +/** + * An {@link Extractor} that describes how to extract data from classes that test a {@code + * BugChecker}. + */ +// XXX: Handle other methods from `{BugCheckerRefactoring,Compilation}TestHelper`: +// - Indicate which custom arguments are specified, if any. +// - For replacement tests, indicate which `FixChooser` is used. +// - ... (We don't use all optional features; TBD what else to support.) +@Immutable +@AutoService(Extractor.class) +@SuppressWarnings("rawtypes" /* See https://github.com/google/auto/issues/870. */) +public final class BugPatternTestExtractor implements Extractor { + /** Instantiates a new {@link BugPatternTestExtractor} instance. */ + public BugPatternTestExtractor() {} + + @Override + public String identifier() { + return "bugpattern-test"; + } + + @Override + public Optional tryExtract(ClassTree tree, VisitorState state) { + BugPatternTestCollector collector = new BugPatternTestCollector(); + + collector.scan(tree, state); + + return Optional.of(collector.getCollectedTests()) + .filter(not(ImmutableList::isEmpty)) + .map( + tests -> + new AutoValue_BugPatternTestExtractor_TestCases( + ASTHelpers.getSymbol(tree).className(), tests)); + } + + private static final class BugPatternTestCollector + extends TreeScanner<@Nullable Void, VisitorState> { + private static final Matcher COMPILATION_HELPER_DO_TEST = + instanceMethod() + .onDescendantOf("com.google.errorprone.CompilationTestHelper") + .named("doTest"); + private static final Matcher TEST_HELPER_NEW_INSTANCE = + staticMethod() + .onDescendantOfAny( + "com.google.errorprone.CompilationTestHelper", + "com.google.errorprone.BugCheckerRefactoringTestHelper") + .named("newInstance") + .withParameters("java.lang.Class", "java.lang.Class"); + private static final Matcher IDENTIFICATION_SOURCE_LINES = + instanceMethod() + .onDescendantOf("com.google.errorprone.CompilationTestHelper") + .named("addSourceLines"); + private static final Matcher REPLACEMENT_DO_TEST = + instanceMethod() + .onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper") + .named("doTest"); + private static final Matcher REPLACEMENT_EXPECT_UNCHANGED = + instanceMethod() + .onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput") + .named("expectUnchanged"); + private static final Matcher REPLACEMENT_OUTPUT_SOURCE_LINES = + instanceMethod() + .onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput") + .namedAnyOf("addOutputLines", "expectUnchanged"); + + private final List collectedTestCases = new ArrayList<>(); + + private ImmutableList getCollectedTests() { + return ImmutableList.copyOf(collectedTestCases); + } + + @Override + public @Nullable Void visitMethodInvocation(MethodInvocationTree node, VisitorState state) { + boolean isReplacementTest = REPLACEMENT_DO_TEST.matches(node, state); + if (isReplacementTest || COMPILATION_HELPER_DO_TEST.matches(node, state)) { + getClassUnderTest(node, state) + .ifPresent( + classUnderTest -> { + List entries = new ArrayList<>(); + if (isReplacementTest) { + extractReplacementTestCases(node, entries, state); + } else { + extractIdentificationTestCases(node, entries, state); + } + + if (!entries.isEmpty()) { + collectedTestCases.add( + new AutoValue_BugPatternTestExtractor_TestCase( + classUnderTest, ImmutableList.copyOf(entries).reverse())); + } + }); + } + + return super.visitMethodInvocation(node, state); + } + + private static Optional getClassUnderTest( + MethodInvocationTree tree, VisitorState state) { + if (TEST_HELPER_NEW_INSTANCE.matches(tree, state)) { + return Optional.ofNullable(ASTHelpers.getSymbol(tree.getArguments().get(0))) + .filter(s -> !s.type.allparams().isEmpty()) + .map(s -> s.type.allparams().get(0).tsym.getQualifiedName().toString()); + } + + ExpressionTree receiver = ASTHelpers.getReceiver(tree); + return receiver instanceof MethodInvocationTree + ? getClassUnderTest((MethodInvocationTree) receiver, state) + : Optional.empty(); + } + + private static void extractIdentificationTestCases( + MethodInvocationTree tree, List sink, VisitorState state) { + if (IDENTIFICATION_SOURCE_LINES.matches(tree, state)) { + String path = ASTHelpers.constValue(tree.getArguments().get(0), String.class); + Optional sourceCode = + getSourceCode(tree).filter(s -> s.contains("// BUG: Diagnostic")); + if (path != null && sourceCode.isPresent()) { + sink.add( + new AutoValue_BugPatternTestExtractor_IdentificationTestEntry( + path, sourceCode.orElseThrow())); + } + } + + ExpressionTree receiver = ASTHelpers.getReceiver(tree); + if (receiver instanceof MethodInvocationTree) { + extractIdentificationTestCases((MethodInvocationTree) receiver, sink, state); + } + } + + private static void extractReplacementTestCases( + MethodInvocationTree tree, List sink, VisitorState state) { + if (REPLACEMENT_OUTPUT_SOURCE_LINES.matches(tree, state)) { + /* + * Retrieve the method invocation that contains the input source code. Note that this cast + * is safe, because this code is guarded by an earlier call to `#getClassUnderTest(..)`, + * which ensures that `tree` is part of a longer method invocation chain. + */ + MethodInvocationTree inputTree = (MethodInvocationTree) ASTHelpers.getReceiver(tree); + + String path = ASTHelpers.constValue(inputTree.getArguments().get(0), String.class); + Optional inputCode = getSourceCode(inputTree); + if (path != null && inputCode.isPresent()) { + Optional outputCode = + REPLACEMENT_EXPECT_UNCHANGED.matches(tree, state) ? inputCode : getSourceCode(tree); + + if (outputCode.isPresent() && !inputCode.equals(outputCode)) { + sink.add( + new AutoValue_BugPatternTestExtractor_ReplacementTestEntry( + path, inputCode.orElseThrow(), outputCode.orElseThrow())); + } + } + } + + ExpressionTree receiver = ASTHelpers.getReceiver(tree); + if (receiver instanceof MethodInvocationTree) { + extractReplacementTestCases((MethodInvocationTree) receiver, sink, state); + } + } + + // 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()); + } + } + + @AutoValue + abstract static class TestCases { + abstract String testClass(); + + abstract ImmutableList testCases(); + } + + @AutoValue + abstract static class TestCase { + abstract String classUnderTest(); + + abstract ImmutableList entries(); + } + + interface TestEntry { + String path(); + } + + @AutoValue + abstract static class ReplacementTestEntry implements TestEntry { + abstract String input(); + + abstract String output(); + } + + @AutoValue + abstract static class IdentificationTestEntry implements TestEntry { + abstract String code(); + } +} diff --git a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/DocumentationGeneratorTaskListener.java b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/DocumentationGeneratorTaskListener.java index 29fa2203d2..07261650ee 100644 --- a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/DocumentationGeneratorTaskListener.java +++ b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/DocumentationGeneratorTaskListener.java @@ -5,10 +5,14 @@ import com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility; import com.fasterxml.jackson.annotation.PropertyAccessor; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.VisitorState; import com.sun.source.tree.ClassTree; +import com.sun.source.tree.CompilationUnitTree; import com.sun.source.util.TaskEvent; import com.sun.source.util.TaskEvent.Kind; import com.sun.source.util.TaskListener; +import com.sun.source.util.TreePath; import com.sun.tools.javac.api.JavacTrees; import com.sun.tools.javac.util.Context; import java.io.File; @@ -19,6 +23,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.ServiceLoader; import javax.tools.JavaFileObject; /** @@ -27,6 +32,13 @@ */ // XXX: Find a better name for this class; it doesn't generate documentation per se. final class DocumentationGeneratorTaskListener implements TaskListener { + @SuppressWarnings({"rawtypes", "unchecked"}) + private static final ImmutableList> EXTRACTORS = + (ImmutableList) + ImmutableList.copyOf( + ServiceLoader.load( + Extractor.class, DocumentationGeneratorTaskListener.class.getClassLoader())); + private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper().setVisibility(PropertyAccessor.FIELD, Visibility.ANY); @@ -51,19 +63,25 @@ public void finished(TaskEvent taskEvent) { return; } - ClassTree classTree = JavacTrees.instance(context).getTree(taskEvent.getTypeElement()); JavaFileObject sourceFile = taskEvent.getSourceFile(); - if (classTree == null || sourceFile == null) { + CompilationUnitTree compilationUnit = taskEvent.getCompilationUnit(); + ClassTree classTree = JavacTrees.instance(context).getTree(taskEvent.getTypeElement()); + if (sourceFile == null || compilationUnit == null || classTree == null) { return; } - ExtractorType.findMatchingType(classTree) - .ifPresent( - extractorType -> - writeToFile( - extractorType.getIdentifier(), - getSimpleClassName(sourceFile.toUri()), - extractorType.getExtractor().extract(classTree, context))); + VisitorState state = + VisitorState.createForUtilityPurposes(context) + .withPath(new TreePath(new TreePath(compilationUnit), classTree)); + + for (Extractor extractor : EXTRACTORS) { + extractor + .tryExtract(classTree, state) + .ifPresent( + data -> + writeToFile( + extractor.identifier(), getSimpleClassName(sourceFile.toUri()), data)); + } } private void createDocsDirectory() { diff --git a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/Extractor.java b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/Extractor.java index 210c84ab94..4a746fe0db 100644 --- a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/Extractor.java +++ b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/Extractor.java @@ -1,8 +1,9 @@ package tech.picnic.errorprone.documentation; +import com.google.errorprone.VisitorState; import com.google.errorprone.annotations.Immutable; import com.sun.source.tree.ClassTree; -import com.sun.tools.javac.util.Context; +import java.util.Optional; /** * Interface implemented by classes that define how to extract data of some type {@link T} from a @@ -13,21 +14,20 @@ @Immutable interface Extractor { /** - * Extracts and returns an instance of {@link T} using the provided arguments. + * Returns the unique identifier of this extractor. * - * @param tree The {@link ClassTree} to analyze and from which to extract instances of {@link T}. - * @param context The {@link Context} in which the current compilation takes place. - * @return A non-null instance of {@link T}. + * @return A non-{@code null} string. */ - // XXX: Drop `Context` parameter unless used. - T extract(ClassTree tree, Context context); + String identifier(); /** - * Tells whether this {@link Extractor} can extract documentation content from the given {@link - * ClassTree}. + * Attempts to extract an instance of type {@link T} using the provided arguments. * - * @param tree The {@link ClassTree} of interest. - * @return {@code true} iff data extraction is supported. + * @param tree The {@link ClassTree} to analyze and from which to extract an instance of type + * {@link T}. + * @param state A {@link VisitorState} describing the context in which the given {@link ClassTree} + * is found. + * @return An instance of type {@link T}, if possible. */ - boolean canExtract(ClassTree tree); + Optional tryExtract(ClassTree tree, VisitorState state); } diff --git a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/ExtractorType.java b/documentation-support/src/main/java/tech/picnic/errorprone/documentation/ExtractorType.java deleted file mode 100644 index 8ba9e8b8d0..0000000000 --- a/documentation-support/src/main/java/tech/picnic/errorprone/documentation/ExtractorType.java +++ /dev/null @@ -1,36 +0,0 @@ -package tech.picnic.errorprone.documentation; - -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Sets; -import com.sun.source.tree.ClassTree; -import java.util.EnumSet; -import java.util.Optional; - -/** An enumeration of {@link Extractor} types. */ -enum ExtractorType { - BUG_PATTERN("bugpattern", new BugPatternExtractor()); - - private static final ImmutableSet TYPES = - Sets.immutableEnumSet(EnumSet.allOf(ExtractorType.class)); - - private final String identifier; - private final Extractor extractor; - - ExtractorType(String identifier, Extractor extractor) { - this.identifier = identifier; - this.extractor = extractor; - } - - String getIdentifier() { - return identifier; - } - - @SuppressWarnings("java:S1452" /* The extractor returns data of an unspecified type. */) - Extractor getExtractor() { - return extractor; - } - - static Optional findMatchingType(ClassTree tree) { - return TYPES.stream().filter(type -> type.getExtractor().canExtract(tree)).findFirst(); - } -} diff --git a/documentation-support/src/test/java/tech/picnic/errorprone/documentation/BugPatternExtractorTest.java b/documentation-support/src/test/java/tech/picnic/errorprone/documentation/BugPatternExtractorTest.java index da6467c78d..137b2aa88e 100644 --- a/documentation-support/src/test/java/tech/picnic/errorprone/documentation/BugPatternExtractorTest.java +++ b/documentation-support/src/test/java/tech/picnic/errorprone/documentation/BugPatternExtractorTest.java @@ -1,18 +1,9 @@ package tech.picnic.errorprone.documentation; -import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import com.google.common.io.Resources; -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.ClassTreeMatcher; -import com.google.errorprone.matchers.Description; -import com.sun.source.tree.ClassTree; import java.io.IOException; import java.nio.file.Path; import org.junit.jupiter.api.Test; @@ -45,10 +36,7 @@ void minimalBugPattern(@TempDir Path outputDirectory) throws IOException { "@BugPattern(summary = \"MinimalBugChecker summary\", severity = SeverityLevel.ERROR)", "public final class MinimalBugChecker extends BugChecker {}"); - verifyFileMatchesResource( - outputDirectory, - "bugpattern-MinimalBugChecker.json", - "bugpattern-documentation-minimal.json"); + verifyGeneratedFileContent(outputDirectory, "MinimalBugChecker"); } @Test @@ -76,10 +64,7 @@ void completeBugPattern(@TempDir Path outputDirectory) throws IOException { " suppressionAnnotations = {BugPattern.class, Test.class})", "public final class CompleteBugChecker extends BugChecker {}"); - verifyFileMatchesResource( - outputDirectory, - "bugpattern-CompleteBugChecker.json", - "bugpattern-documentation-complete.json"); + verifyGeneratedFileContent(outputDirectory, "CompleteBugChecker"); } @Test @@ -99,55 +84,23 @@ void undocumentedSuppressionBugPattern(@TempDir Path outputDirectory) throws IOE " documentSuppression = false)", "public final class UndocumentedSuppressionBugPattern extends BugChecker {}"); - verifyFileMatchesResource( - outputDirectory, - "bugpattern-UndocumentedSuppressionBugPattern.json", - "bugpattern-documentation-undocumented-suppression.json"); - } - - @Test - void bugPatternAnnotationIsAbsent() { - CompilationTestHelper.newInstance(TestChecker.class, getClass()) - .addSourceLines( - "TestChecker.java", - "import com.google.errorprone.bugpatterns.BugChecker;", - "", - "// BUG: Diagnostic contains: Can extract: false", - "public final class TestChecker extends BugChecker {}") - .doTest(); + verifyGeneratedFileContent(outputDirectory, "UndocumentedSuppressionBugPattern"); } - private static void verifyFileMatchesResource( - Path outputDirectory, String fileName, String resourceName) throws IOException { - assertThat(outputDirectory.resolve(fileName)) + private static void verifyGeneratedFileContent(Path outputDirectory, String testClass) + throws IOException { + String resourceName = String.format("bugpattern-%s.json", testClass); + assertThat(outputDirectory.resolve(resourceName)) .content(UTF_8) - .isEqualToIgnoringWhitespace(getResource(resourceName)); + .isEqualToIgnoringWhitespace( + getResource( + String.join("-", BugPatternExtractorTest.class.getSimpleName(), resourceName))); } // XXX: Once we support only JDK 15+, drop this method in favour of including the resources as - // text blocks in this class. (This also requires renaming the `verifyFileMatchesResource` - // method.) + // text blocks in this class. private static String getResource(String resourceName) throws IOException { return Resources.toString( Resources.getResource(BugPatternExtractorTest.class, resourceName), UTF_8); } - - /** A {@link BugChecker} that validates the {@link BugPatternExtractor}. */ - @BugPattern(summary = "Validates `BugPatternExtractor` extraction", severity = ERROR) - public static final class TestChecker extends BugChecker implements ClassTreeMatcher { - private static final long serialVersionUID = 1L; - - @Override - public Description matchClass(ClassTree tree, VisitorState state) { - BugPatternExtractor extractor = new BugPatternExtractor(); - - assertThatThrownBy(() -> extractor.extract(tree, state.context)) - .isInstanceOf(NullPointerException.class) - .hasMessage("BugPattern annotation must be present"); - - return buildDescription(tree) - .setMessage(String.format("Can extract: %s", extractor.canExtract(tree))) - .build(); - } - } } diff --git a/documentation-support/src/test/java/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest.java b/documentation-support/src/test/java/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest.java new file mode 100644 index 0000000000..ecd6d2032c --- /dev/null +++ b/documentation-support/src/test/java/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest.java @@ -0,0 +1,468 @@ +package tech.picnic.errorprone.documentation; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.assertj.core.api.Assertions.assertThat; + +import com.google.common.io.Resources; +import java.io.IOException; +import java.nio.file.Path; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +final class BugPatternTestExtractorTest { + @Test + void noTestClass(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "TestCheckerWithoutAnnotation.java", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "public final class TestCheckerWithoutAnnotation extends BugChecker {}"); + + assertThat(outputDirectory.toAbsolutePath()).isEmptyDirectory(); + } + + @Test + void noDoTestInvocation(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "TestCheckerTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class TestCheckerTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(TestChecker.class, getClass())", + " .addSourceLines(\"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\");", + "", + " BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass())", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\");", + " }", + "}"); + + assertThat(outputDirectory.toAbsolutePath()).isEmptyDirectory(); + } + + @Test + void nullBugCheckerInstance(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "TestCheckerTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class TestCheckerTest {", + " void m() {", + " CompilationTestHelper.newInstance((Class) null, getClass())", + " .addSourceLines(\"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\")", + " .doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance((Class) null, getClass())", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\")", + " .doTest();", + " }", + "}"); + + assertThat(outputDirectory.toAbsolutePath()).isEmptyDirectory(); + } + + @Test + void rawBugCheckerInstance(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "TestCheckerTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class TestCheckerTest {", + " private static class TestChecker extends BugChecker {}", + "", + " @SuppressWarnings(\"unchecked\")", + " void m() {", + " @SuppressWarnings(\"rawtypes\")", + " Class bugChecker = TestChecker.class;", + "", + " CompilationTestHelper.newInstance(bugChecker, getClass())", + " .addSourceLines(\"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\")", + " .doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance(bugChecker, getClass())", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\")", + " .doTest();", + " }", + "}"); + + assertThat(outputDirectory.toAbsolutePath()).isEmptyDirectory(); + } + + @Test + void scannerSupplierInstance(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "TestCheckerTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "import com.google.errorprone.scanner.ScannerSupplier;", + "", + "final class TestCheckerTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(", + " ScannerSupplier.fromBugCheckerClasses(TestChecker.class), getClass())", + " .addSourceLines(\"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\")", + " .doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance(", + " ScannerSupplier.fromBugCheckerClasses(TestChecker.class), getClass())", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\")", + " .doTest();", + " }", + "}"); + + assertThat(outputDirectory.toAbsolutePath()).isEmptyDirectory(); + } + + @Test + void nonCompileTimeConstantStrings(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "TestCheckerTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class TestCheckerTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(TestChecker.class, getClass())", + " .addSourceLines(toString() + \"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\")", + " .addSourceLines(\"B.java\", \"// BUG: Diagnostic contains:\", \"class B {}\", toString())", + " .doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass())", + " .addInputLines(toString() + \"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\")", + " .addInputLines(\"B.java\", \"class B {}\", toString())", + " .addOutputLines(\"B.java\", \"class B { /* This is a change. */ }\")", + " .addInputLines(\"C.java\", \"class C {}\")", + " .addOutputLines(\"C.java\", \"class C { /* This is a change. */ }\", toString())", + " .doTest();", + " }", + "}"); + + assertThat(outputDirectory.toAbsolutePath()).isEmptyDirectory(); + } + + @Test + void nonFluentTestHelperExpressions(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "TestCheckerTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class TestCheckerTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper testHelper =", + " CompilationTestHelper.newInstance(TestChecker.class, getClass())", + " .addSourceLines(\"A.java\", \"class A {}\");", + " testHelper.doTest();", + "", + " BugCheckerRefactoringTestHelper.ExpectOutput expectedOutput =", + " BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass())", + " .addInputLines(\"A.java\", \"class A {}\");", + " expectedOutput.addOutputLines(\"A.java\", \"class A {}\").doTest();", + " expectedOutput.expectUnchanged().doTest();", + " }", + "}"); + + assertThat(outputDirectory.toAbsolutePath()).isEmptyDirectory(); + } + + @Test + void noSource(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "TestCheckerTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class TestCheckerTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(TestChecker.class, getClass()).doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass()).doTest();", + " }", + "}"); + + assertThat(outputDirectory.toAbsolutePath()).isEmptyDirectory(); + } + + @Test + void noDiagnostics(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "TestCheckerTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class TestCheckerTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(TestChecker.class, getClass())", + " .addSourceLines(\"A.java\", \"class A {}\")", + " .doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass())", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A {}\")", + " .addInputLines(\"B.java\", \"class B {}\")", + " .expectUnchanged()", + " .doTest();", + " }", + "}"); + + assertThat(outputDirectory.toAbsolutePath()).isEmptyDirectory(); + } + + @Test + void singleFileCompilationTestHelper(@TempDir Path outputDirectory) throws IOException { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "SingleFileCompilationTestHelperTest.java", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class SingleFileCompilationTestHelperTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(TestChecker.class, getClass())", + " .addSourceLines(\"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\")", + " .doTest();", + " }", + "}"); + + verifyGeneratedFileContent(outputDirectory, "SingleFileCompilationTestHelperTest"); + } + + @Test + void singleFileCompilationTestHelperWithSetArgs(@TempDir Path outputDirectory) + throws IOException { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "SingleFileCompilationTestHelperWithSetArgsTest.java", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class SingleFileCompilationTestHelperWithSetArgsTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(TestChecker.class, getClass())", + " .setArgs(\"-XepAllSuggestionsAsWarnings\")", + " .addSourceLines(\"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\")", + " .doTest();", + " }", + "}"); + + verifyGeneratedFileContent(outputDirectory, "SingleFileCompilationTestHelperWithSetArgsTest"); + } + + @Test + void multiFileCompilationTestHelper(@TempDir Path outputDirectory) throws IOException { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "MultiFileCompilationTestHelperTest.java", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class MultiFileCompilationTestHelperTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(TestChecker.class, getClass())", + " .addSourceLines(\"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\")", + " .addSourceLines(\"B.java\", \"// BUG: Diagnostic contains:\", \"class B {}\")", + " .doTest();", + " }", + "}"); + + verifyGeneratedFileContent(outputDirectory, "MultiFileCompilationTestHelperTest"); + } + + @Test + void singleFileBugCheckerRefactoringTestHelper(@TempDir Path outputDirectory) throws IOException { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "SingleFileBugCheckerRefactoringTestHelperTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class SingleFileBugCheckerRefactoringTestHelperTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass())", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\")", + " .doTest();", + " }", + "}"); + + verifyGeneratedFileContent(outputDirectory, "SingleFileBugCheckerRefactoringTestHelperTest"); + } + + @Test + void singleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestMode( + @TempDir Path outputDirectory) throws IOException { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "SingleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestModeTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers;", + "import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class SingleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestModeTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass())", + " .setArgs(\"-XepAllSuggestionsAsWarnings\")", + " .setFixChooser(FixChoosers.SECOND)", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\")", + " .doTest(TestMode.TEXT_MATCH);", + " }", + "}"); + + verifyGeneratedFileContent( + outputDirectory, + "SingleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestModeTest"); + } + + @Test + void multiFileBugCheckerRefactoringTestHelper(@TempDir Path outputDirectory) throws IOException { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "MultiFileBugCheckerRefactoringTestHelperTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class MultiFileBugCheckerRefactoringTestHelperTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass())", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\")", + " .addInputLines(\"B.java\", \"class B {}\")", + " .addOutputLines(\"B.java\", \"class B { /* This is a change. */ }\")", + " .doTest();", + " }", + "}"); + + verifyGeneratedFileContent(outputDirectory, "MultiFileBugCheckerRefactoringTestHelperTest"); + } + + @Test + void compilationAndBugCheckerRefactoringTestHelpers(@TempDir Path outputDirectory) + throws IOException { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "CompilationAndBugCheckerRefactoringTestHelpersTest.java", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class CompilationAndBugCheckerRefactoringTestHelpersTest {", + " private static class TestChecker extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(TestChecker.class, getClass())", + " .addSourceLines(\"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\")", + " .doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance(TestChecker.class, getClass())", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\")", + " .doTest();", + " }", + "}"); + + verifyGeneratedFileContent( + outputDirectory, "CompilationAndBugCheckerRefactoringTestHelpersTest"); + } + + @Test + void compilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNames( + @TempDir Path outputDirectory) throws IOException { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest.java", + "package pkg;", + "", + "import com.google.errorprone.BugCheckerRefactoringTestHelper;", + "import com.google.errorprone.CompilationTestHelper;", + "import com.google.errorprone.bugpatterns.BugChecker;", + "", + "final class CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest {", + " private static class CustomTestChecker extends BugChecker {}", + "", + " private static class CustomTestChecker2 extends BugChecker {}", + "", + " void m() {", + " CompilationTestHelper.newInstance(CustomTestChecker.class, getClass())", + " .addSourceLines(\"A.java\", \"// BUG: Diagnostic contains:\", \"class A {}\")", + " .doTest();", + "", + " BugCheckerRefactoringTestHelper.newInstance(CustomTestChecker2.class, getClass())", + " .addInputLines(\"A.java\", \"class A {}\")", + " .addOutputLines(\"A.java\", \"class A { /* This is a change. */ }\")", + " .doTest();", + " }", + "}"); + + verifyGeneratedFileContent( + outputDirectory, + "CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest"); + } + + private static void verifyGeneratedFileContent(Path outputDirectory, String testClass) + throws IOException { + String resourceName = String.format("bugpattern-test-%s.json", testClass); + assertThat(outputDirectory.resolve(resourceName)) + .content(UTF_8) + .isEqualToIgnoringWhitespace( + getResource( + String.join("-", BugPatternTestExtractorTest.class.getSimpleName(), resourceName))); + } + + // XXX: Once we support only JDK 15+, drop this method in favour of including the resources as + // text blocks in this class. + private static String getResource(String resourceName) throws IOException { + return Resources.toString( + Resources.getResource(BugPatternTestExtractorTest.class, resourceName), UTF_8); + } +} 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 26d5004c58..452b5d2df5 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 @@ -1,5 +1,7 @@ package tech.picnic.errorprone.documentation; +import static org.assertj.core.api.Assertions.assertThat; + import com.google.common.collect.ImmutableList; import com.google.errorprone.FileManagers; import com.google.errorprone.FileObjects; @@ -7,39 +9,66 @@ import com.sun.tools.javac.api.JavacTool; import com.sun.tools.javac.file.JavacFileManager; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import javax.tools.Diagnostic; import javax.tools.JavaCompiler; import javax.tools.JavaFileObject; // XXX: Generalize and move this class so that it can also be used by `refaster-compiler`. -// XXX: Add support for this class to the `ErrorProneTestHelperSourceFormat` check. +// XXX: This class is supported by the `ErrorProneTestHelperSourceFormat` check, but until that +// support is covered by unit tests, make sure to update that logic if this class or its methods are +// moved/renamed. public final class Compilation { private Compilation() {} public static void compileWithDocumentationGenerator( - Path outputDirectory, String fileName, String... lines) { - compileWithDocumentationGenerator(outputDirectory.toAbsolutePath().toString(), fileName, lines); + Path outputDirectory, String path, String... lines) { + compileWithDocumentationGenerator(outputDirectory.toAbsolutePath().toString(), path, lines); } public static void compileWithDocumentationGenerator( - String outputDirectory, String fileName, String... lines) { + String outputDirectory, String path, String... lines) { + /* + * The compiler options specified here largely match those used by Error Prone's + * `CompilationTestHelper`. A key difference is the stricter linting configuration. When + * compiling using JDK 21+, these lint options also require that certain JDK modules are + * explicitly exported. + */ compile( - ImmutableList.of("-Xplugin:DocumentationGenerator -XoutputDirectory=" + outputDirectory), - FileObjects.forSourceLines(fileName, lines)); + ImmutableList.of( + "--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "-encoding", + "UTF-8", + "-parameters", + "-proc:none", + "-Werror", + "-Xlint:all,-serial", + "-Xplugin:DocumentationGenerator -XoutputDirectory=" + outputDirectory, + "-XDdev", + "-XDcompilePolicy=simple"), + FileObjects.forSourceLines(path, lines)); } private static void compile(ImmutableList options, JavaFileObject javaFileObject) { JavacFileManager javacFileManager = FileManagers.testFileManager(); JavaCompiler compiler = JavacTool.create(); + + List> diagnostics = new ArrayList<>(); JavacTaskImpl task = (JavacTaskImpl) compiler.getTask( null, javacFileManager, - null, + diagnostics::add, options, ImmutableList.of(), ImmutableList.of(javaFileObject)); - task.call(); + Boolean result = task.call(); + assertThat(diagnostics).isEmpty(); + assertThat(result).isTrue(); } } diff --git a/documentation-support/src/test/java/tech/picnic/errorprone/documentation/DocumentationGeneratorTaskListenerTest.java b/documentation-support/src/test/java/tech/picnic/errorprone/documentation/DocumentationGeneratorTaskListenerTest.java index b49e7ba17d..2394b15a14 100644 --- a/documentation-support/src/test/java/tech/picnic/errorprone/documentation/DocumentationGeneratorTaskListenerTest.java +++ b/documentation-support/src/test/java/tech/picnic/errorprone/documentation/DocumentationGeneratorTaskListenerTest.java @@ -1,19 +1,29 @@ package tech.picnic.errorprone.documentation; import static com.google.common.collect.ImmutableList.toImmutableList; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.nio.file.attribute.AclEntryPermission.ADD_SUBDIRECTORY; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.condition.OS.WINDOWS; +import com.google.auto.service.AutoService; +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; +import com.google.common.collect.Streams; +import com.google.errorprone.VisitorState; +import com.google.errorprone.annotations.Immutable; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.Tree; import java.io.IOException; import java.nio.file.FileSystemException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.attribute.AclEntry; import java.nio.file.attribute.AclFileAttributeView; +import java.util.Optional; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.DisabledOnOs; import org.junit.jupiter.api.condition.EnabledOnOs; @@ -75,4 +85,56 @@ void excessArguments(@TempDir Path outputDirectory) { .isInstanceOf(IllegalArgumentException.class) .hasMessage("Precisely one path must be provided"); } + + @Test + void extraction(@TempDir Path outputDirectory) { + Compilation.compileWithDocumentationGenerator( + outputDirectory, + "DocumentationGeneratorTaskListenerTestClass.java", + "class DocumentationGeneratorTaskListenerTestClass {}"); + + // XXX: Once we support only JDK 15+, use a text block for the `expected` string. + assertThat( + outputDirectory.resolve( + "documentation-generator-task-listener-test-DocumentationGeneratorTaskListenerTestClass.json")) + .content(UTF_8) + .isEqualToIgnoringWhitespace( + "{\"className\":\"DocumentationGeneratorTaskListenerTestClass\",\"path\":[\"CLASS: DocumentationGeneratorTaskListenerTestClass\",\"COMPILATION_UNIT\"]}"); + } + + @Immutable + @AutoService(Extractor.class) + @SuppressWarnings("rawtypes" /* See https://github.com/google/auto/issues/870. */) + public static final class TestExtractor implements Extractor { + @Override + public String identifier() { + return "documentation-generator-task-listener-test"; + } + + @Override + public Optional tryExtract(ClassTree tree, VisitorState state) { + return Optional.of(tree.getSimpleName().toString()) + .filter(n -> n.contains(DocumentationGeneratorTaskListenerTest.class.getSimpleName())) + .map( + className -> + new AutoValue_DocumentationGeneratorTaskListenerTest_ExtractionParameters( + className, + Streams.stream(state.getPath()) + .map(TestExtractor::describeTree) + .collect(toImmutableList()))); + } + + private static String describeTree(Tree tree) { + return (tree instanceof ClassTree) + ? String.join(": ", String.valueOf(tree.getKind()), ((ClassTree) tree).getSimpleName()) + : tree.getKind().toString(); + } + } + + @AutoValue + abstract static class ExtractionParameters { + abstract String className(); + + abstract ImmutableList path(); + } } diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/bugpattern-documentation-complete.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternExtractorTest-bugpattern-CompleteBugChecker.json similarity index 100% rename from documentation-support/src/test/resources/tech/picnic/errorprone/documentation/bugpattern-documentation-complete.json rename to documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternExtractorTest-bugpattern-CompleteBugChecker.json diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/bugpattern-documentation-minimal.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternExtractorTest-bugpattern-MinimalBugChecker.json similarity index 100% rename from documentation-support/src/test/resources/tech/picnic/errorprone/documentation/bugpattern-documentation-minimal.json rename to documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternExtractorTest-bugpattern-MinimalBugChecker.json diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/bugpattern-documentation-undocumented-suppression.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternExtractorTest-bugpattern-UndocumentedSuppressionBugPattern.json similarity index 100% rename from documentation-support/src/test/resources/tech/picnic/errorprone/documentation/bugpattern-documentation-undocumented-suppression.json rename to documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternExtractorTest-bugpattern-UndocumentedSuppressionBugPattern.json diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-CompilationAndBugCheckerRefactoringTestHelpersTest.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-CompilationAndBugCheckerRefactoringTestHelpersTest.json new file mode 100644 index 0000000000..c7672cccc4 --- /dev/null +++ b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-CompilationAndBugCheckerRefactoringTestHelpersTest.json @@ -0,0 +1,24 @@ +{ + "testClass": "CompilationAndBugCheckerRefactoringTestHelpersTest", + "testCases": [ + { + "classUnderTest": "CompilationAndBugCheckerRefactoringTestHelpersTest.TestChecker", + "entries": [ + { + "path": "A.java", + "code": "// BUG: Diagnostic contains:\nclass A {}\n" + } + ] + }, + { + "classUnderTest": "CompilationAndBugCheckerRefactoringTestHelpersTest.TestChecker", + "entries": [ + { + "path": "A.java", + "input": "class A {}\n", + "output": "class A { /* This is a change. */ }\n" + } + ] + } + ] +} diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest.json new file mode 100644 index 0000000000..49b1e6bb49 --- /dev/null +++ b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest.json @@ -0,0 +1,24 @@ +{ + "testClass": "pkg.CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest", + "testCases": [ + { + "classUnderTest": "pkg.CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest.CustomTestChecker", + "entries": [ + { + "path": "A.java", + "code": "// BUG: Diagnostic contains:\nclass A {}\n" + } + ] + }, + { + "classUnderTest": "pkg.CompilationAndBugCheckerRefactoringTestHelpersWithCustomCheckerPackageAndNamesTest.CustomTestChecker2", + "entries": [ + { + "path": "A.java", + "input": "class A {}\n", + "output": "class A { /* This is a change. */ }\n" + } + ] + } + ] +} diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-MultiFileBugCheckerRefactoringTestHelperTest.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-MultiFileBugCheckerRefactoringTestHelperTest.json new file mode 100644 index 0000000000..4f93dc9ce2 --- /dev/null +++ b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-MultiFileBugCheckerRefactoringTestHelperTest.json @@ -0,0 +1,20 @@ +{ + "testClass": "MultiFileBugCheckerRefactoringTestHelperTest", + "testCases": [ + { + "classUnderTest": "MultiFileBugCheckerRefactoringTestHelperTest.TestChecker", + "entries": [ + { + "path": "A.java", + "input": "class A {}\n", + "output": "class A { /* This is a change. */ }\n" + }, + { + "path": "B.java", + "input": "class B {}\n", + "output": "class B { /* This is a change. */ }\n" + } + ] + } + ] +} diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-MultiFileCompilationTestHelperTest.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-MultiFileCompilationTestHelperTest.json new file mode 100644 index 0000000000..608717802f --- /dev/null +++ b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-MultiFileCompilationTestHelperTest.json @@ -0,0 +1,18 @@ +{ + "testClass": "MultiFileCompilationTestHelperTest", + "testCases": [ + { + "classUnderTest": "MultiFileCompilationTestHelperTest.TestChecker", + "entries": [ + { + "path": "A.java", + "code": "// BUG: Diagnostic contains:\nclass A {}\n" + }, + { + "path": "B.java", + "code": "// BUG: Diagnostic contains:\nclass B {}\n" + } + ] + } + ] +} diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileBugCheckerRefactoringTestHelperTest.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileBugCheckerRefactoringTestHelperTest.json new file mode 100644 index 0000000000..636e19ebdf --- /dev/null +++ b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileBugCheckerRefactoringTestHelperTest.json @@ -0,0 +1,15 @@ +{ + "testClass": "SingleFileBugCheckerRefactoringTestHelperTest", + "testCases": [ + { + "classUnderTest": "SingleFileBugCheckerRefactoringTestHelperTest.TestChecker", + "entries": [ + { + "path": "A.java", + "input": "class A {}\n", + "output": "class A { /* This is a change. */ }\n" + } + ] + } + ] +} diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestModeTest.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestModeTest.json new file mode 100644 index 0000000000..4c2d66d89a --- /dev/null +++ b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestModeTest.json @@ -0,0 +1,15 @@ +{ + "testClass": "SingleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestModeTest", + "testCases": [ + { + "classUnderTest": "SingleFileBugCheckerRefactoringTestHelperWithSetArgsFixChooserAndCustomTestModeTest.TestChecker", + "entries": [ + { + "path": "A.java", + "input": "class A {}\n", + "output": "class A { /* This is a change. */ }\n" + } + ] + } + ] +} diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileCompilationTestHelperTest.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileCompilationTestHelperTest.json new file mode 100644 index 0000000000..37720e1ed5 --- /dev/null +++ b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileCompilationTestHelperTest.json @@ -0,0 +1,14 @@ +{ + "testClass": "SingleFileCompilationTestHelperTest", + "testCases": [ + { + "classUnderTest": "SingleFileCompilationTestHelperTest.TestChecker", + "entries": [ + { + "path": "A.java", + "code": "// BUG: Diagnostic contains:\nclass A {}\n" + } + ] + } + ] +} diff --git a/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileCompilationTestHelperWithSetArgsTest.json b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileCompilationTestHelperWithSetArgsTest.json new file mode 100644 index 0000000000..ea543ab7ed --- /dev/null +++ b/documentation-support/src/test/resources/tech/picnic/errorprone/documentation/BugPatternTestExtractorTest-bugpattern-test-SingleFileCompilationTestHelperWithSetArgsTest.json @@ -0,0 +1,14 @@ +{ + "testClass": "SingleFileCompilationTestHelperWithSetArgsTest", + "testCases": [ + { + "classUnderTest": "SingleFileCompilationTestHelperWithSetArgsTest.TestChecker", + "entries": [ + { + "path": "A.java", + "code": "// BUG: Diagnostic contains:\nclass A {}\n" + } + ] + } + ] +} 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 3663816479..99e1d92350 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 @@ -5,6 +5,7 @@ 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.google.errorprone.matchers.Matchers.staticMethod; import static java.util.stream.Collectors.joining; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; @@ -66,7 +67,12 @@ public final class ErrorProneTestHelperSourceFormat extends BugChecker .named("addSourceLines"), instanceMethod() .onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper") - .named("addInputLines")); + .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. + staticMethod() + .onClass("tech.picnic.errorprone.documentation.Compilation") + .named("compileWithDocumentationGenerator")); private static final Matcher OUTPUT_SOURCE_ACCEPTING_METHOD = instanceMethod() .onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper.ExpectOutput") @@ -83,7 +89,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } List sourceLines = - tree.getArguments().subList(1, tree.getArguments().size()); + tree.getArguments() + .subList(ASTHelpers.getSymbol(tree).params().size() - 1, tree.getArguments().size()); if (sourceLines.isEmpty()) { return buildDescription(tree).setMessage("No source code provided").build(); } @@ -149,12 +156,13 @@ 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) { - Object value = ASTHelpers.constValue(sourceLine); + String value = ASTHelpers.constValue(sourceLine, String.class); if (value == null) { return Optional.empty(); }