[clangd] synthesize fix message when the diagnostic doesn't provide one.
Summary: Currently if a fix is attached directly to a diagnostic, we repeat the diagnostic message as the fix message. From eyeballing the top diagnostics, it seems describing the textual replacement would be much clearer. e.g. error: use of undeclared identifier 'goo'; did you mean 'foo'? action before: use of undeclared identifier 'goo'; did you mean 'foo'? action after: change 'goo' to 'foo' Reviewers: ilya-biryukov Subscribers: klimek, jkorous-apple, ioeric, MaskRay, cfe-commits Differential Revision: https://reviews.llvm.org/D45069 llvm-svn: 329090
This commit is contained in:
parent
f7226ed67d
commit
ba3c02e461
|
@ -297,7 +297,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
|
|||
return D;
|
||||
};
|
||||
|
||||
auto AddFix = [&]() -> bool {
|
||||
auto AddFix = [&](bool SyntheticMessage) -> bool {
|
||||
assert(!Info.getFixItHints().empty() &&
|
||||
"diagnostic does not have attached fix-its");
|
||||
if (!InsideMainFile)
|
||||
|
@ -312,6 +312,26 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
|
|||
}
|
||||
|
||||
llvm::SmallString<64> Message;
|
||||
// If requested and possible, create a message like "change 'foo' to 'bar'".
|
||||
if (SyntheticMessage && Info.getNumFixItHints() == 1) {
|
||||
const auto &FixIt = Info.getFixItHint(0);
|
||||
bool Invalid = false;
|
||||
StringRef Remove = Lexer::getSourceText(
|
||||
FixIt.RemoveRange, Info.getSourceManager(), *LangOpts, &Invalid);
|
||||
StringRef Insert = FixIt.CodeToInsert;
|
||||
if (!Invalid) {
|
||||
llvm::raw_svector_ostream M(Message);
|
||||
if (!Remove.empty() && !Insert.empty())
|
||||
M << "change '" << Remove << "' to '" << Insert << "'";
|
||||
else if (!Remove.empty())
|
||||
M << "remove '" << Remove << "'";
|
||||
else if (!Insert.empty())
|
||||
M << "insert '" << Insert << "'";
|
||||
// Don't allow source code to inject newlines into diagnostics.
|
||||
std::replace(Message.begin(), Message.end(), '\n', ' ');
|
||||
}
|
||||
}
|
||||
if (Message.empty()) // either !SytheticMessage, or we failed to make one.
|
||||
Info.FormatDiagnostic(Message);
|
||||
LastDiag->Fixes.push_back(Fix{Message.str(), std::move(Edits)});
|
||||
return true;
|
||||
|
@ -325,7 +345,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
|
|||
FillDiagBase(*LastDiag);
|
||||
|
||||
if (!Info.getFixItHints().empty())
|
||||
AddFix();
|
||||
AddFix(true /* try to invent a message instead of repeating the diag */);
|
||||
} else {
|
||||
// Handle a note to an existing diagnostic.
|
||||
if (!LastDiag) {
|
||||
|
@ -337,7 +357,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
|
|||
if (!Info.getFixItHints().empty()) {
|
||||
// A clang note with fix-it is not a separate diagnostic in clangd. We
|
||||
// attach it as a Fix to the main diagnostic instead.
|
||||
if (!AddFix())
|
||||
if (!AddFix(false /* use the note as the message */))
|
||||
IgnoreDiagnostics::log(DiagLevel, Info);
|
||||
} else {
|
||||
// A clang note without fix-its corresponds to clangd::Note.
|
||||
|
|
|
@ -92,14 +92,6 @@ Position pos(int line, int character) {
|
|||
return Res;
|
||||
}
|
||||
|
||||
/// Matches diagnostic that has exactly one fix with the same range and message
|
||||
/// as the diagnostic itself.
|
||||
testing::Matcher<const clangd::Diag &> DiagWithEqualFix(clangd::Range Range,
|
||||
std::string Replacement,
|
||||
std::string Message) {
|
||||
return AllOf(Diag(Range, Message), WithFix(Fix(Range, Replacement, Message)));
|
||||
}
|
||||
|
||||
TEST(DiagnosticsTest, DiagnosticRanges) {
|
||||
// Check we report correct ranges, including various edge-cases.
|
||||
Annotations Test(R"cpp(
|
||||
|
@ -111,19 +103,19 @@ o]]();
|
|||
$unk[[unknown]]();
|
||||
}
|
||||
)cpp");
|
||||
llvm::errs() << Test.code();
|
||||
EXPECT_THAT(
|
||||
buildDiags(Test.code()),
|
||||
ElementsAre(
|
||||
// This range spans lines.
|
||||
AllOf(DiagWithEqualFix(
|
||||
Test.range("typo"), "foo",
|
||||
AllOf(Diag(Test.range("typo"),
|
||||
"use of undeclared identifier 'goo'; did you mean 'foo'?"),
|
||||
WithFix(
|
||||
Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
|
||||
// This is a pretty normal range.
|
||||
WithNote(Diag(Test.range("decl"), "'foo' declared here"))),
|
||||
// This range is zero-width, and at the end of a line.
|
||||
DiagWithEqualFix(Test.range("semicolon"), ";",
|
||||
"expected ';' after expression"),
|
||||
AllOf(Diag(Test.range("semicolon"), "expected ';' after expression"),
|
||||
WithFix(Fix(Test.range("semicolon"), ";", "insert ';'"))),
|
||||
// This range isn't provided by clang, we expand to the token.
|
||||
Diag(Test.range("unk"), "use of undeclared identifier 'unknown'")));
|
||||
}
|
||||
|
@ -131,8 +123,9 @@ o]]();
|
|||
TEST(DiagnosticsTest, FlagsMatter) {
|
||||
Annotations Test("[[void]] main() {}");
|
||||
EXPECT_THAT(buildDiags(Test.code()),
|
||||
ElementsAre(DiagWithEqualFix(Test.range(), "int",
|
||||
"'main' must return 'int'")));
|
||||
ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"),
|
||||
WithFix(Fix(Test.range(), "int",
|
||||
"change 'void' to 'int'")))));
|
||||
// Same code built as C gets different diagnostics.
|
||||
EXPECT_THAT(
|
||||
buildDiags(Test.code(), {"-x", "c"}),
|
||||
|
|
Loading…
Reference in New Issue