[clangd] Strip /showIncludes in clangd compile commands

[aeubanks]

Summary

In command lines with /showIncludes, clangd would output includes to stdout, breaking clients.

Fixes https://github.com/clangd/clangd/issues/322.

[aeubanks]

I did verify that clangd on vscode now works on Windows on the LLVM codebase.

[sammccall]

Thanks!

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

82

It's useful to have this test to verify the behavior at the clangd level, but the arg adjuster is a separate library and also used in other contexts.

The unit-test for this arg-adjuster lives in clang/unittests/Tooling/ToolingTest.cpp around line 500. Could you add a test for the new behavior there, too?

[aeubanks]

Add test in ToolingTest

[aeubanks]

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

82

Done.

[kadircet]

clang/lib/Tooling/ArgumentsAdjusters.cpp

101

this can also be --show-includes

[kadircet]

clang/lib/Tooling/ArgumentsAdjusters.cpp

101

and looks like there's also /showIncludes:user :/

[aeubanks]

Handle /showIncludes:user

[aeubanks]

clang/lib/Tooling/ArgumentsAdjusters.cpp

101

Handled /showIncludes:user, thanks for catching that. I don't see --show-includes though, clang-cl and clang both don't like the option?

[kadircet]

thanks, lgtm. do you have commit access?

clang/lib/Tooling/ArgumentsAdjusters.cpp

101

ah sorry, that one is a frontend option rather than a driver one. so nvm that one.

[aeubanks]

In D78836#2003696, @kadircet wrote:

thanks, lgtm. do you have commit access?

Yep :)

Thanks for the reviews!

[thakis]

clang/lib/Tooling/ArgumentsAdjusters.cpp

101

clang-cl suports both /foo and -foo style flags. See the top of clang/include/clang/Driver/CLCompatOptions.td

You probably want to add the -showIncludes prefix here too. Ideally this probably wouldn't do its own commandline parsing here (?)

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

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());
 

clang/lib/Tooling/ArgumentsAdjusters.cpp

 
 ArgumentsAdjuster getClangStripDependencyFileAdjuster() {
   return [](const CommandLineArguments &Args, StringRef /*unused*/) {
     CommandLineArguments AdjustedArgs;
     for (size_t i = 0, e = Args.size(); i < e; ++i) {
       StringRef Arg = Args[i];
       // All dependency-file options begin with -M. These include -MM,
       // -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
-      if (!Arg.startswith("-M")) {
+      if (!Arg.startswith("-M") && !Arg.startswith("/showIncludes")) {
         AdjustedArgs.push_back(Args[i]);
         continue;
       }
 
       if (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")
         // These flags take an argument: -MX foo. Skip the next argument also.
         ++i;
     }

clang/unittests/Tooling/ToolingTest.cpp

auto HasFlag = [&FinalArgs](const std::string &Flag) {
     return llvm::find(FinalArgs, Flag) != FinalArgs.end();
   };
   EXPECT_FALSE(HasFlag("-MD"));
   EXPECT_FALSE(HasFlag("-MMD"));
   EXPECT_TRUE(HasFlag("-c"));
   EXPECT_TRUE(HasFlag("-w"));
 }
 
+// Check getClangStripDependencyFileAdjuster strips /showIncludes
+TEST(ClangToolTest, StripDependencyFileAdjusterShowIncludes) {
+  FixedCompilationDatabase Compilations("/", {"/showIncludes", "-c"});
+
+  ClangTool Tool(Compilations, std::vector<std::string>(1, "/a.cc"));
+  Tool.mapVirtualFile("/a.cc", "void a() {}");
+
+  std::unique_ptr<FrontendActionFactory> Action(
+      newFrontendActionFactory<SyntaxOnlyAction>());
+
+  CommandLineArguments FinalArgs;
+  ArgumentsAdjuster CheckFlagsAdjuster =
+      [&FinalArgs](const CommandLineArguments &Args, StringRef /*unused*/) {
+        FinalArgs = Args;
+        return Args;
+      };
+  Tool.clearArgumentsAdjusters();
+  Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
+  Tool.appendArgumentsAdjuster(CheckFlagsAdjuster);
+  Tool.run(Action.get());
+
+  auto HasFlag = [&FinalArgs](const std::string &Flag) {
+    return llvm::find(FinalArgs, Flag) != FinalArgs.end();
+  };
+  EXPECT_FALSE(HasFlag("/showIncludes"));
+  EXPECT_TRUE(HasFlag("-c"));
+}
+
+// Check getClangStripDependencyFileAdjuster strips /showIncludes:user
+TEST(ClangToolTest, StripDependencyFileAdjusterShowIncludesUser) {
+  FixedCompilationDatabase Compilations("/", {"/showIncludes:user", "-c"});
+
+  ClangTool Tool(Compilations, std::vector<std::string>(1, "/a.cc"));
+  Tool.mapVirtualFile("/a.cc", "void a() {}");
+
+  std::unique_ptr<FrontendActionFactory> Action(
+      newFrontendActionFactory<SyntaxOnlyAction>());
+
+  CommandLineArguments FinalArgs;
+  ArgumentsAdjuster CheckFlagsAdjuster =
+      [&FinalArgs](const CommandLineArguments &Args, StringRef /*unused*/) {
+        FinalArgs = Args;
+        return Args;
+      };
+  Tool.clearArgumentsAdjusters();
+  Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
+  Tool.appendArgumentsAdjuster(CheckFlagsAdjuster);
+  Tool.run(Action.get());
+
+  auto HasFlag = [&FinalArgs](const std::string &Flag) {
+    return llvm::find(FinalArgs, Flag) != FinalArgs.end();
+  };
+  EXPECT_FALSE(HasFlag("/showIncludes:user"));
+  EXPECT_TRUE(HasFlag("-c"));
+}
+
 // Check getClangStripPluginsAdjuster strips plugin related args.
 TEST(ClangToolTest, StripPluginsAdjuster) {
   FixedCompilationDatabase Compilations(
       "/", {"-Xclang", "-add-plugin", "-Xclang", "random-plugin"});
 
   ClangTool Tool(Compilations, std::vector<std::string>(1, "/a.cc"));
   Tool.mapVirtualFile("/a.cc", "void a() {}");