[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

2

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

[arphaman]

Remove test and update unit test.

test/clangd/capitalize-diagnostics.test

2

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/ClangdLSPServer.h

 /// This class provides implementation of an LSP server, glueing the JSON
 /// dispatch and ClangdServer together.
 class ClangdLSPServer : private DiagnosticsConsumer, private ProtocolCallbacks {
 public:
   /// If \p CompileCommandsDir has a value, compile_commands.json will be
   /// loaded only from \p CompileCommandsDir. Otherwise, clangd will look
   /// for compile_commands.json in all parent directories of each file.
   ClangdLSPServer(JSONOutput &Out, const clangd::CodeCompleteOptions &CCOpts,
+                  const clangd::DiagnosticOptions &DiagOpts,
                   llvm::Optional<Path> CompileCommandsDir,
                   bool ShouldUseInMemoryCDB, const ClangdServer::Options &Opts);
 
   /// Run LSP server loop, receiving input for it from \p In. \p In must be
   /// opened in binary mode. Output will be written using Out variable passed to
   /// class constructor. This method must not be executed more than once for
   /// each instance of ClangdLSPServer.
   ///
private:
 
   // Various ClangdServer parameters go here. It's important they're created
   // before ClangdServer.
   CompilationDB CDB;
 
   RealFileSystemProvider FSProvider;
   /// Options used for code completion
   clangd::CodeCompleteOptions CCOpts;
+  /// Options used for diagnostics.
+  clangd::DiagnosticOptions DiagOpts;
   /// The supported kinds of the client.
   SymbolKindBitset SupportedSymbolKinds;
 
   // Store of the current versions of the open documents.
   DraftStore DraftMgr;
 
   // Server must be the last member of the class to allow its destructor to exit
   // the worker thread that may otherwise run an async callback on partially
   // destructed instance of ClangdLSPServer.
   ClangdServer Server;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clangd/ClangdLSPServer.cpp

 // FIXME: This function needs to be properly tested.
 void ClangdLSPServer::onChangeConfiguration(
     DidChangeConfigurationParams &Params) {
   applyConfiguration(Params.settings);
 }
 
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
                                  const clangd::CodeCompleteOptions &CCOpts,
+                                 const clangd::DiagnosticOptions &DiagOpts,
                                  llvm::Optional<Path> CompileCommandsDir,
                                  bool ShouldUseInMemoryCDB,
                                  const ClangdServer::Options &Opts)
     : Out(Out), CDB(ShouldUseInMemoryCDB ? CompilationDB::makeInMemory()
                                          : CompilationDB::makeDirectoryBased(
                                                std::move(CompileCommandsDir))),
-      CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()),
+      CCOpts(CCOpts), DiagOpts(DiagOpts),
+      SupportedSymbolKinds(defaultSymbolKinds()),
       Server(CDB.getCDB(), FSProvider, /*DiagConsumer=*/*this, Opts) {}
 
 bool ClangdLSPServer::run(std::FILE *In, JSONStreamStyle InputStyle) {
   assert(!IsDone && "Run was called before");
 
   // Set up JSONRPCDispatcher.
   JSONRPCDispatcher Dispatcher([](const json::Value &Params) {
     replyError(ErrorCode::MethodNotFound, "method not found");

 }
 
 void ClangdLSPServer::onDiagnosticsReady(PathRef File,
                                          std::vector<Diag> Diagnostics) {
   json::Array DiagnosticsJSON;
 
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
   for (auto &Diag : Diagnostics) {
-    toLSPDiags(Diag, [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
+    toLSPDiags(Diag, DiagOpts,
+               [&](clangd::Diagnostic Diag, llvm::ArrayRef<Fix> Fixes) {
                  DiagnosticsJSON.push_back(json::Object{
                      {"range", Diag.range},
                      {"severity", Diag.severity},
                      {"message", Diag.message},
                  });
 
                  auto &FixItsForDiagnostic = LocalFixIts[Diag];
                  std::copy(Fixes.begin(), Fixes.end(),
                            std::back_inserter(FixItsForDiagnostic));
                });
   }
 
   // Cache FixIts
   {
     // FIXME(ibiryukov): should be deleted when documents are removed
     std::lock_guard<std::mutex> Lock(FixItsMutex);
     FixItsMap[File] = LocalFixIts;
   }

clangd/ClangdUnit.cpp

if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
     // We proceed anyway, our lit-tests rely on results for non-existing working
     // dirs.
   }
 
   // FIXME(ibiryukov): store diagnostics from CommandLine when we start
   // reporting them.
   IgnoreDiagnostics IgnoreDiagnostics;
   IntrusiveRefCntPtr<DiagnosticsEngine> CommandLineDiagsEngine =
-      CompilerInstance::createDiagnostics(new DiagnosticOptions,
+      CompilerInstance::createDiagnostics(new clang::DiagnosticOptions,
                                           &IgnoreDiagnostics, false);
   std::unique_ptr<CompilerInvocation> CI = createInvocationFromCommandLine(
       ArgStrs, CommandLineDiagsEngine, Inputs.FS);
   if (!CI)
     return nullptr;
   // createInvocationFromCommandLine sets DisableFree.
   CI->getFrontendOpts().DisableFree = false;
   CI->getLangOpts()->CommentOpts.ParseAllComments = true;

clangd/Diagnostics.h

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include <cassert>
 #include <string>
 
 namespace clang {
 namespace clangd {
 
+struct DiagnosticOptions {
+  /// Capitalize the first character in the diagnostic's message.
+  bool CapitalizeDiagnosticText = false;
+};
+
 /// Contains basic information about a diagnostic.
 struct DiagBase {
   std::string Message;
   // Intended to be used only in error messages.
   // May be relative, absolute or even artifically constructed.
   std::string File;
   clangd::Range Range;
   DiagnosticsEngine::Level Severity = DiagnosticsEngine::Note;

 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Diag &D);
 
 /// Conversion to LSP diagnostics. Formats the error message of each diagnostic
 /// to include all its notes. Notes inside main file are also provided as
 /// separate diagnostics with their corresponding fixits. Notes outside main
 /// file do not have a corresponding LSP diagnostic, but can still be included
 /// as part of their main diagnostic's message.
 void toLSPDiags(
-    const Diag &D,
+    const Diag &D, const DiagnosticOptions &DiagOpts,
     llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn);
 
 /// Convert from clang diagnostic level to LSP severity.
 int getSeverity(DiagnosticsEngine::Level L);
 
 /// StoreDiags collects the diagnostics that can later be reported by
 /// clangd. It groups all notes for a diagnostic into a single Diag
 /// and filters out diagnostics that don't mention the main file (i.e. neither

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;
 }
 
+/// Applies common text transformations to a diagnostic's string.
+std::string capitalizeIfNeeded(std::string Message,
+                               const DiagnosticOptions &DiagOpts) {
+  if (DiagOpts.CapitalizeDiagnosticText && !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 mainMessage(const Diag &D, const DiagnosticOptions &DiagOpts) {
   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 capitalizeIfNeeded(std::move(Result), DiagOpts);
 }
 
 /// 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 noteMessage(const Diag &Main, const DiagBase &Note,
+                        const DiagnosticOptions &DiagOpts) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   OS << Note.Message;
   OS << "\n\n";
   printDiag(OS, Main);
   OS.flush();
-  return Result;
+  return capitalizeIfNeeded(std::move(Result), DiagOpts);
 }
 } // namespace
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D) {
   if (!D.InsideMainFile)
     OS << "[in " << D.File << "] ";
   return OS << D.Message;
 }
for (auto &Fix : D.Fixes) {
       OS << Sep << Fix;
       Sep = ", ";
     }
   }
   return OS;
 }
 
 void toLSPDiags(
-    const Diag &D,
+    const Diag &D, const DiagnosticOptions &DiagOpts,
     llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
   auto FillBasicFields = [](const DiagBase &D) -> clangd::Diagnostic {
     clangd::Diagnostic Res;
     Res.range = D.Range;
     Res.severity = getSeverity(D.Severity);
     return Res;
   };
 
   {
     clangd::Diagnostic Main = FillBasicFields(D);
-    Main.message = mainMessage(D);
+    Main.message = mainMessage(D, DiagOpts);
     OutFn(std::move(Main), D.Fixes);
   }
 
   for (auto &Note : D.Notes) {
     if (!Note.InsideMainFile)
       continue;
     clangd::Diagnostic Res = FillBasicFields(Note);
-    Res.message = noteMessage(D, Note);
+    Res.message = noteMessage(D, Note, DiagOpts);
     OutFn(std::move(Res), llvm::ArrayRef<Fix>());
   }
 }
 
 int getSeverity(DiagnosticsEngine::Level L) {
   switch (L) {
   case DiagnosticsEngine::Remark:
     return 4;

clangd/tool/ClangdMain.cpp

static llvm::cl::opt<CompileArgsFrom> CompileArgsFrom(
     llvm::cl::values(clEnumValN(LSPCompileArgs, "lsp",
                                 "All compile commands come from LSP and "
                                 "'compile_commands.json' files are ignored"),
                      clEnumValN(FilesystemCompileArgs, "filesystem",
                                 "All compile commands come from the "
                                 "'compile_commands.json' files")),
     llvm::cl::init(FilesystemCompileArgs), llvm::cl::Hidden);
 
+static llvm::cl::opt<bool> CapitalizeDiagnosticText(
+    "capitialize-diagnostic-text",
+    llvm::cl::desc(
+        "Capitalize the first character in the diagnostic's message"),
+    llvm::cl::init(false));
+
 int main(int argc, char *argv[]) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {
     OS << clang::getClangToolFullVersion("clangd") << "\n";
   });
   llvm::cl::ParseCommandLineOptions(
       argc, argv,
       "clangd is a language server that provides IDE-like features to editors. "
int main(int argc, char *argv[]) {
   CCOpts.Limit = LimitResults;
   CCOpts.BundleOverloads = CompletionStyle != Detailed;
   CCOpts.ShowOrigins = ShowOrigins;
   if (!HeaderInsertionDecorators) {
     CCOpts.IncludeIndicator.Insert.clear();
     CCOpts.IncludeIndicator.NoInsert.clear();
   }
 
+  clangd::DiagnosticOptions DiagOpts;
+  DiagOpts.CapitalizeDiagnosticText = CapitalizeDiagnosticText;
+
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
-      Out, CCOpts, CompileCommandsDirPath,
+      Out, CCOpts, DiagOpts, CompileCommandsDirPath,
       /*ShouldUseInMemoryCDB=*/CompileArgsFrom == LSPCompileArgs, Opts);
   constexpr int NoShutdownRequestErrorCode = 1;
   llvm::set_thread_name("clangd.main");
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
   return LSPServer.run(stdin, InputStyle) ? 0 : NoShutdownRequestErrorCode;
 }

test/clangd/capitalize-diagnostics.test

+# RUN: clangd -capitialize-diagnostic-text -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 foo() { }\nvoid foo() { }"}}}
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "message": "Redefinition of 'foo'\n\nfoo.c:1:6: note: previous definition is here",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": 8,
+# CHECK-NEXT:            "line": 1
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": 5,
+# CHECK-NEXT:            "line": 1
+# CHECK-NEXT:          }
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "severity": 1
+# CHECK-NEXT:      },
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "message": "Previous definition is here\n\nfoo.c:2:6: error: redefinition of 'foo'",
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": 8,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": 5,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          }
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "severity": 3
+# CHECK-NEXT:      }
+# CHECK-NEXT:    ],
+# CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}