[clangd] Add support for per-file override compilation command

[arphaman]

Summary

This patch builds on top of the "extra flags" extension added in r307241.
It adds the ability to specify user-defined custom compilation command for an opened file through the LSP layer. This is a non-standard extension to the protocol.

The particular use-case is follows: our clients do not have a compilation database and will specify all compilation commands dynamically when a file is open.

[arphaman]

Btw, the "extraFlags" extension is still usable with "compilationCommand".

[malaperle]

Interesting! We also have a need for passing compilation commands in a context where there is no compile_commands.json, but we were thinking of putting this in a "didChangeConfiguration" message so that all the commands would be available even before files are opened. This would be allow Clangd to have the necessary information for background indexing which would include unopened files. Subsequent changes to compilation commands would probably go through a similar didChangeConfiguration and the appropriate (opened) files would get reparsed (not unlike D49267). I'm making a few guesses here: I assume that in the context of XCode, you would not do background indexing in Clangd but let XCode do it as it can also coordinate (and not overlap) with build tasks. Is that correct? In any case, I think the approach in the patch is not incompatible with what we had in mind, i.e. we could also reuse "overrideCompilationCommandForFile" for each file specified in didChangeConfiguration. I'm point this out because if you *do* end up needing all the compilation commands beforehand like I mentioned, then maybe we can skip the approach of specifying them with didOpen and send them all with didChangeConfiguration from start.

[arphaman]

In D49523#1167553, @malaperle wrote:

Interesting! We also have a need for passing compilation commands in a context where there is no compile_commands.json, but we were thinking of putting this in a "didChangeConfiguration" message so that all the commands would be available even before files are opened. This would be allow Clangd to have the necessary information for background indexing which would include unopened files. Subsequent changes to compilation commands would probably go through a similar didChangeConfiguration and the appropriate (opened) files would get reparsed (not unlike D49267). I'm making a few guesses here: I assume that in the context of XCode, you would not do background indexing in Clangd but let XCode do it as it can also coordinate (and not overlap) with build tasks. Is that correct? In any case, I think the approach in the patch is not incompatible with what we had in mind, i.e. we could also reuse "overrideCompilationCommandForFile" for each file specified in didChangeConfiguration. I'm point this out because if you *do* end up needing all the compilation commands beforehand like I mentioned, then maybe we can skip the approach of specifying them with didOpen and send them all with didChangeConfiguration from start.

Thanks for your response,
As it stands right now we will not run the indexer in Clangd for our use case, and it's unclear if we can even assemble a set of compile commands, so we would like to provide the commands when a file is open. We might be interested in a "didChangeConfiguration" message extension in the future (ideally it would be possible to change the subset of the constructed compilation database).

[jkorous]

Hi Marc-Andre, what is a structure of data you are passing as parameter of didChangeConfiguration message? All we need is to pass per-file compilation command to clangd. Maybe we could send didChangeConfiguration message right after didOpen.

EDIT: Well, provided we would find a way how not to parse the file twice.

[jkorous]

Alex, I am just wondering if we shouldn't rather create another implementation of GlobalCompilationDatabase interface (something like InMemoryGlobalCompilationDatabase), add it to ClangdServer and use it as the first place to be searched in ClangdServer::getCompileCommand(PathRef File). What do you think?

[malaperle]

In D49523#1169000, @arphaman wrote:

In D49523#1167553, @malaperle wrote:

Interesting! We also have a need for passing compilation commands in a context where there is no compile_commands.json, but we were thinking of putting this in a "didChangeConfiguration" message so that all the commands would be available even before files are opened. This would be allow Clangd to have the necessary information for background indexing which would include unopened files. Subsequent changes to compilation commands would probably go through a similar didChangeConfiguration and the appropriate (opened) files would get reparsed (not unlike D49267). I'm making a few guesses here: I assume that in the context of XCode, you would not do background indexing in Clangd but let XCode do it as it can also coordinate (and not overlap) with build tasks. Is that correct? In any case, I think the approach in the patch is not incompatible with what we had in mind, i.e. we could also reuse "overrideCompilationCommandForFile" for each file specified in didChangeConfiguration. I'm point this out because if you *do* end up needing all the compilation commands beforehand like I mentioned, then maybe we can skip the approach of specifying them with didOpen and send them all with didChangeConfiguration from start.

