Skip to content

[Diagnostics] Improve {{none}} fix-it verifier #30791

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

Merged
merged 12 commits into from
Apr 9, 2020
Merged

[Diagnostics] Improve {{none}} fix-it verifier #30791

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
94bb16a
[Diagnostics] Improve {{none}} fix-it verifier
omochi Apr 3, 2020
1a434a4
split two conditions
omochi Apr 4, 2020
ca42e69
define "none" constant
omochi Apr 4, 2020
87edf8e
support plural
omochi Apr 4, 2020
1971f4a
use Twine and add comment for replacement range
omochi Apr 6, 2020
08d332f
check if {{none}} is at the end
omochi Apr 6, 2020
45c9034
use noneMarkerStartLoc
omochi Apr 6, 2020
11a20c9
update second {{none}} error message
omochi Apr 8, 2020
34d52bb
update test case for second {{none}}
omochi Apr 8, 2020
e8cc1f1
fix test case for new {{none}} check
omochi Apr 8, 2020
eed176a
Use named struct
omochi Apr 9, 2020
9f59f11
set const
omochi Apr 9, 2020
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
162 changes: 126 additions & 36 deletions lib/Frontend/DiagnosticVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ struct ExpectedFixIt {
} // end namespace swift

namespace {

static constexpr StringLiteral fixitExpectationNoneString("none");

struct ExpectedDiagnosticInfo {
// This specifies the full range of the "expected-foo {{}}" specifier.
const char *ExpectedStart, *ExpectedEnd = nullptr;
Expand All @@ -46,7 +49,7 @@ struct ExpectedDiagnosticInfo {

// This is true if a '{{none}}' is present to mark that there should be no
// extra fixits.
bool noExtraFixitsMayAppear = false;
bool noExtraFixitsMayAppear() const { return noneMarkerStartLoc != nullptr; };

// This is the raw input buffer for the message text, the part in the
// {{...}}
Expand All @@ -59,6 +62,9 @@ struct ExpectedDiagnosticInfo {

std::vector<ExpectedFixIt> Fixits;

// Loc of {{none}}
const char *noneMarkerStartLoc = nullptr;

ExpectedDiagnosticInfo(const char *ExpectedStart,
DiagnosticKind Classification)
: ExpectedStart(ExpectedStart), Classification(Classification) {}
Expand Down Expand Up @@ -288,16 +294,15 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
unsigned PrevExpectedContinuationLine = 0;

std::vector<ExpectedDiagnosticInfo> ExpectedDiagnostics;
auto addError = [&](const char *Loc, std::string message,

auto addError = [&](const char *Loc, const Twine &message,
ArrayRef<llvm::SMFixIt> FixIts = {}) {
auto loc = SourceLoc(SMLoc::getFromPointer(Loc));
auto diag = SM.GetMessage(loc, llvm::SourceMgr::DK_Error, message,
{}, FixIts);
Errors.push_back(diag);
};



// Scan the memory buffer looking for expected-note/warning/error.
for (size_t Match = InputFile.find("expected-");
Match != StringRef::npos; Match = InputFile.find("expected-", Match+1)) {
Expand Down Expand Up @@ -457,11 +462,25 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {
ExtraChecks = ExtraChecks.substr(EndLoc+2).ltrim();

// Special case for specifying no fixits should appear.
if (FixItStr == "none") {
Expected.noExtraFixitsMayAppear = true;
if (FixItStr == fixitExpectationNoneString) {
if (Expected.noneMarkerStartLoc) {
addError(FixItStr.data() - 2,
Twine("A second {{") + fixitExpectationNoneString +
"}} was found. It may only appear once in an expectation.");
break;
}

Expected.noneMarkerStartLoc = FixItStr.data() - 2;
continue;
}


if (Expected.noneMarkerStartLoc) {
addError(Expected.noneMarkerStartLoc, Twine("{{") +
fixitExpectationNoneString +
"}} must be at the end.");
break;
}

// Parse the pieces of the fix-it.
size_t MinusLoc = FixItStr.find('-');
if (MinusLoc == StringRef::npos) {
Expand Down Expand Up @@ -547,46 +566,117 @@ DiagnosticVerifier::Result DiagnosticVerifier::verifyFile(unsigned BufferID) {

auto &FoundDiagnostic = *FoundDiagnosticIter;

const char *IncorrectFixit = nullptr;
const char *missedFixitLoc = nullptr;
// Verify that any expected fix-its are present in the diagnostic.
for (auto fixit : expected.Fixits) {
// If we found it, we're ok.
if (!checkForFixIt(fixit, FoundDiagnostic, InputFile))
IncorrectFixit = fixit.StartLoc;
if (!checkForFixIt(fixit, FoundDiagnostic, InputFile)) {
missedFixitLoc = fixit.StartLoc;
break;
}
}

bool matchedAllFixIts =
expected.Fixits.size() == FoundDiagnostic.FixIts.size();
bool isUnexpectedFixitsSeen =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: const

expected.Fixits.size() < FoundDiagnostic.FixIts.size();

// it returns actual fix-its string and diagnostic phrase
auto makeActualFixitsPhrase =
[&](ArrayRef<DiagnosticInfo::FixIt> actualFixits)
-> std::tuple<std::string, std::string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

std::tuple is not quite the vocabulary type you want to use here. Please use a real struct with named fields so we can avoid all of that tuple unpacking noise below and get some decently self-documenting code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it 🙂

std::string actualFixitsStr = renderFixits(actualFixits, InputFile);

auto phrase = Twine("actual fix-it") +
(actualFixits.size() >= 2 ? "s" : "") +
" seen: " + actualFixitsStr;
return std::make_tuple(actualFixitsStr, phrase.str());
};

auto emitFixItsError = [&](const char *location, const Twine &message,
const char *replStartLoc, const char *replEndLoc,
const std::string &replStr) {
llvm::SMFixIt fix(llvm::SMRange(SMLoc::getFromPointer(replStartLoc),
SMLoc::getFromPointer(replEndLoc)),
replStr);
addError(location, message, fix);
};

// If we have any expected fixits that didn't get matched, then they are
// wrong. Replace the failed fixit with what actually happened.
if (IncorrectFixit) {

if (missedFixitLoc) {
// If we had an incorrect expected fixit, render it and produce a fixit
// of our own.

assert(!expected.Fixits.empty() &&
"some fix-its should be expected here");

const char *replStartLoc = expected.Fixits.front().StartLoc;
const char *replEndLoc = expected.Fixits.back().EndLoc;

std::string message = "expected fix-it not seen";
std::string actualFixits;

if (FoundDiagnostic.FixIts.empty()) {
addError(IncorrectFixit, "expected fix-it not seen");
/// If actual fix-its is empty,
/// eat a space before first marker.
/// For example,
///
/// @code
/// expected-error {{message}} {{1-2=aa}}
/// ~~~~~~~~~~~
/// ^ remove
/// @endcode
if (replStartLoc[-1] == ' ') {
replStartLoc--;
}
Comment on lines +633 to +635
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: a comment explaining why this is needed might be nice for future readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added comment.
https://github.com/apple/swift/blob/efaacddecf0b309215ff8e41b24d73558c64d6b8/lib/Frontend/DiagnosticVerifier.cpp#L604-L615

I also added here.
https://github.com/apple/swift/blob/efaacddecf0b309215ff8e41b24d73558c64d6b8/lib/Frontend/DiagnosticVerifier.cpp#L650-L661

} else {
// If we had an incorrect expected fixit, render it and produce a fixit
// of our own.
auto actual = renderFixits(FoundDiagnostic.FixIts, InputFile);
auto replStartLoc = SMLoc::getFromPointer(expected.Fixits[0].StartLoc);
auto replEndLoc = SMLoc::getFromPointer(expected.Fixits.back().EndLoc);

llvm::SMFixIt fix(llvm::SMRange(replStartLoc, replEndLoc), actual);
addError(IncorrectFixit,
"expected fix-it not seen; actual fix-its: " + actual, fix);
auto actualAndPhrase = makeActualFixitsPhrase(FoundDiagnostic.FixIts);
actualFixits = std::get<0>(actualAndPhrase);
message += "; " + std::get<1>(actualAndPhrase);
}
} else if (expected.noExtraFixitsMayAppear &&
!matchedAllFixIts &&
!expected.mayAppear) {
// If there was no fixit specification, but some were produced, add a
// fixit to add them in.
auto actual = renderFixits(FoundDiagnostic.FixIts, InputFile);
auto replStartLoc = SMLoc::getFromPointer(expected.ExpectedEnd - 8); // {{none}} length
auto replEndLoc = SMLoc::getFromPointer(expected.ExpectedEnd);

llvm::SMFixIt fix(llvm::SMRange(replStartLoc, replEndLoc), actual);
addError(replStartLoc.getPointer(), "expected no fix-its; actual fix-it seen: " + actual, fix);

emitFixItsError(missedFixitLoc, message, replStartLoc, replEndLoc,
actualFixits);
} else if (expected.noExtraFixitsMayAppear() && isUnexpectedFixitsSeen) {
// If unexpected fixit were produced, add a fixit to add them in.

assert(!FoundDiagnostic.FixIts.empty() &&
"some fix-its should be produced here");
assert(expected.noneMarkerStartLoc && "none marker location is null");

const char *replStartLoc = nullptr, *replEndLoc = nullptr;
std::string message;
if (expected.Fixits.empty()) {
message = "expected no fix-its";
replStartLoc = expected.noneMarkerStartLoc;
replEndLoc = expected.noneMarkerStartLoc;
} else {
message = "unexpected fix-it seen";
replStartLoc = expected.Fixits.front().StartLoc;
replEndLoc = expected.Fixits.back().EndLoc;
}

auto actualAndPhrase = makeActualFixitsPhrase(FoundDiagnostic.FixIts);
std::string actualFixits = std::get<0>(actualAndPhrase);
message += "; " + std::get<1>(actualAndPhrase);

if (replStartLoc == replEndLoc) {
/// If no fix-its was expected and range of replacement is empty,
/// insert space after new last marker.
/// For example:
///
/// @code
/// expected-error {{message}} {{none}}
/// ^
/// insert `{{1-2=aa}} `
/// @endcode
actualFixits += " ";
}

emitFixItsError(expected.noneMarkerStartLoc, message, replStartLoc,
replEndLoc, actualFixits);
}

// Actually remove the diagnostic from the list, so we don't match it
// again. We do have to do this after checking fix-its, though, because
// the diagnostic owns its fix-its.
Expand Down
60 changes: 59 additions & 1 deletion test/FixCode/verify-fixits.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,62 @@ func f1() {
guard true { return } // expected-error {{...}}

guard true { return } // expected-error {{expected 'else' after 'guard' condition}} {{none}}
}
}

func labeledFunc(aa: Int, bb: Int) {}

func test0Fixits() {
undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}} {{1-1=a}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}} {{1-1=a}} {{2-2=b}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}} {{none}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}} {{1-1=a}} {{none}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}} {{1-1=a}} {{2-2=b}} {{none}}
}

func test1Fixits() {
labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=xx}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}} {{15-18=xx}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=xx}} {{15-18=aa}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{none}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}} {{none}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=xx}} {{none}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}} {{15-18=xx}} {{none}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=xx}} {{15-18=aa}} {{none}}
}

