[clangd] Config: config struct propagated through Context

[sammccall]

Summary

This introduces the "semantic form" of config exposed to features,
contrasted with the "syntactic form" exposed to users in e9fb1506b83d.

The two are not connected, CompiledFragment and Provider will bridge that gap.
Nor is configuration actually set: that needs changes to ClangdServer,
TUScheduler, and BackgroundQueue.

[sammccall]

config() -> Config::current(). "config" already names a namespace.

[kadircet]

Thanks, LGTM. Just a question around the order of config vs other mangling.

clang-tools-extra/clangd/CompileCommands.cpp

187

what's the rationale behind applying this before any other mangling?

I can see that the rest of the mangling happens to make sure clangd works out-of-the-box for "more" users, so should be safe to apply as a final step.
But on the other hand, applying config after those would give the user full control over the final command, which I believe is equally important.

[sammccall]

clang-tools-extra/clangd/CompileCommands.cpp

187

I'll be honest, I don't really know which is better here. The differences are subtle, and there are arguments for each. I think we should probably just pick one and be open to changing it later.

My reasoning for this behavior: currently the user view of compile commands is basically "strings in compile_commands.json", and this mangling we do is best thought of as modifying the behavior of the driver. E.g. in an ideal world -fsyntax-only would not be a flag, we'd just use APIs that imply that behavior.
In this view of the world, the user is expected to understand compile commands + tweaks but not the mangling, so placing tweaks after mangling means they can't really reason about the transformations. And it allows stripping structurally important things we inject like fsyntax-only which seems wrong.

This argument works better for some args/manglings than others, and the way we log args cuts against it a bit too.

[kadircet]

oops, thought I've stamped it last time.

clang-tools-extra/clangd/CompileCommands.cpp

187

SG, as you mentioned in the last paragraph I would be looking at logs to figure out what my compile commands for a file are, but may be it's just me. Hence having this tweaking in the middle was a little bit surprising. (Moreover, if one day we decide to have build system integrations it might imply there won't be any written compile_commands.json, but we'll rather fetch them on the fly and logs might be the only way to look at those commands. Even in such a scenario, I suppose changing the way we log might be a better approach because we indeed do more manipulations even after logging e.g. turning off preamble related options)

[sammccall]

clang-tools-extra/clangd/CompileCommands.cpp

187

Yeah logging earlier would be nice but there's a layering problem - CDB doesn't know if this is a file we're actually editing, and we only want to log if it is. I think this is annoying, but not urgent to solve.

[thakis]

The test fails on mac and win:
http://45.33.8.238/mac/16341/step_9.txt
http://45.33.8.238/win/18704/step_9.txt

[sammccall]

Thanks, reverted and looking into it.

clang-tools-extra/clangd/CMakeLists.txt

