[clangd] capitalize diagnostic messages

[arphaman]

Summary

This patch adds support for diagnostic message capitalization to Clangd. The diagnostic messages are now always capitalized.

[ioeric]

Is it possible for clangd to always capitalize diagnostics if -capitialize-diagnostic-text is found in the compile comand?

[ilya-biryukov]

Maybe we could simply always capitalize the messages? Any cons to capitalizing error messages?
@simark, @malaperle, @ioeric, @hokein, WDYT?

[ioeric]

In D50154#1185545, @ilya-biryukov wrote:

Maybe we could simply always capitalize the messages? Any cons to capitalizing error messages?
@simark, @malaperle, @ioeric, @hokein, WDYT?

Oh sorry, I thought -capitialize-diagnostic-text was an existing clang flag, in which case we should respect it. I don't think there is any con that I could think of, as long as it looks nice in clients :)

[simark]

That's fine with me. I'll try it and time will tell if I like it or not :).

[sammccall]

Capitalizing sounds nice. +1 to just do it without an option.

My favorite essay on this is
http://neugierig.org/software/blog/2018/07/options.html

[simark]

In D50154#1185605, @sammccall wrote:

Capitalizing sounds nice. +1 to just do it without an option.

My favorite essay on this is
http://neugierig.org/software/blog/2018/07/options.html

Agreed.

[ilya-biryukov]

Seems we have consensus here.
Let's remove the option and just always capitalize the messages.

[arphaman]

Always capitalize the diagnostic's message.

[ioeric]

test/clangd/capitalize-diagnostics.test

1 ↗

(On Diff #159036)

This new test doesn't seem to be needed anymore?

[arphaman]

Remove test and update unit test.

test/clangd/capitalize-diagnostics.test

1 ↗

(On Diff #159036)

Right, the note message is exercised by the unit test, which I didn't update. Updated it now.

[ioeric]

lgtm

clangd/Diagnostics.cpp

149

nit: this doesn't have to be diagnostic text, so I'd probably call this capitalize().

[hokein]

nit: The patch description needs to be updated.

[dblaikie]

What's the motivation for clangd to differ from clang here? (& if the first
letter is going to be capitalized, should there be a period at the end? But
also the phrasing of most/all diagnostic text isn't in the form of complete
sentences, so this might not make sense)

[arphaman]

In D50154#1191002, @dblaikie wrote:

What's the motivation for clangd to differ from clang here? (& if the first
letter is going to be capitalized, should there be a period at the end? But
also the phrasing of most/all diagnostic text isn't in the form of complete
sentences, so this might not make sense)

It's mostly for the presentation purposes, to match the needs of our client. I first implemented it as an opt-in feature, but the consensus was to capitalize the messages all the time.
I don't think it would make sense to insert the period at the end, because, as you said, not all diagnostics are complete sentences

[ilya-biryukov]

In D50154#1191002, @dblaikie wrote:

What's the motivation for clangd to differ from clang here?

The presentation of diagnostics to the users is different between clang and clangd:

• clang diagnostics are always prefixed with the file, location and severity of the diagnostic, e.g. main.cpp:1:2: error: expected '}'
• In LSP clients (VSCode in particular), the diagnostic message is the first thing the user sees and it looks a little nicer (subjectively) if the first letter is capitalized.

See the screenshots from VSCode on how diagnostics are presented:

clangd/Diagnostics.cpp

void printDiag(llvm::raw_string_ostream &OS, const DiagBase &D) {
   // line improves readability.
   if (D.InsideMainFile)
     OS << " ";
   else
     OS << "\n";
   OS << diagLeveltoString(D.Severity) << ": " << D.Message;
 }
 
+/// Capitalizes the first word in the diagnostic's message.
+std::string capitalize(std::string Message) {
+  if (!Message.empty())
+    Message[0] = llvm::toUpper(Message[0]);
+  return Message;
+}
+
 /// Returns a message sent to LSP for the main diagnostic in \p D.
 /// The message includes all the notes with their corresponding locations.
 /// However, notes with fix-its are excluded as those usually only contain a
 /// fix-it message and just add noise if included in the message for diagnostic.
 /// Example output:
 ///
 ///     no matching function for call to 'foo'
 ///
 ///     main.cpp:3:5: note: candidate function not viable: requires 2 arguments
 ///
 ///     dir1/dir2/dir3/../../dir4/header.h:12:23
 ///     note: candidate function not viable: requires 3 arguments
 std::string mainMessage(const Diag &D) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   OS << D.Message;
   for (auto &Note : D.Notes) {
     OS << "\n\n";
     printDiag(OS, Note);
   }
   OS.flush();
