[clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request

[arphaman]

Summary

This patch is based on https://reviews.llvm.org/D49523.

It extends the 'workspace/didChangeConfiguration' request to allow us to pass in a compilation database subset that we would like to update in the workspace.

[ilya-biryukov]

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

clangd/Protocol.h

429

• Maybe add a comment that the key of the map is a file name?
• The value does not contain the working directory at the time, but we need that for building tooling::CompileCommand, maybe add it?

[arphaman]

In D49758#1174629, @ilya-biryukov wrote:

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

The client will control the commands exclusively.

clangd/Protocol.h

429

Could you please elaborate on the issue with the working directory? I didn't quite get that concern, sorry.

[ilya-biryukov]

In D49758#1177747, @arphaman wrote:

In D49758#1174629, @ilya-biryukov wrote:

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

The client will control the commands exclusively.

Maybe a cleaner design would be to untangle the two use-cases and control them with a flag to clangd?
I.e. we can have two implementations of compilation databases in clangd:

• one that uses clang tooling capabilities, i.e. reads compile_commands.json, etc.
• one that gets all compile commands from the protocol and won't use the clang tooling.

The command-line arg to clangd will control which implementation is used.

The advantage is that we don't have to think about interactions between the clang plugins and explicit overrides and it should be easier to make sure that we don't accidentally read compilation args from the wrong place.
Would also help to keep DirectoryBasedCompilationDatabase a bit simpler, and the other implementation would be extremely simple. WDYT?

[ilya-biryukov]

clangd/Protocol.h

429

If arguments passed to clang contain relative paths, it's important to run compilation in the same working directory as the compiler.
E.g. clang++ -Ideps/boost/include foo.cpp

Even if that's a non-issue for your use-case (e.g. all paths are absolute or clangd runs in the same working dir as clang would), we should still allow passing custom working directories in this protocol extension.

[arphaman]

In D49758#1178056, @ilya-biryukov wrote:

In D49758#1177747, @arphaman wrote:

In D49758#1174629, @ilya-biryukov wrote:

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

The client will control the commands exclusively.

Maybe a cleaner design would be to untangle the two use-cases and control them with a flag to clangd?
I.e. we can have two implementations of compilation databases in clangd:

• one that uses clang tooling capabilities, i.e. reads compile_commands.json, etc.
• one that gets all compile commands from the protocol and won't use the clang tooling.

The command-line arg to clangd will control which implementation is used.

The advantage is that we don't have to think about interactions between the clang plugins and explicit overrides and it should be easier to make sure that we don't accidentally read compilation args from the wrong place.
Would also help to keep DirectoryBasedCompilationDatabase a bit simpler, and the other implementation would be extremely simple. WDYT?

Sounds good to me. I'll update the patch to do that.
I'll add the flag as well. We won't be setting this flag directly though, our XPC protocol will assume it, but that's fine for us.

[arphaman]

Updated patch to address review comments:

• The compilation database updated are used only when '-in-memory-compile-commands' flag is used.
• It's now possible to set the working directory as well.

[ilya-biryukov]

clangd/ClangdLSPServer.h

106 ↗

(On Diff #157762)

This code starts to be a little hard to follow. Could we extract it into an external class that encapsulates this logic? Something like:

class ClangdLSPServer {

private:
  class CompilationDB {
  public:
    static CompilationDB makeInMemory();
    stiatc CompilationDB makeDirectoryBased();
  
    void invalidate(PathRef File);
     /// Only valid for in-memory CDB, no-op and error log on DirectoryBasedCDB.
    void setCompileCommandsForFile(PathRef File);
    /// Only valid for directory-based CDB, no-op and error log on InMemoryCDB;
    void setExtraFlags(PathRef File);
    /// Returns a CDB that should be used to get compile commands for the current instance of ClangdLSPServer.
    GlobalCompilationDatabase& getCDB(); 
  
  private:
     // if IsDirectoryBased is true, an instance of InMemoryCDB.
     // If IsDirectoryBased is false, an instance of DirectoryBasedCDB.  unique_ptr<GlobalCompilationDatabase> CDB;
    unique_ptr<GlobalCompilationDatabase> CDB;
    // non-null only for directory-based CDB
    unique_ptr<CachingCompilationDatabase> CachingCDB;
    bool IsDirectoryBased;
  };
 
  CompilationDB CDB;
  // .....
}

We can static_cast to InMemoryCDB or DirectoryBasedCDB based on the IsDirectoryBased flag to implement all the operations we define in the helper class.

clangd/GlobalCompilationDatabase.h

136 ↗

(On Diff #157762)

Maybe remove 'mutable' from Commands? We shouldn't need it.

136 ↗

(On Diff #157762)

Maybe make the value type CompileCommand instead of Optional<CompileCommand>?
We seem to only use the optional when inserting values and we could easily rewrite that code to use map::insert().

clangd/Protocol.h

426

Maybe use a shorter name, e.g. ClangdCompileCommand? This class looks *almost* like tooling::CompileCommand, so having a similar name seems reasonable.

clangd/tool/ClangdMain.cpp

170 ↗

(On Diff #157762)

The important distinction seems to be where the compile commands come from, maybe make the name centered on the fact that compile commands are read from the protocol rather from the filesystem?

E.g. using named option values:
--compile_args_from={lsp|filesystem}
Or a boolean option:
--compile_args_from_lsp

WDYT?

171 ↗

(On Diff #157762)

Maybe elaborate a bit more on this mode? E.g. clangd will get compile all commands from via LSP and won't look at the filesystem for 'compile_commands.json' files

[arphaman]

Address review comments

clangd/tool/ClangdMain.cpp

170 ↗

(On Diff #157762)

Yeah, that looks better. I added this argument in the updated patch.

[ilya-biryukov]

Just a few nits left.

clangd/ClangdLSPServer.cpp

433

Other usages of FileName in tooling::CompileCommand seem to set this either to an absolute or a relative-to-cwd path to the file for the command.
Maybe put the whole filepath there too? (even though I don't think we particularly rely on this anywhere in clangd)

539

Other methods already assume it's non-null for directory-based CDB, so maybe remove the check for it being non-null here or replace the check with an assert?

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

44

Matching the stderr output seems fragile, but that's the only way to inspect the actual compile command that's used, so that's probably fine here.
However, the paths on windows will probably differ (notice the regexp matches for file uris), so we might need to do the regexp match here as well, e.g.Updating file {{.*}}foo.c with command ... to keep the Windows buildbots happy.

[arphaman]

Updated to address review comments.

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

44

Good point.
I also realized that I have to make this a UNIX-specific test as the compilation database updates require a path, which will obviously be different on Windows.

[ilya-biryukov]

Thanks! LGTM

clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onChangeConfiguration(
   // Compilation database change.
   if (Settings.compilationDatabasePath.hasValue()) {
     NonCachedCDB.setCompileCommandsDir(
         Settings.compilationDatabasePath.getValue());
     CDB.clear();
 
     reparseOpenedFiles();
   }
+
+  // Update to the compilation database.
+  if (Settings.compilationDatabaseChanges) {
+    const auto &CompileCommandUpdates = *Settings.compilationDatabaseChanges;
+    for (const auto &Entry : CompileCommandUpdates) {
+      NonCachedCDB.overrideCompilationCommandForFile(Entry.first,
+                                                     std::move(Entry.second));
+    }
+    CDB.clear();
+
+    reparseOpenedFiles();
+  }
 }
 
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
                                  const clangd::CodeCompleteOptions &CCOpts,
                                  llvm::Optional<Path> CompileCommandsDir,
                                  const ClangdServer::Options &Opts)
     : Out(Out), NonCachedCDB(std::move(CompileCommandsDir)), CDB(NonCachedCDB),
       CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()),
       Server(CDB, FSProvider, /*DiagConsumer=*/*this, Opts) {}
Out.writeMessage(json::Object{
        }},
   });
 }
 
 void ClangdLSPServer::reparseOpenedFiles() {
   for (const Path &FilePath : DraftMgr.getActiveFiles())
     Server.addDocument(FilePath, *DraftMgr.getDraft(FilePath),
                        WantDiagnostics::Auto);
 }

clangd/Protocol.h

struct DidChangeWatchedFilesParams {
   /// The actual file events.
   std::vector<FileEvent> changes;
 };
 bool fromJSON(const llvm::json::Value &, DidChangeWatchedFilesParams &);
 
 /// Clangd extension to manage a workspace/didChangeConfiguration notification
 /// since the data received is described as 'any' type in LSP.
 struct ClangdConfigurationParamsChange {
   llvm::Optional<std::string> compilationDatabasePath;
+
+  // The changes that happened to the compilation database.
+  llvm::Optional<std::map<std::string, std::vector<std::string>>>
+      compilationDatabaseChanges;
 };
 bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
 
 struct DidChangeConfigurationParams {
   // We use this predefined struct because it is easier to use
   // than the protocol specified type of 'any'.
   ClangdConfigurationParamsChange settings;
 };

clangd/Protocol.cpp

 bool fromJSON(const json::Value &Params, DidChangeConfigurationParams &CCP) {
   json::ObjectMapper O(Params);
   return O && O.map("settings", CCP.settings);
 }
 
 bool fromJSON(const json::Value &Params,
               ClangdConfigurationParamsChange &CCPC) {
   json::ObjectMapper O(Params);
-  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath);
+  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath) &&
+         O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
 } // namespace clangd
 } // namespace clang

test/clangd/did-change-configuration-params.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; }"}}}
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [],
+# CHECK-NEXT:    "uri": "file://{{.*}}/foo.c"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///bar.c","languageId":"c","version":1,"text":"int main() { int i; return i; }"}}}
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [],
+# CHECK-NEXT:    "uri": "file://{{.*}}/bar.c"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c": ["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:  }
+
+#      CHECK:  "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:  "params": {
+# CHECK-NEXT:    "diagnostics": [],
+# CHECK-NEXT:    "uri": "file://{{.*}}/bar.c"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":5,"method":"shutdown"}
+---
+{"jsonrpc":"2.0":"method":"exit"}
+
+