[clangd] Add support for per-file extra flags

[krasimir]

Summary

This patch adds the ability to specify user-defined extra flags per opened file
through the LSP layer. This is a non-standard extension to the protocol.
I've already created a feature request about it for upstream lsp:
https://github.com/Microsoft/language-server-protocol/issues/255

The particular use-case is ycmd, which has a python script for figuring out
extra flags per file:
https://github.com/Valloric/ycmd#flagsforfile-filename-kwargs-

[ilya-biryukov]

clangd/GlobalCompilationDatabase.cpp

20 ↗

(On Diff #105074)

Maybe pass Command by reference to avoid checking for null?
Is there a guideline to use pointers to make things more explicit that I've missed?

21 ↗

(On Diff #105074)

There's an assert that Command is always non-null, so !Command is always false.
Maybe remove this check?

23 ↗

(On Diff #105074)

There's an assert that checks that CommandLine is never empty.
Maybe remove this if statement?

27 ↗

(On Diff #105074)

This way extra flags won't override flags from compilation database, we should put ExtraFlags right before the input files.

clangd will break if it receives CommandLine with more than one input file, so it should be safe to assume that the last arg is the name of the input file, i.e. always add ExtraFlags right before the last arg.

38 ↗

(On Diff #105074)

Maybe extract a small helper function to get default CompileCommand and use it both in ClangdUnitStore and here?
Just in case we'll ever want to change the defaults at some point.

47 ↗

(On Diff #105074)

This will break for options, containing spaces (e.g. -I"/home/user/my dir with spaces").
There should a function parsing command-line args somewhere in LLVM? Can we avoid splitting command-line args altogether (see next comment)?

58 ↗

(On Diff #105074)

Maybe store splitted command-line args in ExtraFlagsForFile?
ycm_extra_conf.py seems to pass splited command-line arguments, maybe change our extension to receive a list of arguments to avoid doing command-line arg splitting?

[krasimir]

• Addess review comments

clangd/GlobalCompilationDatabase.cpp

20 ↗

(On Diff #105074)

There's a guideline to use pointers for google style, but I'm not aware for such for llvm style.

[ilya-biryukov]

clangd/GlobalCompilationDatabase.cpp

22 ↗

(On Diff #105147)

If Command.CommandLine.empty() is true, extra flags will be added before 'clang' in the command-line.
Maybe restore assert(Command.CommandLine.size() >= 2) (i.e. at least a compiler binary and input file should be in the commandline) and remove this if?

60 ↗

(On Diff #105147)

Maybe std::move the vector before assignment to avoid copying?

[krasimir]

• Addess review comments

[ilya-biryukov]

I forgot to submit this last comment yesterday, sorry about that.

clangd/GlobalCompilationDatabase.h

51 ↗

(On Diff #105257)

Maybe rename to setExtraFlagsForFile?
The name addExtraFlags... sounds like it appends to the existing list, but we actually replace the flags there.

[krasimir]

• Address review comment

[ilya-biryukov]

LGTM

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

 }
 
 void ClangdLSPServer::LSPProtocolCallbacks::onShutdown(JSONOutput &Out) {
   LangServer.IsDone = true;
 }
 
 void ClangdLSPServer::LSPProtocolCallbacks::onDocumentDidOpen(
     DidOpenTextDocumentParams Params, JSONOutput &Out) {
+  if (Params.metadata && !Params.metadata->extraFlags.empty())
+    LangServer.CDB.setExtraFlagsForFile(Params.textDocument.uri.file,
+                                        std::move(Params.metadata->extraFlags));
   LangServer.Server.addDocument(Params.textDocument.uri.file,
                                 Params.textDocument.text);
 }
 
 void ClangdLSPServer::LSPProtocolCallbacks::onDocumentDidChange(
     DidChangeTextDocumentParams Params, JSONOutput &Out) {
   // We only support full syncing right now.
   LangServer.Server.addDocument(Params.textDocument.uri.file,

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

void ClangdUnitStore::removeUnitIfPresent(PathRef File) {
   std::lock_guard<std::mutex> Lock(Mutex);
 
   auto It = OpenedFiles.find(File);
   if (It == OpenedFiles.end())
     return;
   OpenedFiles.erase(It);
 }
 
-std::vector<tooling::CompileCommand> ClangdUnitStore::getCompileCommands(GlobalCompilationDatabase &CDB, PathRef File) {
+std::vector<tooling::CompileCommand>
+ClangdUnitStore::getCompileCommands(GlobalCompilationDatabase &CDB,
+                                    PathRef File) {
   std::vector<tooling::CompileCommand> Commands = CDB.getCompileCommands(File);
-  if (Commands.empty()) {
+  if (Commands.empty())
     // Add a fake command line if we know nothing.
-    Commands.push_back(tooling::CompileCommand(
-        llvm::sys::path::parent_path(File), llvm::sys::path::filename(File),
-        {"clang", "-fsyntax-only", File.str()}, ""));
-  }
+    Commands.push_back(getDefaultCompileCommand(File));
   return Commands;
 }

clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h

 
 namespace tooling {
 class CompilationDatabase;
 struct CompileCommand;
 } // namespace tooling
 
 namespace clangd {
 
+/// Returns a default compile command to use for \p File.
+tooling::CompileCommand getDefaultCompileCommand(PathRef File);
+
 /// Provides compilation arguments used for building ClangdUnit.
 class GlobalCompilationDatabase {
 public:
   virtual ~GlobalCompilationDatabase() = default;
 
   virtual std::vector<tooling::CompileCommand>
   getCompileCommands(PathRef File) = 0;
 
   /// FIXME(ibiryukov): add facilities to track changes to compilation flags of
   /// existing targets.
 };
 
 /// Gets compile args from tooling::CompilationDatabases built for parent
 /// directories.
 class DirectoryBasedGlobalCompilationDatabase
     : public GlobalCompilationDatabase {
 public:
   std::vector<tooling::CompileCommand>
   getCompileCommands(PathRef File) override;
 
+  void setExtraFlagsForFile(PathRef File, std::vector<std::string> ExtraFlags);
+
 private:
   tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
 
   std::mutex Mutex;
   /// Caches compilation databases loaded from directories(keys are
   /// directories).
   llvm::StringMap<std::unique_ptr<clang::tooling::CompilationDatabase>>
       CompilationDatabases;
+
+  /// Stores extra flags per file.
+  llvm::StringMap<std::vector<std::string>> ExtraFlagsForFile;
 };
 } // namespace clangd
 } // namespace clang
 
 #endif

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

 //===--- GlobalCompilationDatabase.cpp --------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "GlobalCompilationDatabase.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
-using namespace clang::clangd;
-using namespace clang;
+namespace clang {
+namespace clangd {
+
+static void addExtraFlags(tooling::CompileCommand &Command,
+                          const std::vector<std::string> &ExtraFlags) {
+  if (ExtraFlags.empty())
+    return;
+  assert(Command.CommandLine.size() >= 2 &&
+         "Expected a command line containing at least 2 arguments, the "
+         "compiler binary and the output file");
+  // The last argument of CommandLine is the name of the input file.
+  // Add ExtraFlags before it.
+  auto It = Command.CommandLine.end();
+  --It;
+  Command.CommandLine.insert(It, ExtraFlags.begin(), ExtraFlags.end());
+}
+
+tooling::CompileCommand getDefaultCompileCommand(PathRef File) {
+  std::vector<std::string> CommandLine{"clang", "-fsyntax-only", File.str()};
+  return tooling::CompileCommand(llvm::sys::path::parent_path(File),
+                                 llvm::sys::path::filename(File), CommandLine,
+                                 /*Output=*/"");
+}
 
 std::vector<tooling::CompileCommand>
 DirectoryBasedGlobalCompilationDatabase::getCompileCommands(PathRef File) {
   std::vector<tooling::CompileCommand> Commands;
 
   auto CDB = getCompilationDatabase(File);
-  if (!CDB)
-    return {};
-  return CDB->getCompileCommands(File);
+  if (CDB)
+    Commands = CDB->getCompileCommands(File);
+  if (Commands.empty())
+    Commands.push_back(getDefaultCompileCommand(File));
+
+  auto It = ExtraFlagsForFile.find(File);
+  if (It != ExtraFlagsForFile.end()) {
+    // Append the user-specified flags to the compile commands.
+    for (tooling::CompileCommand &Command : Commands)
+      addExtraFlags(Command, It->second);
+  }
+
+  return Commands;
+}
+
+void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile(
+    PathRef File, std::vector<std::string> ExtraFlags) {
+  ExtraFlagsForFile[File] = std::move(ExtraFlags);
 }
 
 tooling::CompilationDatabase *
 DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
   std::lock_guard<std::mutex> Lock(Mutex);
 
   namespace path = llvm::sys::path;
 
for (auto Path = path::parent_path(File); !Path.empty();
     return result;
   }
 
   // FIXME(ibiryukov): logging
   // Output.log("Failed to find compilation database for " + Twine(File) +
   // "\n");
   return nullptr;
 }
+
+} // namespace clangd
+} // namespace clang

clang-tools-extra/trunk/clangd/Protocol.h

struct Location {
 
   friend bool operator<(const Location &LHS, const Location &RHS) {
     return std::tie(LHS.uri, LHS.range) < std::tie(RHS.uri, RHS.range);
   }
 
   static std::string unparse(const Location &P);
 };
 
+struct Metadata {
+  std::vector<std::string> extraFlags;
+
+  static llvm::Optional<Metadata> parse(llvm::yaml::MappingNode *Params);
+};
+
 struct TextEdit {
   /// The range of the text document to be manipulated. To insert
   /// text into a document create a range where start === end.
   Range range;
 
   /// The string to be inserted. For delete operations use an
   /// empty string.
   std::string newText;
struct TextDocumentItem {
   static llvm::Optional<TextDocumentItem>
   parse(llvm::yaml::MappingNode *Params);
 };
 
 struct DidOpenTextDocumentParams {
   /// The document that was opened.
   TextDocumentItem textDocument;
 
+  /// Extension storing per-file metadata, such as compilation flags.
+  llvm::Optional<Metadata> metadata;
+
   static llvm::Optional<DidOpenTextDocumentParams>
   parse(llvm::yaml::MappingNode *Params);
 };
 
 struct DidCloseTextDocumentParams {
   /// The document that was closed.
   TextDocumentIdentifier textDocument;
 

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

if (KeyValue == "uri") {
       Result.text = Value->getValue(Storage);
     } else {
       return llvm::None;
     }
   }
   return Result;
 }
 
+llvm::Optional<Metadata> Metadata::parse(llvm::yaml::MappingNode *Params) {
+  Metadata Result;
+  for (auto &NextKeyValue : *Params) {
+    auto *KeyString = dyn_cast<llvm::yaml::ScalarNode>(NextKeyValue.getKey());
+    if (!KeyString)
+      return llvm::None;
+
+    llvm::SmallString<10> KeyStorage;
+    StringRef KeyValue = KeyString->getValue(KeyStorage);
+    auto *Value = NextKeyValue.getValue();
+
+    llvm::SmallString<10> Storage;
+    if (KeyValue == "extraFlags") {
+      auto *Seq = dyn_cast<llvm::yaml::SequenceNode>(Value);
+      if (!Seq)
+        return llvm::None;
+      for (auto &Item : *Seq) {
+        auto *Node = dyn_cast<llvm::yaml::ScalarNode>(&Item);
+        if (!Node)
+          return llvm::None;
+        Result.extraFlags.push_back(Node->getValue(Storage));
+      }
+    }
+  }
+  return Result;
+}
+
 llvm::Optional<TextEdit> TextEdit::parse(llvm::yaml::MappingNode *Params) {
   TextEdit Result;
   for (auto &NextKeyValue : *Params) {
     auto *KeyString = dyn_cast<llvm::yaml::ScalarNode>(NextKeyValue.getKey());
     if (!KeyString)
       return llvm::None;
 
     llvm::SmallString<10> KeyStorage;
if (!Value)
       return llvm::None;
 
     llvm::SmallString<10> Storage;
     if (KeyValue == "textDocument") {
       auto Parsed = TextDocumentItem::parse(Value);
       if (!Parsed)
         return llvm::None;
       Result.textDocument = std::move(*Parsed);
+    } else if (KeyValue == "metadata") {
+      auto Parsed = Metadata::parse(Value);
+      if (!Parsed)
+        return llvm::None;
+      Result.metadata = std::move(*Parsed);
     } else {
       return llvm::None;
     }
   }
   return Result;
 }
 
 llvm::Optional<DidCloseTextDocumentParams>

clang-tools-extra/trunk/test/clangd/extra-flags.test

+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+#
+Content-Length: 205
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"int main() { int i; return i; }"},"metadata":{"extraFlags":["-Wall"]}}}
+# CHECK: {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///foo.c","diagnostics":[{"range":{"start": {"line": 0, "character": 28}, "end": {"line": 0, "character": 28}},"severity":2,"message":"variable 'i' is uninitialized when used here"},{"range":{"start": {"line": 0, "character": 19}, "end": {"line": 0, "character": 19}},"severity":3,"message":"initialize the variable 'i' to silence this warning"}]}}
+#
+Content-Length: 175
+
+{"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///foo.c","version":2},"contentChanges":[{"text":"int main() { int i; return i; }"}]}}
+# CHECK: {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///foo.c","diagnostics":[{"range":{"start": {"line": 0, "character": 28}, "end": {"line": 0, "character": 28}},"severity":2,"message":"variable 'i' is uninitialized when used here"},{"range":{"start": {"line": 0, "character": 19}, "end": {"line": 0, "character": 19}},"severity":3,"message":"initialize the variable 'i' to silence this warning"}]}}
+#
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+
+