func test2Fixits() {
labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=xx}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=bb}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=xx}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{none}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{none}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=bb}} {{none}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=xx}} {{none}}
}
62 changes: 60 additions & 2 deletions test/FixCode/verify-fixits.swift.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,63 @@
func f1() {
guard true { return } // expected-error {{expected 'else' after 'guard' condition}}

guard true { return } // expected-error {{expected 'else' after 'guard' condition}} {{14-14=else }}
}
guard true { return } // expected-error {{expected 'else' after 'guard' condition}} {{14-14=else }} {{none}}
}

func labeledFunc(aa: Int, bb: Int) {}

func test0Fixits() {
undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}} {{none}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}} {{none}}

undefinedFunc() // expected-error {{use of unresolved identifier 'undefinedFunc'}} {{none}}
}

func test1Fixits() {
labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}} {{none}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}} {{none}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}} {{none}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}} {{none}}

labeledFunc(aax: 0, bb: 1) // expected-error {{incorrect argument label in call (have 'aax:bb:', expected 'aa:bb:')}} {{15-18=aa}} {{none}}
}

func test2Fixits() {
labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=bb}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=bb}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=bb}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=bb}} {{none}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=bb}} {{none}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=bb}} {{none}}

labeledFunc(aax: 0, bbx: 1) // expected-error {{incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')}} {{15-18=aa}} {{23-26=bb}} {{none}}
}
Loading