Skip to content

Commit

Permalink
Simplify ExpectedFact (#102)
Browse files Browse the repository at this point in the history 
Simplify: replace `ExpectedFact` with a string, and rename
`ExpectedFactAssertion` to `ExpectedFact`.
  • Loading branch information
@netdpb
netdpb authored Aug 23, 2023
1 parent 5512cce commit 9e6590f
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 107 deletions.
7 changes: 4 additions & 3 deletions src/test/java/tests/ConformanceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -119,7 +120,7 @@ protected boolean mustBeExpected() {
}

@Override
protected @Nullable ExpectedFact expectedFact() {
protected @Nullable String expectedFact() {
if (NULLNESS_MISMATCH_KEYS.contains(detailMessage.messageKey)) {
ImmutableList<String> reversedArguments = detailMessage.messageArguments.reverse();
String sourceType = fixType(reversedArguments.get(1)); // penultimate
Expand Down
134 changes: 44 additions & 90 deletions src/test/java/tests/conformance/AbstractConformanceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -117,28 +118,27 @@ private Stream<ConformanceTestReport> analyzeFiles(List<Path> files) {
*/
protected abstract Iterable<ReportedFact> analyze(ImmutableList<Path> files);

/** Reads {@link ExpectedFactAssertion}s from comments in a file. */
private ImmutableList<ExpectedFactAssertion> readExpectedFacts(Path file) {
/** Reads {@link ExpectedFact}s from comments in a file. */
private ImmutableList<ExpectedFact> readExpectedFacts(Path file) {
try {
ImmutableList.Builder<ExpectedFactAssertion> expectedFactAssertions = ImmutableList.builder();
List<ExpectedFact> expectedFacts = new ArrayList<>();
ImmutableList.Builder<ExpectedFact> expectedFacts = ImmutableList.builder();
List<String> facts = new ArrayList<>();
for (ListIterator<String> 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);
}
Expand Down Expand Up @@ -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.*");

Expand All @@ -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;
}

Expand All @@ -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);
Expand All @@ -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
Expand Down
29 changes: 15 additions & 14 deletions src/test/java/tests/conformance/ConformanceTestReport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -44,18 +44,19 @@ public final class ConformanceTestReport {
static ConformanceTestReport forFile(
Path file,
ImmutableList<ReportedFact> reportedFacts,
ImmutableList<ExpectedFactAssertion> expectedFacts) {
ImmutableList<ExpectedFact> expectedFacts) {
ImmutableListMultimap<Long, ReportedFact> reportedFactsByLine =
index(reportedFacts, ReportedFact::getLineNumber);
ImmutableListMultimap<ExpectedFactAssertion, ReportedFact> matchingFacts =
ImmutableListMultimap<ExpectedFact, ReportedFact> 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);
}
Expand All @@ -71,18 +72,18 @@ static ConformanceTestReport combine(ConformanceTestReport left, ConformanceTest
}

private final ImmutableSortedSet<Path> files;
private final ImmutableListMultimap<Path, ExpectedFactAssertion> expectedFactsByFile;
private final ImmutableListMultimap<Path, ExpectedFact> expectedFactsByFile;
private final ImmutableListMultimap<Path, ReportedFact> reportedFactsByFile;
private final ImmutableListMultimap<ExpectedFactAssertion, ReportedFact> matchingFacts;
private final ImmutableListMultimap<ExpectedFact, ReportedFact> matchingFacts;

private ConformanceTestReport(
Collection<Path> files, // Path unfortunately implements Iterable<Path>.
Iterable<ExpectedFactAssertion> expectedFacts,
Iterable<ExpectedFact> expectedFacts,
Iterable<ReportedFact> reportedFacts,
Multimap<ExpectedFactAssertion, ReportedFact> matchingFacts) {
Multimap<ExpectedFact, ReportedFact> 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);
}

Expand All @@ -103,15 +104,15 @@ 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<Long, ExpectedFactAssertion> expectedFactsInFile =
ImmutableListMultimap<Long, ExpectedFact> expectedFactsInFile =
index(expectedFactsByFile.get(file), Fact::getLineNumber);
ImmutableListMultimap<Long, ReportedFact> reportedFactsInFile =
index(reportedFactsByFile.get(file), Fact::getLineNumber);
for (long lineNumber :
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) {
Expand Down Expand Up @@ -146,7 +147,7 @@ private boolean hasUnexpectedFacts(Collection<ReportedFact> reportedFacts) {
return reportedFacts.stream().anyMatch(rf -> rf.mustBeExpected() && isUnexpected(rf));
}

private boolean matchesReportedFact(ExpectedFactAssertion expectedFact) {
private boolean matchesReportedFact(ExpectedFact expectedFact) {
return matchingFacts.containsKey(expectedFact);
}

Expand Down

0 comments on commit 9e6590f

Please sign in to comment.