-  return Result;
+  return capitalize(std::move(Result));
 }
 
 /// Returns a message sent to LSP for the note of the main diagnostic.
 /// The message includes the main diagnostic to provide the necessary context
 /// for the user to understand the note.
 std::string noteMessage(const Diag &Main, const DiagBase &Note) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   OS << Note.Message;
   OS << "\n\n";
   printDiag(OS, Main);
   OS.flush();
-  return Result;
+  return capitalize(std::move(Result));
 }
 } // namespace
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D) {
   if (!D.InsideMainFile)
     OS << "[in " << D.File << "] ";
   return OS << D.Message;
 }

test/clangd/diagnostics.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"void main() {}"}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "return type of 'main' is not 'int'",
+# CHECK-NEXT:        "message": "Return type of 'main' is not 'int'",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 4,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 0,
 # CHECK-NEXT:            "line": 0

test/clangd/did-change-configuration-params.test

 # CHECK-NEXT:    "uri": "file://{{.*}}/bar.c"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c": {"workingDirectory":"/clangd-test2", "compilationCommand": ["clang", "-c", "foo.c", "-Wall", "-Werror"]}}}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "variable 'i' is uninitialized when used here",
+# CHECK-NEXT:        "message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 28,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 27,
 # CHECK-NEXT:            "line": 0

test/clangd/execute-command.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "using the result of an assignment as a condition without parentheses",
+# CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0

test/clangd/extra-flags.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main() { int i; return i; }"},"metadata":{"extraFlags":["-Wall"]}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "variable 'i' is uninitialized when used here",
+# CHECK-NEXT:        "message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 28,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 27,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
 {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.c","version":2},"contentChanges":[{"text":"int main() { int i; return i+1; }"}]}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "variable 'i' is uninitialized when used here",
+# CHECK-NEXT:        "message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 28,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 27,
 # CHECK-NEXT:            "line": 0

test/clangd/fixits.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
 #      CHECK:    "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
-# CHECK-NEXT:        "message": "using the result of an assignment as a condition without parentheses",
+# CHECK-NEXT:        "message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 37,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {
 # CHECK-NEXT:            "character": 32,
 # CHECK-NEXT:            "line": 0
 # CHECK-NEXT:          }
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "severity": 2
 # CHECK-NEXT:      }
 # CHECK-NEXT:    ],
 # CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses"}]}}}
 #      CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changes": {
 # CHECK-NEXT:            "file://{{.*}}/foo.c": [

 # CHECK-NEXT:          }
 # CHECK-NEXT:        }
 # CHECK-NEXT:      ],
 # CHECK-NEXT:      "command": "clangd.applyFix",
 # CHECK-NEXT:      "title": "Apply fix: use '==' to turn this assignment into an equality comparison"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  ]
 ---
-{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"}]}}}
+{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses"}]}}}
 # Make sure unused "code" and "source" fields ignored gracefully
 #      CHECK:  "id": 3,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changes": {

unittests/clangd/ClangdUnitTests.cpp

auto MatchingLSP = [](const DiagBase &D, llvm::StringRef Message) {
     clangd::Diagnostic Res;
     Res.range = D.Range;
     Res.severity = getSeverity(D.Severity);
     Res.message = Message;
     return Res;
   };
 
   // Diagnostics should turn into these:
-  clangd::Diagnostic MainLSP = MatchingLSP(D, R"(something terrible happened
+  clangd::Diagnostic MainLSP = MatchingLSP(D, R"(Something terrible happened
 
 main.cpp:6:7: remark: declared somewhere in the main file
 
 ../foo/baz/header.h:10:11:
 note: declared somewhere in the header file)");
 
   clangd::Diagnostic NoteInMainLSP =
-      MatchingLSP(NoteInMain, R"(declared somewhere in the main file
+      MatchingLSP(NoteInMain, R"(Declared somewhere in the main file
 
 main.cpp:2:3: error: something terrible happened)");
 
   // Transform dianostics and check the results.
   std::vector<std::pair<clangd::Diagnostic, std::vector<clangd::Fix>>> LSPDiags;
   toLSPDiags(D, [&](clangd::Diagnostic LSPDiag,
                     llvm::ArrayRef<clangd::Fix> Fixes) {
     LSPDiags.push_back({std::move(LSPDiag),