Thanks for your response,
As it stands right now we will not run the indexer in Clangd for our use case, and it's unclear if we can even assemble a set of compile commands, so we would like to provide the commands when a file is open. We might be interested in a "didChangeConfiguration" message extension in the future (ideally it would be possible to change the subset of the constructed compilation database).

Sounds good, I think sending it in didOpen makes sense then. And yes, I agree that we would need to support changing a subset of commands through didChangeConfiguration, eventually!

In D49523#1169620, @jkorous wrote:

Hi Marc-Andre, what is a structure of data you are passing as parameter of didChangeConfiguration message?

We don't yet :) But we will need to send the information per-file through it instead of using the compile_commands.json. Right now, what is supported in the didChangeConfiguration is pointing to a different compile_commands.json, in order to support multiple configuration (actually pointing to the folder containing the json file).

All we need is to pass per-file compilation command to clangd. Maybe we could send didChangeConfiguration message right after didOpen.

EDIT: Well, provided we would find a way how not to parse the file twice.

The idea would be to send the per-file commands to clangd *before* anything is opened. So the didOpen would just read the latest command in memory. And subsequent changes to commands would be communicated with didChangeConfiguration and then files would get reparsed. I'm actually thinking we might want to send the commands in the "initialize" request for all the initial commands and then update them with didChangeConfiguration whenever they change. That way, there is no risk for reparsing as we should not do anything (indexing!) before the initialize.
But it doesn't sounds like you need this right now :)

[jkorous]

BTW it looks like we already had kind of support for compilation command before (extra flags).

commit 5ec1f7ca32eb85077a22ce81d41aa02a017d4852
Author: Krasimir Georgiev <[e-mail]>
Date: Thu Jul 6 08:44:54 2017 +0000

[clangd] Add support for per-file extra flags

There is even an LSP extension proposal:

https://github.com/Microsoft/language-server-protocol/issues/255

But it seems as if we lost access to it here:

commit aa49372548ff984ae9c48879a0d05833538a76b3
Author: Sam McCall <[e-mail]>
Date: Mon Dec 4 10:08:45 2017 +0000

[clangd] GlobalCompilationDatabase interface changes

[arphaman]

In D49523#1169743, @jkorous wrote:

BTW it looks like we already had kind of support for compilation command before (extra flags).

commit 5ec1f7ca32eb85077a22ce81d41aa02a017d4852
Author: Krasimir Georgiev <[e-mail]>
Date: Thu Jul 6 08:44:54 2017 +0000

[clangd] Add support for per-file extra flags

There is even an LSP extension proposal:

https://github.com/Microsoft/language-server-protocol/issues/255

But it seems as if we lost access to it here:

commit aa49372548ff984ae9c48879a0d05833538a76b3
Author: Sam McCall <[e-mail]>
Date: Mon Dec 4 10:08:45 2017 +0000

[clangd] GlobalCompilationDatabase interface changes

The extra-flags are still supported, see the test for example. They're not a full compilation command, just extra flags.

[arphaman]

In D49523#1169728, @malaperle wrote:

In D49523#1169000, @arphaman wrote:

In D49523#1167553, @malaperle wrote:

Interesting! We also have a need for passing compilation commands in a context where there is no compile_commands.json, but we were thinking of putting this in a "didChangeConfiguration" message so that all the commands would be available even before files are opened. This would be allow Clangd to have the necessary information for background indexing which would include unopened files. Subsequent changes to compilation commands would probably go through a similar didChangeConfiguration and the appropriate (opened) files would get reparsed (not unlike D49267). I'm making a few guesses here: I assume that in the context of XCode, you would not do background indexing in Clangd but let XCode do it as it can also coordinate (and not overlap) with build tasks. Is that correct? In any case, I think the approach in the patch is not incompatible with what we had in mind, i.e. we could also reuse "overrideCompilationCommandForFile" for each file specified in didChangeConfiguration. I'm point this out because if you *do* end up needing all the compilation commands beforehand like I mentioned, then maybe we can skip the approach of specifying them with didOpen and send them all with didChangeConfiguration from start.

