diff --git a/src/test/java/tests/ConformanceTest.java b/src/test/java/tests/ConformanceTest.java index 6bbd0ab..9c39acf 100644 --- a/src/test/java/tests/ConformanceTest.java +++ b/src/test/java/tests/ConformanceTest.java @@ -22,6 +22,7 @@ import static tests.conformance.AbstractConformanceTest.ExpectedFact.cannotConvert; import static tests.conformance.AbstractConformanceTest.ExpectedFact.expressionType; import static tests.conformance.AbstractConformanceTest.ExpectedFact.irrelevantAnnotation; +import static tests.conformance.AbstractConformanceTest.ExpectedFact.isNullnessMismatch; import static tests.conformance.AbstractConformanceTest.ExpectedFact.sinkType; import com.google.common.base.Splitter; @@ -106,8 +107,8 @@ static final class DetailMessageReportedFact extends ReportedFact { } @Override - protected boolean matches(ExpectedFact expectedFact) { - if (expectedFact.isNullnessMismatch()) { + protected boolean matches(String expectedFact) { + if (isNullnessMismatch(expectedFact)) { return NULLNESS_MISMATCH_KEYS.contains(detailMessage.messageKey); } return super.matches(expectedFact); @@ -119,7 +120,7 @@ protected boolean mustBeExpected() { } @Override - protected @Nullable ExpectedFact expectedFact() { + protected @Nullable String expectedFact() { if (NULLNESS_MISMATCH_KEYS.contains(detailMessage.messageKey)) { ImmutableList reversedArguments = detailMessage.messageArguments.reverse(); String sourceType = fixType(reversedArguments.get(1)); // penultimate diff --git a/src/test/java/tests/conformance/AbstractConformanceTest.java b/src/test/java/tests/conformance/AbstractConformanceTest.java index 3325648..f980e69 100644 --- a/src/test/java/tests/conformance/AbstractConformanceTest.java +++ b/src/test/java/tests/conformance/AbstractConformanceTest.java @@ -11,6 +11,7 @@ import static java.nio.file.Files.walk; import static java.util.Arrays.stream; import static java.util.Objects.requireNonNull; +import static java.util.Objects.requireNonNullElse; import static java.util.stream.Collectors.toList; import static tests.conformance.AbstractConformanceTest.ExpectedFact.readExpectedFact; @@ -117,28 +118,27 @@ private Stream analyzeFiles(List files) { */ protected abstract Iterable analyze(ImmutableList files); - /** Reads {@link ExpectedFactAssertion}s from comments in a file. */ - private ImmutableList readExpectedFacts(Path file) { + /** Reads {@link ExpectedFact}s from comments in a file. */ + private ImmutableList readExpectedFacts(Path file) { try { - ImmutableList.Builder expectedFactAssertions = ImmutableList.builder(); - List expectedFacts = new ArrayList<>(); + ImmutableList.Builder expectedFacts = ImmutableList.builder(); + List facts = new ArrayList<>(); for (ListIterator i = readAllLines(testDirectory.resolve(file), UTF_8).listIterator(); i.hasNext(); ) { String line = i.next(); Matcher matcher = EXPECTATION_COMMENT.matcher(line); - ExpectedFact fact = - matcher.matches() ? readExpectedFact(matcher.group("expectation")) : null; + String fact = matcher.matches() ? readExpectedFact(matcher.group("expectation")) : null; if (fact != null) { - expectedFacts.add(fact); + facts.add(fact); } else { long lineNumber = i.nextIndex(); - expectedFacts.stream() - .map(expectedFact -> new ExpectedFactAssertion(file, lineNumber, expectedFact)) - .forEach(expectedFactAssertions::add); - expectedFacts.clear(); + facts.stream() + .map(expectedFact -> new ExpectedFact(file, lineNumber, expectedFact)) + .forEach(expectedFacts::add); + facts.clear(); } } - return expectedFactAssertions.build(); + return expectedFacts.build(); } catch (IOException e) { throw new UncheckedIOException(e); } @@ -245,13 +245,12 @@ public final long getLineNumber() { } /** - * An assertion that the tool behaves in a way consistent with a specific fact. Some of these - * facts indicate that according to the JSpecify specification, the code in question may have an - * error that should be reported to users; other expected facts are informational, such as the - * expected nullness-augmented type of an expression. + * An assertion that the tool behaves in a way consistent with a specific fact about a line in the + * source code. Some of these facts indicate that according to the JSpecify specification, the + * code in question may have an error that should be reported to users; other expected facts are + * informational, such as the expected nullness-augmented type of an expression. */ - // TODO(dpb): Maybe just use the string instead of this class? - public static final class ExpectedFact { + public static final class ExpectedFact extends Fact { private static final Pattern NULLNESS_MISMATCH = Pattern.compile("jspecify_nullness_mismatch\\b.*"); @@ -268,93 +267,49 @@ public static final class ExpectedFact { * Returns an expected fact representing that the source type cannot be converted to the sink * type in any world. */ - public static ExpectedFact cannotConvert(String sourceType, String sinkType) { - return new ExpectedFact(String.format("test:cannot-convert:%s to %s", sourceType, sinkType)); + public static String cannotConvert(String sourceType, String sinkType) { + return String.format("test:cannot-convert:%s to %s", sourceType, sinkType); } /** Returns an expected fact representing an expected expression type. */ - public static ExpectedFact expressionType(String expressionType, String expression) { - return new ExpectedFact( - String.format("test:expression-type:%s:%s", expressionType, expression)); + public static String expressionType(String expressionType, String expression) { + return String.format("test:expression-type:%s:%s", expressionType, expression); } /** Returns an expected fact representing that an annotation is not relevant. */ - public static ExpectedFact irrelevantAnnotation(String annotationType) { - return new ExpectedFact(String.format("test:irrelevant-annotation:%s", annotationType)); + public static String irrelevantAnnotation(String annotationType) { + return String.format("test:irrelevant-annotation:%s", annotationType); } /** Returns an expected fact representing that an annotation is not relevant. */ - public static ExpectedFact sinkType(String sinkType, String sink) { - return new ExpectedFact(String.format("test:sink-type:%s:%s", sinkType, sink)); - } - - /** Read an {@link ExpectedFact} from a line of either a source file or a report. */ - static @Nullable ExpectedFact readExpectedFact(String text) { - return ASSERTION_PATTERNS.stream().anyMatch(pattern -> pattern.matcher(text).matches()) - ? new ExpectedFact(text) - : null; - } - - private ExpectedFact(String commentText) { - this.commentText = commentText; - } - - private final String commentText; - - /** The comment text representing this expected fact. */ - public String commentText() { - return commentText; - } - - /** Returns {@code true} if this is a legacy {@code jspecify_nullness_mismatch} assertion. */ - public boolean isNullnessMismatch() { - return NULLNESS_MISMATCH.matcher(commentText).matches(); - } - - @Override - public boolean equals(Object obj) { - if (obj == this) { - return true; - } - if (!(obj instanceof ExpectedFact)) { - return false; - } - ExpectedFact that = (ExpectedFact) obj; - return this.commentText.equals(that.commentText); + public static String sinkType(String sinkType, String sink) { + return String.format("test:sink-type:%s:%s", sinkType, sink); } - @Override - public int hashCode() { - return commentText.hashCode(); + /** + * Returns {@code true} if {@code fact} is a legacy {@code jspecify_nullness_mismatch} + * assertion. + */ + public static boolean isNullnessMismatch(String fact) { + return NULLNESS_MISMATCH.matcher(fact).matches(); } - @Override - public String toString() { - return commentText; + /** Read an expected fact from a line of either a source file or a report. */ + static @Nullable String readExpectedFact(String text) { + return ASSERTION_PATTERNS.stream().anyMatch(pattern -> pattern.matcher(text).matches()) + ? text + : null; } - } - /** - * An assertion that the tool behaves in a way consistent with a specific fact about a line in the - * source code. Some of these facts indicate that according to the JSpecify specification, the - * code in question may have an error that should be reported to users; other expected facts are - * informational, such as the expected nullness-augmented type of an expression. - */ - public static final class ExpectedFactAssertion extends Fact { - private final ExpectedFact fact; + private final String fact; - ExpectedFactAssertion(Path file, long lineNumber, ExpectedFact fact) { + ExpectedFact(Path file, long lineNumber, String fact) { super(file, lineNumber); this.fact = fact; } @Override public String getFactText() { - return fact.commentText(); - } - - /** Returns the fact expected at the file and line number. */ - public ExpectedFact getFact() { return fact; } @@ -363,10 +318,10 @@ public boolean equals(Object o) { if (this == o) { return true; } - if (!(o instanceof ExpectedFactAssertion)) { + if (!(o instanceof ExpectedFact)) { return false; } - ExpectedFactAssertion that = (ExpectedFactAssertion) o; + ExpectedFact that = (ExpectedFact) o; return this.getFile().equals(that.getFile()) && this.getLineNumber() == that.getLineNumber() && this.fact.equals(that.fact); @@ -386,20 +341,19 @@ protected ReportedFact(Path file, long lineNumber) { @Override public String getFactText() { - ExpectedFact expectedFact = expectedFact(); - return expectedFact != null ? expectedFact.commentText() : toString(); + return requireNonNullElse(expectedFact(), toString()); } - /** Returns true if this reported fact must match an {@link ExpectedFactAssertion}. */ + /** Returns true if this reported fact must match an {@link ExpectedFact}. */ protected abstract boolean mustBeExpected(); /** Returns true if this reported fact matches the given expected fact. */ - protected boolean matches(ExpectedFact expectedFact) { + protected boolean matches(String expectedFact) { return expectedFact.equals(expectedFact()); } /** Returns the equivalent expected fact. */ - protected abstract @Nullable ExpectedFact expectedFact(); + protected abstract @Nullable String expectedFact(); /** Returns the message reported, without the file name or line number. */ @Override diff --git a/src/test/java/tests/conformance/ConformanceTestReport.java b/src/test/java/tests/conformance/ConformanceTestReport.java index 94b31ee..2210c8d 100644 --- a/src/test/java/tests/conformance/ConformanceTestReport.java +++ b/src/test/java/tests/conformance/ConformanceTestReport.java @@ -29,7 +29,7 @@ import java.util.Collection; import java.util.Formatter; import tests.ConformanceTest; -import tests.conformance.AbstractConformanceTest.ExpectedFactAssertion; +import tests.conformance.AbstractConformanceTest.ExpectedFact; import tests.conformance.AbstractConformanceTest.Fact; import tests.conformance.AbstractConformanceTest.ReportedFact; @@ -44,18 +44,19 @@ public final class ConformanceTestReport { static ConformanceTestReport forFile( Path file, ImmutableList reportedFacts, - ImmutableList expectedFacts) { + ImmutableList expectedFacts) { ImmutableListMultimap reportedFactsByLine = index(reportedFacts, ReportedFact::getLineNumber); - ImmutableListMultimap matchingFacts = + ImmutableListMultimap matchingFacts = expectedFacts.stream() - .sorted(comparingLong(ExpectedFactAssertion::getLineNumber)) + .sorted(comparingLong(ExpectedFact::getLineNumber)) .collect( flatteningToImmutableListMultimap( f -> f, expectedFact -> reportedFactsByLine.get(expectedFact.getLineNumber()).stream() - .filter(reportedFact -> reportedFact.matches(expectedFact.getFact())))); + .filter( + reportedFact -> reportedFact.matches(expectedFact.getFactText())))); return new ConformanceTestReport( ImmutableSortedSet.of(file), expectedFacts, reportedFacts, matchingFacts); } @@ -71,18 +72,18 @@ static ConformanceTestReport combine(ConformanceTestReport left, ConformanceTest } private final ImmutableSortedSet files; - private final ImmutableListMultimap expectedFactsByFile; + private final ImmutableListMultimap expectedFactsByFile; private final ImmutableListMultimap reportedFactsByFile; - private final ImmutableListMultimap matchingFacts; + private final ImmutableListMultimap matchingFacts; private ConformanceTestReport( Collection files, // Path unfortunately implements Iterable. - Iterable expectedFacts, + Iterable expectedFacts, Iterable reportedFacts, - Multimap matchingFacts) { + Multimap matchingFacts) { this.files = ImmutableSortedSet.copyOf(files); - this.expectedFactsByFile = index(expectedFacts, ExpectedFactAssertion::getFile); - this.reportedFactsByFile = index(reportedFacts, ReportedFact::getFile); + this.expectedFactsByFile = index(expectedFacts, Fact::getFile); + this.reportedFactsByFile = index(reportedFacts, Fact::getFile); this.matchingFacts = ImmutableListMultimap.copyOf(matchingFacts); } @@ -103,7 +104,7 @@ public String report(boolean details) { "# %,d pass; %,d fail; %,d total; %.1f%% score%n", passes, fails, total, 100.0 * passes / total); for (Path file : files) { - ImmutableListMultimap expectedFactsInFile = + ImmutableListMultimap expectedFactsInFile = index(expectedFactsByFile.get(file), Fact::getLineNumber); ImmutableListMultimap reportedFactsInFile = index(reportedFactsByFile.get(file), Fact::getLineNumber); @@ -111,7 +112,7 @@ public String report(boolean details) { ImmutableSortedSet.copyOf( union(expectedFactsInFile.keySet(), reportedFactsInFile.keySet()))) { // Report all expected facts on this line and whether they're reported or not. - for (ExpectedFactAssertion expectedFact : expectedFactsInFile.get(lineNumber)) { + for (ExpectedFact expectedFact : expectedFactsInFile.get(lineNumber)) { writeFact(report, expectedFact, matchesReportedFact(expectedFact) ? "PASS" : "FAIL"); } if (details) { @@ -146,7 +147,7 @@ private boolean hasUnexpectedFacts(Collection reportedFacts) { return reportedFacts.stream().anyMatch(rf -> rf.mustBeExpected() && isUnexpected(rf)); } - private boolean matchesReportedFact(ExpectedFactAssertion expectedFact) { + private boolean matchesReportedFact(ExpectedFact expectedFact) { return matchingFacts.containsKey(expectedFact); }