Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use DetailedTestDiagnostic from EISOP instead of DetailMessage #186

Merged
merged 3 commits into from
Jul 25, 2024
Merged

Use DetailedTestDiagnostic from EISOP instead of DetailMessage #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ffe4a0e
Use DetailedTestDiagnostic from EISOP instead of DetailMessage
wmdietl Jul 10, 2024
9833733
No need to use `-Adetailedmsgtext` in tests
wmdietl Jul 14, 2024
1aadcab
Fix formatting
wmdietl Jul 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 32 additions & 25 deletions src/test/java/tests/ConformanceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Stream;
Expand All @@ -37,7 +38,9 @@
import org.checkerframework.framework.test.TestUtilities;
import org.checkerframework.framework.test.TypecheckExecutor;
import org.checkerframework.framework.test.TypecheckResult;
import org.checkerframework.framework.test.diagnostics.DetailedTestDiagnostic;
import org.checkerframework.framework.test.diagnostics.DiagnosticKind;
import org.checkerframework.framework.test.diagnostics.TestDiagnostic;
import org.jspecify.annotations.Nullable;
import org.jspecify.conformance.ConformanceTestRunner;
import org.jspecify.conformance.ExpectedFact;
Expand Down Expand Up @@ -141,10 +144,7 @@ private static ImmutableSet<ReportedFact> analyze(
TestUtilities.getShouldEmitDebugInfo());
TypecheckResult result = new TypecheckExecutor().runTest(config);
return result.getUnexpectedDiagnostics().stream()
.map(d -> DetailMessage.parse(d, testDirectory))
// Do not filter out messages without details.
// .filter(DetailMessage::hasDetails)
.map(DetailMessageReportedFact::new)
.map(d -> new DetailMessageReportedFact(testDirectory, d))
.collect(toImmutableSet());
}

Expand Down Expand Up @@ -177,60 +177,67 @@ static final class DetailMessageReportedFact extends ReportedFact {
"type.parameter.annotated",
"wildcard.annotated");

private final DetailMessage detailMessage;
private final TestDiagnostic diagnostic;