add_clang_library(clangDaemon
   AST.cpp
   ClangdLSPServer.cpp
   ClangdServer.cpp
   CodeComplete.cpp
   CodeCompletionStrings.cpp
   CollectMacros.cpp
   CompileCommands.cpp
   Compiler.cpp
+  Config.cpp
   ConfigYAML.cpp
   Diagnostics.cpp
   DraftStore.cpp
   ExpectedTypes.cpp
   FindSymbols.cpp
   FindTarget.cpp
   FileDistance.cpp
   Format.cpp

clang-tools-extra/clangd/CompileCommands.cpp

 //===--- CompileCommands.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CompileCommands.h"
+#include "Config.h"
 #include "support/Logger.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
CommandMangler CommandMangler::detect() {
   return Result;
 }
 
 CommandMangler CommandMangler::forTests() {
   return CommandMangler();
 }
 
 void CommandMangler::adjust(std::vector<std::string> &Cmd) const {
+  // FIXME: remove const_cast once unique_function is const-compatible.
+  for (auto &Edit : const_cast<Config &>(config()).CompileFlags.Edits)
+    Edit(Cmd);
+
   // Check whether the flag exists, either as -flag or -flag=*
   auto Has = [&](llvm::StringRef Flag) {
     for (llvm::StringRef Arg : Cmd) {
       if (Arg.consume_front(Flag) && (Arg.empty() || Arg[0] == '='))
         return true;
     }
     return false;
   };

clang-tools-extra/clangd/Config.h

+//===--- Config.h - User configuration of clangd behavior --------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Various clangd features have configurable behaviour (or can be disabled).
+// This file defines "resolved" configuration seen by features within clangd.
+// For example, settings may vary per-file, the resolved Config only contains
+// settings that apply to the current file.
+//
+// This is distinct from how the config is specified by the user (Fragment)
+// interpreted (CompiledFragment), and combined (Provider).
+// ConfigFragment.h describes the steps to add a new configuration option.
+//
+// Because this structure is shared throughout clangd, it's a potential source
+// of layering problems. Config should be expressed in terms of simple
+// vocubulary types where possible.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
+
+#include "support/Context.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include <string>
+#include <vector>
+
+namespace clang {
+namespace clangd {
+
+/// Settings that express user/project preferences and control clangd behavior.
+///
+/// Generally, features should consume config() and the caller is responsible
+/// for setting it appropriately. In practice these callers are ClangdServer,
+/// TUScheduler, and BackgroundQueue.
+struct Config {
+  /// Context key which can be used to set the current Config.
+  static clangd::Key<Config> Key;
+
+  Config() = default;
+  Config(const Config &) = delete;
+  Config &operator=(const Config &) = delete;
+  Config(Config &&) = default;
+  Config &operator=(Config &&) = default;
+
+  /// Controls how the compile command for the current file is determined.
+  struct {
+    // Edits to apply to the compile command, in sequence.
+    // FIXME: these functions need to be const-callable. For now, const_cast.
+    std::vector<llvm::unique_function<void(std::vector<std::string> &)>> Edits;
+  } CompileFlags;
+};
+
+/// Returns the Config of the current Context, or an empty configuration.
+const Config &config();
+
+} // namespace clangd
+} // namespace clang
+
+#endif

clang-tools-extra/clangd/Config.cpp

+//===--- Config.cpp - User configuration of clangd behavior ---------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Config.h"
+#include "support/Context.h"
+
+namespace clang {
+namespace clangd {
+
+Key<Config> Config::Key;
+
+const Config &config() {
+  if (const Config *C = Context::current().get(Config::Key))
+    return *C;
+  static Config Default;
+  return Default;
+}
+
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp

 //===-- CompileCommandsTests.cpp ------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CompileCommands.h"
+#include "Config.h"
 #include "TestFS.h"
+#include "support/Context.h"
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 
 #include "gmock/gmock.h"
TEST(CommandMangler, ClangPathResolve) {
 
   // Symlink not resolved with -no-canonical-prefixes.
   Cmd = {"foo", "-no-canonical-prefixes", "foo.cc"};
   Mangler.adjust(Cmd);
   EXPECT_EQ((TempDir + "/bin/foo").str(), Cmd.front());
 }
 #endif
 
+TEST(CommandMangler, ConfigEdits) {
+  auto Mangler = CommandMangler::forTests();
+  std::vector<std::string> Cmd = {"clang++", "foo.cc"};
+  {
+    Config Cfg;
+    Cfg.CompileFlags.Edits.push_back([](std::vector<std::string> &Argv) {
+      for (auto &Arg : Argv)
+        for (char &C : Arg)
+          C = llvm::toUpper(C);
+    });
+    Cfg.CompileFlags.Edits.push_back(
+        [](std::vector<std::string> &Argv) { Argv.push_back("--hello"); });
+    WithContextValue WithConfig(Config::Key, std::move(Cfg));
+    Mangler.adjust(Cmd);
+  }
+  // Edits are applied in given order and before other mangling.
+  EXPECT_THAT(Cmd,
+              ElementsAre("CLANG++", "FOO.CC", "--hello", "-fsyntax-only"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang