[clangd] Get rid of arg adjusters in CommandMangler

[kadircet]

• Also get rid of the insertion adjuster

[sammccall]

This raises the question "is it safe to move functionality from CommandMangler to buildCompilerInvocation for convenience, are they always called on the same codepaths?"

I think from our brief investigation the answer is yes, but:

• this expectation should probably be documented on CommandMangler and/or buildCompilerInvocation
• *neither* of them are called when background indexing, which should probably be a fixme on the static indexer

clang-tools-extra/clangd/CompileCommands.cpp

226

oops, might have left this comment on the wrong patch

use range insert to save shuffling the tail over and over and std::make_move_iterator() to avoid copies?

[kadircet]

Use move_iterator instead of inserting in a loop.

clang-tools-extra/clangd/CompileCommands.cpp

226

yeah the main reason this is a loop is because I didn't know about make_move_iterator, switching to a range insert instead, thanks!

[kadircet]

• Add FIXME for using CommandMangler and buildCompilerInvocation in clangd-indexer.

clang-tools-extra/clangd/CompileCommands.h

 namespace clangd {
 
 // CommandMangler transforms compile commands from some external source
 // for use in clangd. This means:
 //  - running the frontend only, stripping args regarding output files etc
 //  - forcing the use of clangd's builtin headers rather than clang's
 //  - resolving argv0 as cc1 expects
 //  - injecting -isysroot flags on mac as the system clang does
+// FIXME: This is currently not used in all code paths that create invocations.
+// Make use of these adjusters and buildCompilerInvocation in clangd-indexer as
+// well. It should be possible to hook it up by overriding RunInvocation in
+// FrontendActionFactory.
 struct CommandMangler {
   // Absolute path to clang.
   llvm::Optional<std::string> ClangPath;
   // Directory containing builtin headers.
   llvm::Optional<std::string> ResourceDir;
   // Root for searching for standard library (passed to -isysroot).
   llvm::Optional<std::string> Sysroot;
 

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/Driver/Options.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Debug.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"
+#include <iterator>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Query apple's `xcrun` launcher, which is the source of truth for "how should"
void CommandMangler::adjust(std::vector<std::string> &Cmd) const {
   auto Has = [&](llvm::StringRef Flag) {
     for (llvm::StringRef Arg : Cmd) {
       if (Arg.consume_front(Flag) && (Arg.empty() || Arg[0] == '='))
         return true;
     }
     return false;
   };
 
-  // clangd should not write files to disk, including dependency files
-  // requested on the command line.
-  Cmd = tooling::getClangStripDependencyFileAdjuster()(Cmd, "");
-  // Strip plugin related command line arguments. Clangd does
-  // not support plugins currently. Therefore it breaks if
-  // compiler tries to load plugins.
-  Cmd = tooling::getStripPluginsAdjuster()(Cmd, "");
-  Cmd = tooling::getClangSyntaxOnlyAdjuster()(Cmd, "");
+  llvm::erase_if(Cmd, [](llvm::StringRef Elem) {
+    return Elem.startswith("--save-temps") || Elem.startswith("-save-temps");
+  });
 
   std::vector<std::string> ToAppend;
   if (ResourceDir && !Has("-resource-dir"))
     ToAppend.push_back(("-resource-dir=" + *ResourceDir));
 
   // Don't set `-isysroot` if it is already set or if `--sysroot` is set.
   // `--sysroot` is a superset of the `-isysroot` argument.
   if (Sysroot && !Has("-isysroot") && !Has("--sysroot")) {
     ToAppend.push_back("-isysroot");
     ToAppend.push_back(*Sysroot);
   }
 
   if (!ToAppend.empty()) {
-    Cmd = tooling::getInsertArgumentAdjuster(
-        std::move(ToAppend), tooling::ArgumentInsertPosition::END)(Cmd, "");
+    Cmd.insert(llvm::find(Cmd, "--"), std::make_move_iterator(ToAppend.begin()),
+               std::make_move_iterator(ToAppend.end()));
   }
 
   if (!Cmd.empty()) {
     bool FollowSymlink = !Has("-no-canonical-prefixes");
     Cmd.front() =
         (FollowSymlink ? ResolvedDrivers : ResolvedDriversNoFollow)
             .get(Cmd.front(), [&, this] {
               return resolveDriver(Cmd.front(), FollowSymlink, ClangPath);
             });

clang-tools-extra/clangd/Compiler.cpp

buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,
   // createInvocationFromCommandLine sets DisableFree.
   CI->getFrontendOpts().DisableFree = false;
   CI->getLangOpts()->CommentOpts.ParseAllComments = true;
   CI->getLangOpts()->RetainCommentsFromSystemHeaders = true;
   // Disable "clang -verify" diagnostics, they are rarely useful in clangd, and
   // our compiler invocation set-up doesn't seem to work with it (leading
   // assertions in VerifyDiagnosticConsumer).
   CI->getDiagnosticOpts().VerifyDiagnostics = false;
+  CI->getDiagnosticOpts().ShowColors = false;
 
   // Disable any dependency outputting, we don't want to generate files or write
   // to stdout/stderr.
   CI->getDependencyOutputOpts().ShowIncludesDest =
       ShowIncludesDestination::None;
   CI->getDependencyOutputOpts().OutputFile.clear();
   CI->getDependencyOutputOpts().HeaderIncludeOutputFile.clear();
   CI->getDependencyOutputOpts().DOTOutputFile.clear();
buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,
   // Don't crash on `#pragma clang __debug parser_crash`
   CI->getPreprocessorOpts().DisablePragmaDebugCrash = true;
 
   // Always default to raw container format as clangd doesn't registry any other
   // and clang dies when faced with unknown formats.
   CI->getHeaderSearchOpts().ModuleFormat =
       PCHContainerOperations().getRawReader().getFormat().str();
 
+  CI->getFrontendOpts().Plugins.clear();
+  CI->getFrontendOpts().AddPluginActions.clear();
+  CI->getFrontendOpts().PluginArgs.clear();
+  CI->getFrontendOpts().ProgramAction = frontend::ParseSyntaxOnly;
+  CI->getFrontendOpts().ActionName.clear();
+
   return CI;
 }
 
 std::unique_ptr<CompilerInstance>
 prepareCompilerInstance(std::unique_ptr<clang::CompilerInvocation> CI,
                         const PrecompiledPreamble *Preamble,
                         std::unique_ptr<llvm::MemoryBuffer> Buffer,
                         llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,

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

 
 // Make use of all features and assert the exact command we get out.
 // Other tests just verify presence/absence of certain args.
 TEST(CommandMangler, Everything) {
   auto Mangler = CommandMangler::forTests();
   Mangler.ClangPath = testPath("fake/clang");
   Mangler.ResourceDir = testPath("fake/resources");
   Mangler.Sysroot = testPath("fake/sysroot");
-  std::vector<std::string> Cmd = {"clang++", "-Xclang", "-load",
-                                  "-Xclang", "plugin",  "-MF",
-                                  "dep",     "--",      "foo.cc"};
+  std::vector<std::string> Cmd = {"clang++", "--", "foo.cc"};
   Mangler.adjust(Cmd);
-  EXPECT_THAT(Cmd, ElementsAre(testPath("fake/clang++"), "-fsyntax-only",
+  EXPECT_THAT(Cmd, ElementsAre(testPath("fake/clang++"),
                                "-resource-dir=" + testPath("fake/resources"),
                                "-isysroot", testPath("fake/sysroot"), "--",
                                "foo.cc"));
 }
 
 TEST(CommandMangler, ResourceDir) {
   auto Mangler = CommandMangler::forTests();
   Mangler.ResourceDir = testPath("fake/resources");
   std::vector<std::string> Cmd = {"clang++", "foo.cc"};
   Mangler.adjust(Cmd);
   EXPECT_THAT(Cmd, Contains("-resource-dir=" + testPath("fake/resources")));
 }
 
 TEST(CommandMangler, Sysroot) {
   auto Mangler = CommandMangler::forTests();
   Mangler.Sysroot = testPath("fake/sysroot");
 
   std::vector<std::string> Cmd = {"clang++", "foo.cc"};
   Mangler.adjust(Cmd);
   EXPECT_THAT(llvm::join(Cmd, " "),
               HasSubstr("-isysroot " + testPath("fake/sysroot")));
 }
 
-TEST(CommandMangler, StripPlugins) {
-  auto Mangler = CommandMangler::forTests();
-  std::vector<std::string> Cmd = {"clang++", "-Xclang", "-load",
-                                  "-Xclang", "plugin",  "foo.cc"};
-  Mangler.adjust(Cmd);
-  for (const char* Stripped : {"-Xclang", "-load", "plugin"})
-    EXPECT_THAT(Cmd, Not(Contains(Stripped)));
-}
-
-TEST(CommandMangler, StripOutput) {
-  auto Mangler = CommandMangler::forTests();
-  std::vector<std::string> Cmd = {"clang++", "-MF", "dependency", "-c",
-                                  "foo.cc"};
-  Mangler.adjust(Cmd);
-  for (const char* Stripped : {"-MF", "dependency"})
-    EXPECT_THAT(Cmd, Not(Contains(Stripped)));
-}
-
-TEST(CommandMangler, StripShowIncludes) {
-  auto Mangler = CommandMangler::forTests();
-  std::vector<std::string> Cmd = {"clang-cl", "/showIncludes", "foo.cc"};
-  Mangler.adjust(Cmd);
-  EXPECT_THAT(Cmd, Not(Contains("/showIncludes")));
-}
-
-TEST(CommandMangler, StripShowIncludesUser) {
-  auto Mangler = CommandMangler::forTests();
-  std::vector<std::string> Cmd = {"clang-cl", "/showIncludes:user", "foo.cc"};
-  Mangler.adjust(Cmd);
-  EXPECT_THAT(Cmd, Not(Contains("/showIncludes:user")));
-}
-
 TEST(CommandMangler, ClangPath) {
   auto Mangler = CommandMangler::forTests();
   Mangler.ClangPath = testPath("fake/clang");
 
   std::vector<std::string> Cmd = {"clang++", "foo.cc"};
   Mangler.adjust(Cmd);
   EXPECT_EQ(testPath("fake/clang++"), Cmd.front());
 
Cfg.CompileFlags.Edits.push_back([](std::vector<std::string> &Argv) {
           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(_, "FOO.CC", "--hello", "-fsyntax-only"));
+  EXPECT_THAT(Cmd, ElementsAre(_, "FOO.CC", "--hello"));
 }
 
 static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
   llvm::SmallVector<llvm::StringRef> Parts;
   llvm::SplitString(Argv, Parts);
   std::vector<std::string> Args = {Parts.begin(), Parts.end()};
   ArgStripper S;
   S.strip(Arg);

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

 //===-- CompilerTests.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 "Compiler.h"
 #include "TestTU.h"
+#include "clang/Frontend/DependencyOutputOptions.h"
+#include "clang/Frontend/FrontendOptions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
EXPECT_THAT(buildCompilerInvocation(TU.inputs(FS), Diags)
               IsEmpty());
 }
 
 TEST(BuildCompilerInvocation, PragmaDebugCrash) {
   TestTU TU = TestTU::withCode("#pragma clang __debug parser_crash");
   TU.build(); // no-crash
 }
 
+TEST(BuildCompilerInvocation, DropsShowIncludes) {
+  MockFS FS;
+  IgnoreDiagnostics Diags;
+  TestTU TU;
+
+  TU.ExtraArgs = {"-Xclang", "--show-includes"};
+  EXPECT_THAT(buildCompilerInvocation(TU.inputs(FS), Diags)
+                  ->getDependencyOutputOpts()
+                  .ShowIncludesDest,
+              ShowIncludesDestination::None);
+
+  TU.ExtraArgs = {"/showIncludes", "--driver-mode=cl"};
+  EXPECT_THAT(buildCompilerInvocation(TU.inputs(FS), Diags)
+                  ->getDependencyOutputOpts()
+                  .ShowIncludesDest,
+              ShowIncludesDestination::None);
+
+  TU.ExtraArgs = {"/showIncludes:user", "--driver-mode=cl"};
+  EXPECT_THAT(buildCompilerInvocation(TU.inputs(FS), Diags)
+                  ->getDependencyOutputOpts()
+                  .ShowIncludesDest,
+              ShowIncludesDestination::None);
+}
+
+TEST(BuildCompilerInvocation, DropsPlugins) {
+  MockFS FS;
+  IgnoreDiagnostics Diags;
+  TestTU TU;
+
+  TU.ExtraArgs = {"-Xclang", "-load",
+                  "-Xclang", "plugins.so",
+                  "-Xclang", "-plugin",
+                  "-Xclang", "my_plugin",
+                  "-Xclang", "-plugin-arg-my_plugin",
+                  "-Xclang", "foo=bar",
+                  "-Xclang", "-add-plugin",
+                  "-Xclang", "my_plugin2"};
+  auto &Opts = buildCompilerInvocation(TU.inputs(FS), Diags)->getFrontendOpts();
+  EXPECT_THAT(Opts.Plugins, IsEmpty());
+  EXPECT_THAT(Opts.PluginArgs, IsEmpty());
+  EXPECT_THAT(Opts.AddPluginActions, IsEmpty());
+  EXPECT_EQ(Opts.ProgramAction, frontend::ActionKind::ParseSyntaxOnly);
+  EXPECT_TRUE(Opts.ActionName.empty());
+}
 } // namespace
 } // namespace clangd
 } // namespace clang