[clangd] synthesize fix message when the diagnostic doesn't provide one.

[sammccall]

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'

[ilya-biryukov]

LGTM with a small nit

clangd/Diagnostics.cpp

322 ↗

(On Diff #140357)

I don't really see a way out of it, but having a limit is a bit frustrating.

It seems weird to have change 'foo' to 'bar', but couldn't find 'fsdfsdf'. did you mean 'verylongidentifierthatsumsuptomorethan200'? for the same error.
Maybe if the message that clang provides is also very long, use the generated message anyway?

[sammccall]

clangd/Diagnostics.cpp

322 ↗

(On Diff #140357)

Hard to know exactly what to do here without motivating cases. I just removed it.

My reasoning: these diagnostics get printed by clang, if they need a shortened message then the diagnostic should/will provide one as note text.

(An alternative that I tried out was replacing the middle of the text with "..." if it was too long, this worked well but probably isn't worth the extra lines of code)

clang-tools-extra/trunk/clangd/Diagnostics.cpp

auto FillDiagBase = [&](DiagBase &D) {
     Info.FormatDiagnostic(Message);
     D.Message = Message.str();
     D.InsideMainFile = InsideMainFile;
     D.File = Info.getSourceManager().getFilename(Info.getLocation());
     D.Severity = DiagLevel;
     return D;
   };
 
-  auto AddFix = [&]() -> bool {
+  auto AddFix = [&](bool SyntheticMessage) -> bool {
     assert(!Info.getFixItHints().empty() &&
            "diagnostic does not have attached fix-its");
     if (!InsideMainFile)
       return false;
 
     llvm::SmallVector<TextEdit, 1> Edits;
     for (auto &FixIt : Info.getFixItHints()) {
       if (!isInsideMainFile(FixIt.RemoveRange.getBegin(),
                             Info.getSourceManager()))
         return false;
       Edits.push_back(toTextEdit(FixIt, Info.getSourceManager(), *LangOpts));
     }
 
     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;
   };
 
   if (!isNote(DiagLevel)) {
     // Handle the new main diagnostic.
     flushLastDiag();
 
     LastDiag = Diag();
     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) {
       assert(false && "Adding a note without main diagnostic");
       IgnoreDiagnostics::log(DiagLevel, Info);
       return;
     }
 
     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.
       Note N;
       FillDiagBase(N);
 
       LastDiag->Notes.push_back(std::move(N));
     }

clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp

 // Helper function to make tests shorter.
 Position pos(int line, int character) {
   Position Res;
   Res.line = line;
   Res.character = 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(
     void $decl[[foo]]();
     int main() {
       $typo[[go\
 o]]();
       foo()$semicolon[[]]
       $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'")));
 }
 
 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"}),
       ElementsAre(AllOf(
           Diag(Test.range(), "return type of 'main' is not 'int'"),
           WithFix(Fix(Test.range(), "int", "change return type to 'int'")))));
 }