Thanks for your response,
As it stands right now we will not run the indexer in Clangd for our use case, and it's unclear if we can even assemble a set of compile commands, so we would like to provide the commands when a file is open. We might be interested in a "didChangeConfiguration" message extension in the future (ideally it would be possible to change the subset of the constructed compilation database).

Sounds good, I think sending it in didOpen makes sense then. And yes, I agree that we would need to support changing a subset of commands through didChangeConfiguration, eventually!

In D49523#1169620, @jkorous wrote:

Hi Marc-Andre, what is a structure of data you are passing as parameter of didChangeConfiguration message?

We don't yet :) But we will need to send the information per-file through it instead of using the compile_commands.json. Right now, what is supported in the didChangeConfiguration is pointing to a different compile_commands.json, in order to support multiple configuration (actually pointing to the folder containing the json file).

All we need is to pass per-file compilation command to clangd. Maybe we could send didChangeConfiguration message right after didOpen.

EDIT: Well, provided we would find a way how not to parse the file twice.

The idea would be to send the per-file commands to clangd *before* anything is opened. So the didOpen would just read the latest command in memory. And subsequent changes to commands would be communicated with didChangeConfiguration and then files would get reparsed. I'm actually thinking we might want to send the commands in the "initialize" request for all the initial commands and then update them with didChangeConfiguration whenever they change. That way, there is no risk for reparsing as we should not do anything (indexing!) before the initialize.
But it doesn't sounds like you need this right now :)

I reconsidered our needs. We would actually want to use the 'didChangeConfiguration' extension right now as well. I will work on a patch for it that is based on top of this one :)

[arphaman]

In D49523#1169621, @jkorous wrote:

Alex, I am just wondering if we shouldn't rather create another implementation of GlobalCompilationDatabase interface (something like InMemoryGlobalCompilationDatabase), add it to ClangdServer and use it as the first place to be searched in ClangdServer::getCompileCommand(PathRef File). What do you think?

We can certainly create another class, but I'd be against adding it into ClangdServer. It makes sense to have just one CDB in there. If we had our own server instead of ClangdLSP then this class would be really useful, but for now this solution is probably preferable.

[ilya-biryukov]

The extensions itself seems like a reasonable way to provide compile commands for the individual files. In case didChangeConfiguration does not work for you for some reason, happy to take a look at this change too.
One drawback of using didChangeConfiguration for compile commands is that it forces global invalidation, which may be a bad thing if your client tracks compile commands on a smaller scale and can send requests to invalidate individual files. However, if you're happy with it, we might get away with not adding yet another mechanism to pass compile commands to clangd, which is definitely a good thing!

[arphaman]

In D49523#1171484, @ilya-biryukov wrote:

The extensions itself seems like a reasonable way to provide compile commands for the individual files. In case didChangeConfiguration does not work for you for some reason, happy to take a look at this change too.
One drawback of using didChangeConfiguration for compile commands is that it forces global invalidation, which may be a bad thing if your client tracks compile commands on a smaller scale and can send requests to invalidate individual files. However, if you're happy with it, we might get away with not adding yet another mechanism to pass compile commands to clangd, which is definitely a good thing!

We actually need both mechanisms. I posted the didChangeConfiguration extension to https://reviews.llvm.org/D49758.

[ilya-biryukov]

In D49523#1174086, @arphaman wrote:

We actually need both mechanisms. I posted the didChangeConfiguration extension to https://reviews.llvm.org/D49758.

Why do we need both? Can we have only the one from D49758 and send two requests (compile command changed for file 'X' + didOpen('X')) whenever a new file gets added?

