From ba3c02e461f3df902a3f3b180ae23860af50fafc Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Tue, 3 Apr 2018 17:35:57 +0000 Subject: [PATCH] [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 --- clang-tools-extra/clangd/Diagnostics.cpp | 28 ++++++++++++++++++---- .../unittests/clangd/ClangdUnitTests.cpp | 25 +++++++------------ 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index e497823..a52891b 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -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,7 +312,27 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, } llvm::SmallString<64> Message; - Info.FormatDiagnostic(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. diff --git a/clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp b/clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp index 0e11082..655aa6c 100644 --- a/clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp +++ b/clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp @@ -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 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", - "use of undeclared identifier 'goo'; did you mean '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"}), -- 2.7.4