DetailMessageReportedFact(DetailMessage detailMessage) {
super(detailMessage.getFile(), detailMessage.getLineNumber());
this.detailMessage = detailMessage;
DetailMessageReportedFact(@Nullable Path testDirectory, TestDiagnostic diag) {
super(
(testDirectory != null && diag.getFile().startsWith(testDirectory))
? testDirectory.relativize(diag.getFile())
: diag.getFile(),
diag.getLineNumber());
this.diagnostic = diag;
}

@Override
protected boolean matches(ExpectedFact expectedFact) {
if (expectedFact.isNullnessMismatch()) {
return DEREFERENCE.equals(detailMessage.messageKey)
|| CANNOT_CONVERT_KEYS.contains(detailMessage.messageKey);
return DEREFERENCE.equals(diagnostic.getMessageKey())
|| CANNOT_CONVERT_KEYS.contains(diagnostic.getMessageKey());
}
return super.matches(expectedFact);
}

@Override
protected boolean mustBeExpected() {
return detailMessage.getKind().equals(DiagnosticKind.Error);
return diagnostic.getKind().equals(DiagnosticKind.Error);
}

@Override
protected String getFactText() {
if (CANNOT_CONVERT_KEYS.contains(detailMessage.messageKey)) {
if (detailMessage.messageArguments.size() < 2) {
if (!(diagnostic instanceof DetailedTestDiagnostic)) {
return toString();
}
List<String> args = ((DetailedTestDiagnostic) diagnostic).getAdditionalTokens();
if (CANNOT_CONVERT_KEYS.contains(diagnostic.getMessageKey())) {
if (args.size() < 2) {
// The arguments must end with sourceType and sinkType.
return toString();
}
ImmutableList<String> reversedArguments = detailMessage.messageArguments.reverse();
String sourceType = fixType(reversedArguments.get(1)); // penultimate
String sinkType = fixType(reversedArguments.get(0)); // last
String sourceType = fixType(args.get(args.size() - 2)); // penultimate
String sinkType = fixType(args.get(args.size() - 1)); // last
return cannotConvert(sourceType, sinkType);
}
if (IRRELEVANT_ANNOTATION_KEYS.contains(detailMessage.messageKey)) {
if (detailMessage.messageArguments.isEmpty()) {
if (IRRELEVANT_ANNOTATION_KEYS.contains(diagnostic.getMessageKey())) {
if (args.isEmpty()) {
// arguments must start with the annotation
return toString();
}
return irrelevantAnnotation(
// Remove the package name (and any enclosing element name); emit just the simple name.
detailMessage.messageArguments.get(0).replaceFirst(".*\\.", ""));
args.get(0).replaceFirst(".*\\.", ""));
}
switch (detailMessage.messageKey) {
switch (diagnostic.getMessageKey()) {
case "sourceType":
{
String expressionType = fixType(detailMessage.messageArguments.get(0));
String expression = detailMessage.messageArguments.get(1);
String expressionType = fixType(args.get(0));
String expression = args.get(1);
return expressionType(expressionType, expression);
}
case "sinkType":
{
String sinkType = fixType(detailMessage.messageArguments.get(0));
String sinkType = fixType(args.get(0));
// Remove the simple name of the class and the dot before the method name.
String sink = detailMessage.messageArguments.get(1).replaceFirst("^[^.]+\\.", "");
String sink = args.get(1).replaceFirst("^[^.]+\\.", "");
return sinkType(sinkType, sink);
}
}
Expand All @@ -239,7 +246,7 @@ protected String getFactText() {

@Override
public String toString() {
return String.format("(%s) %s", detailMessage.messageKey, detailMessage.readableMessage);
return String.format("(%s) %s", diagnostic.getMessageKey(), diagnostic.getMessage());
}

/**
Expand Down
178 changes: 0 additions & 178 deletions src/test/java/tests/DetailMessage.java
View file
Open in desktop

This file was deleted.

27 changes: 8 additions & 19 deletions src/test/java/tests/NullSpecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.ListIterator;
import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest;
import org.checkerframework.framework.test.TypecheckResult;
import org.checkerframework.framework.test.diagnostics.DetailedTestDiagnostic;
import org.checkerframework.framework.test.diagnostics.DiagnosticKind;
import org.checkerframework.framework.test.diagnostics.TestDiagnostic;
import org.checkerframework.javacutil.BugInCF;
Expand Down Expand Up @@ -90,8 +91,7 @@ private static String[] getSamplesDirs() {

private static String[] checkerOptions(boolean strict) {
ImmutableList.Builder<String> options = ImmutableList.builder();
options.add(
"-AassumePure", "-Adetailedmsgtext", "-AcheckImpl", "-AsuppressWarnings=conditional");
options.add("-AassumePure", "-AcheckImpl", "-AsuppressWarnings=conditional");
if (strict) {
options.add("-Astrict");
}
Expand All @@ -117,10 +117,8 @@ public TypecheckResult adjustTypecheckResult(TypecheckResult testResult) {

for (ListIterator<TestDiagnostic> i = unexpected.listIterator(); i.hasNext(); ) {
TestDiagnostic diagnostic = i.next();
DetailMessage detailMessage = DetailMessage.parse(diagnostic, null);
if (detailMessage.hasDetails()) {
// Replace diagnostics that can be parsed with DetailMessage diagnostics.
i.set(detailMessage);
if (diagnostic instanceof DetailedTestDiagnostic) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the normal tests are executed without -Adetailedmsgtext, this is likely a dead branch. I'll address this in a separate PR.

// Keep all detailed test diagnostics.
} else if (diagnostic.getKind() != DiagnosticKind.Error) {
// Remove warnings like explicit.annotation.ignored and deprecation.
i.remove();
Expand All @@ -147,15 +145,6 @@ public TypecheckResult adjustTypecheckResult(TypecheckResult testResult) {
* unexpected}, a reported diagnostic.
*/
private boolean corresponds(TestDiagnostic missing, TestDiagnostic unexpected) {
return unexpected instanceof DetailMessage
&& corresponds(missing, ((DetailMessage) unexpected));
}

/**
* Returns {@code true} if {@code missing} is a JSpecify directive that matches {@code
* unexpected}, a reported diagnostic.
*/
private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) {
// First, make sure the two diagnostics are on the same file and line.
if (!missing.getFilename().equals(unexpected.getFilename())
|| missing.getLineNumber() != unexpected.getLineNumber()) {
Expand All @@ -167,7 +156,7 @@ private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) {
|| missing.getMessage().contains("jspecify_nullness_not_enough_information")
|| missing.getMessage().contains("jspecify_nullness_mismatch")
|| missing.getMessage().contains("test:cannot-convert")) {
switch (unexpected.messageKey) {
switch (unexpected.getMessageKey()) {
case "argument.type.incompatible":
case "assignment.type.incompatible":
case "atomicreference.must.include.null":
Expand All @@ -190,7 +179,7 @@ private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) {

switch (missing.getMessage()) {
case "jspecify_nullness_intrinsically_not_nullable":
switch (unexpected.messageKey) {
switch (unexpected.getMessageKey()) {
case "enum.constant.annotated":
case "outer.annotated":
case "primitive.annotated":
Expand All @@ -199,7 +188,7 @@ private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) {
return false;
}
case "jspecify_unrecognized_location":
switch (unexpected.messageKey) {
switch (unexpected.getMessageKey()) {
/*
* We'd rather avoid this `bound` error (in part because it suggests that the annotation
* is having some effect, which we don't want!), but the most important thing is that the
Expand All @@ -217,7 +206,7 @@ private boolean corresponds(TestDiagnostic missing, DetailMessage unexpected) {
return false;
}
case "jspecify_conflicting_annotations":
switch (unexpected.messageKey) {
switch (unexpected.getMessageKey()) {
case "type.invalid.conflicting.annos":
case "type.invalid.super.wildcard":
return true;
Expand Down
Loading