[arphaman]

In D49523#1174627, @ilya-biryukov wrote:

In D49523#1174086, @arphaman wrote:

We actually need both mechanisms. I posted the didChangeConfiguration extension to https://reviews.llvm.org/D49758.

Why do we need both? Can we have only the one from D49758 and send two requests (compile command changed for file 'X' + didOpen('X')) whenever a new file gets added?

The didChangeConfiguration command would re-parse all the open documents, which is something we want to avoid. I guess one solution is to avoid reparsing anything when the updated file is not in the database in the first place. I'll work on that.

[arphaman]

Abandoned in favor of https://reviews.llvm.org/D49758 (will update it today).

clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onShutdown(ShutdownParams &Params) {
   ShutdownRequestReceived = true;
   reply(nullptr);
 }
 
 void ClangdLSPServer::onExit(ExitParams &Params) { IsDone = true; }
 
 void ClangdLSPServer::onDocumentDidOpen(DidOpenTextDocumentParams &Params) {
   PathRef File = Params.textDocument.uri.file();
-  if (Params.metadata && !Params.metadata->extraFlags.empty()) {
+  if (Params.metadata) {
+    if (!Params.metadata->extraFlags.empty()) {
       NonCachedCDB.setExtraFlagsForFile(File,
                                         std::move(Params.metadata->extraFlags));
       CDB.invalidate(File);
     }
+    if (!Params.metadata->compilationCommand.empty()) {
+      NonCachedCDB.overrideCompilationCommandForFile(
+          File, std::move(Params.metadata->compilationCommand));
+      CDB.invalidate(File);
+    }
+  }
 
   std::string &Contents = Params.textDocument.text;
 
   DraftMgr.addDraft(File, Contents);
   Server.addDocument(File, Contents, WantDiagnostics::Yes);
 }
 
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {

clangd/GlobalCompilationDatabase.h

public:
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
   /// Set the compile commands directory to \p P.
   void setCompileCommandsDir(Path P);
 
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector<std::string> ExtraFlags);
 
+  /// Overrides the compilation command for a particular file.
+  void overrideCompilationCommandForFile(
+      PathRef File, std::vector<std::string> CompilationCommand);
+
 private:
   tooling::CompilationDatabase *getCDBForFile(PathRef File) const;
   tooling::CompilationDatabase *getCDBInDirLocked(PathRef File) const;
   void addExtraFlags(PathRef File, tooling::CompileCommand &C) const;
+  llvm::Optional<tooling::CompileCommand>
+  getOverridenCompilationCommand(PathRef File) const;
 
   mutable std::mutex Mutex;
   /// Caches compilation databases loaded from directories(keys are
   /// directories).
   mutable llvm::StringMap<std::unique_ptr<clang::tooling::CompilationDatabase>>
       CompilationDatabases;
 
   /// Stores extra flags per file.
   llvm::StringMap<std::vector<std::string>> ExtraFlagsForFile;
+  /// Stores overriden compilation commands per file.
+  llvm::StringMap<std::vector<std::string>> OverridenCompilationCommands;
   /// Used for command argument pointing to folder where compile_commands.json
   /// is located.
   llvm::Optional<Path> CompileCommandsDir;
 };
 
 /// A wrapper around GlobalCompilationDatabase that caches the compile commands.
 /// Note that only results of getCompileCommand are cached.
 class CachingCompilationDb : public GlobalCompilationDatabase {

clangd/GlobalCompilationDatabase.cpp

DirectoryBasedGlobalCompilationDatabase(
         llvm::Optional<Path> CompileCommandsDir)
     : CompileCommandsDir(std::move(CompileCommandsDir)) {}
 
 DirectoryBasedGlobalCompilationDatabase::
     ~DirectoryBasedGlobalCompilationDatabase() = default;
 
 llvm::Optional<tooling::CompileCommand>
 DirectoryBasedGlobalCompilationDatabase::getCompileCommand(PathRef File) const {
+  if (auto Command = getOverridenCompilationCommand(File)) {
+    addExtraFlags(File, *Command);
+    return std::move(*Command);
+  }
   if (auto CDB = getCDBForFile(File)) {
     auto Candidates = CDB->getCompileCommands(File);
     if (!Candidates.empty()) {
       addExtraFlags(File, Candidates.front());
       return std::move(Candidates.front());
     }
   } else {
     log("Failed to find compilation database for {0}", File);
void DirectoryBasedGlobalCompilationDatabase::addExtraFlags(
 
   auto &Args = C.CommandLine;
   assert(Args.size() >= 2 && "Expected at least [compiler, source file]");
   // The last argument of CommandLine is the name of the input file.
   // Add ExtraFlags before it.
   Args.insert(Args.end() - 1, It->second.begin(), It->second.end());
 }
 
+void DirectoryBasedGlobalCompilationDatabase::overrideCompilationCommandForFile(
+    PathRef File, std::vector<std::string> CompilationCommand) {
+  std::lock_guard<std::mutex> Lock(Mutex);
+  OverridenCompilationCommands[File] = std::move(CompilationCommand);
+}
+
+llvm::Optional<tooling::CompileCommand>
+DirectoryBasedGlobalCompilationDatabase::getOverridenCompilationCommand(
+    PathRef File) const {
+  std::lock_guard<std::mutex> Lock(Mutex);
+  auto It = OverridenCompilationCommands.find(File);
+  if (It == OverridenCompilationCommands.end())
+    return None;
+  return tooling::CompileCommand(llvm::sys::path::parent_path(File),
+                                 llvm::sys::path::filename(File), It->second,
+                                 /*Output=*/"");
+}
+
 tooling::CompilationDatabase *
 DirectoryBasedGlobalCompilationDatabase::getCDBInDirLocked(PathRef Dir) const {
   // FIXME(ibiryukov): Invalidate cached compilation databases on changes
   auto CachedIt = CompilationDatabases.find(Dir);
   if (CachedIt != CompilationDatabases.end())
     return CachedIt->second.get();
   std::string Error = "";
   auto CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error);

clangd/Protocol.h

friend bool operator<(const Location &LHS, const Location &RHS) {
     return std::tie(LHS.uri, LHS.range) < std::tie(RHS.uri, RHS.range);
   }
 };
 llvm::json::Value toJSON(const Location &);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Location &);
 
 struct Metadata {
   std::vector<std::string> extraFlags;
+  std::vector<std::string> compilationCommand;
 };
 bool fromJSON(const llvm::json::Value &, Metadata &);
 
 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;
 

clangd/Protocol.cpp

return O && O.map("uri", R.uri) && O.map("languageId", R.languageId) &&
          O.map("version", R.version) && O.map("text", R.text);
 }
 
 bool fromJSON(const json::Value &Params, Metadata &R) {
   json::ObjectMapper O(Params);
   if (!O)
     return false;
   O.map("extraFlags", R.extraFlags);
+  O.map("compilationCommand", R.compilationCommand);
   return true;
 }
 
 bool fromJSON(const json::Value &Params, TextEdit &R) {
   json::ObjectMapper O(Params);
   return O && O.map("range", R.range) && O.map("newText", R.newText);
 }
 

test/clangd/override-compile-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; return i; }"},"metadata":{"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:        "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": 1
+# 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; }"}]}}
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [
+# CHECK-NEXT:      {
+# 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": 1
+# CHECK-NEXT:      }
+# CHECK-NEXT:    ],
+# CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
+# CHECK-NEXT:  }
+
+# extraFlags works together with compilationCommand!
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///bar.c","languageId":"c","version":1,"text":"int main() { int i; return i; }"},"metadata":{"compilationCommand":["clang", "-c", "bar.c", "-Wall", "-Werror"], "extraFlags":["-Wno-error=uninitialized"]}}}
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [
+# CHECK-NEXT:      {
+# 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://{{.*}}/bar.c"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+---
+{"jsonrpc":"2.0":"method":"exit"}
+
+