[clangd] add a config knob to disable modules

[sammccall]

Summary

We don't yet have any intentional modules support:
https://github.com/clangd/clangd/issues/1293

However if the user has modules-related flags in their CDB, modules will still
be enabled inside clang.

• for some configurations this works well enough, apparently. We should allow them to keep using this config if they wish, though unsupported.
• for other configurations this is crashy/buggy. We can't really control the behavior/stability, so long-term this should not be the default.
• there is no flag to disable modules if when using -std=c++20. Since clangd does not support modules, we should provide a mechanism.
• in future we're likely to explicitly add modules support. This will not be a simple passthrough of flags to clang, so we'll need another mode.

This patch adds a config option:

CompileFlags:
  NaiveModules: false

For now, the default is true, and modules are internally enabled/disabled
according to compile flags. When set to false, we force Modules/CPlusPlusModules
lang opts off.

[sammccall]

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

497

For existing tests of how modules interacts with other features, testing these with modules-enabled seems sufficient.

For tests that were specifically about modules, I think it makes sense to test in both modes (behavior may differ).

[kadircet]

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

34

testing behaviour indirectly through config options cause some troubles in the long term (e.g. when we want to change the default behaviour a bunch of tests observe a different behaviour all of a sudden, or when we want to override behaviour at different layers it's hard to compose as we can't overlay configs but only override them)

so can we have this as a ParseInput::ParseOption instead? as discussed we can set it in ASTWorker, without doing the extra IO.

(you've got more context as we discussed this offline, there's also a solution that provides tests with the ability to have a different default "test" config and flips bits on it, but that's a little bit more involved so no need to block this patch on it)

clang-tools-extra/clangd/Compiler.cpp

 //===--- Compiler.cpp --------------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "Compiler.h"
+#include "Config.h"
 #include "support/Logger.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/PCHContainerOperations.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
void disableUnsupportedOptions(CompilerInvocation &CI) {
   // These options mostly affect codegen, and aren't relevant to clangd. And
   // clang will die immediately when these files are not existed.
   // Disable these uninteresting options to make clangd more robust.
   CI.getLangOpts()->NoSanitizeFiles.clear();
   CI.getLangOpts()->XRayAttrListFiles.clear();
   CI.getLangOpts()->ProfileListFiles.clear();
   CI.getLangOpts()->XRayAlwaysInstrumentFiles.clear();
   CI.getLangOpts()->XRayNeverInstrumentFiles.clear();
+
+  if (!Config::current().CompileFlags.NaiveModules) {
+    CI.getLangOpts()->Modules = false;
+    CI.getLangOpts()->CPlusPlusModules = false;
+  }
 }
 
 std::unique_ptr<CompilerInvocation>
 buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,
                         std::vector<std::string> *CC1Args) {
   llvm::ArrayRef<std::string> Argv = Inputs.CompileCommand.CommandLine;
   if (Argv.empty())
     return nullptr;

clang-tools-extra/clangd/Config.h

struct Config {
 
   /// Controls how the compile command for the current file is determined.
   struct {
     /// Edits to apply to the compile command, in sequence.
     std::vector<llvm::unique_function<void(std::vector<std::string> &) const>>
         Edits;
     /// Where to search for compilation databases for this file's flags.
     CDBSearchSpec CDBSearch = {CDBSearchSpec::Ancestors, std::nullopt};
+    /// Allow modules when enabled by -fmodules, -std=c++20, etc.
+    bool NaiveModules = true;
   } CompileFlags;
 
   enum class BackgroundPolicy { Build, Skip };
   /// Describes an external index configuration.
   struct ExternalIndexSpec {
     enum { None, File, Server } Kind = None;
     /// This is one of:
     /// - Address of a clangd-index-server, in the form of "ip:port".

clang-tools-extra/clangd/ConfigCompile.cpp

if (F.CompilationDatabase) {
         }
       }
       if (Spec)
         Out.Apply.push_back(
             [Spec(std::move(*Spec))](const Params &, Config &C) {
               C.CompileFlags.CDBSearch = Spec;
             });
     }
+
+    if (F.NaiveModules)
+      Out.Apply.push_back(
+          [NaiveModules(**F.NaiveModules)](const Params &, Config &C) {
+            C.CompileFlags.NaiveModules = NaiveModules;
+          });
   }
 
   void compile(Fragment::IndexBlock &&F) {
     if (F.Background) {
       if (auto Val = compileEnum<Config::BackgroundPolicy>("Background",
                                                            **F.Background)
                          .map("Build", Config::BackgroundPolicy::Build)
                          .map("Skip", Config::BackgroundPolicy::Skip)

clang-tools-extra/clangd/ConfigFragment.h

struct CompileFlagsBlock {
     std::vector<Located<std::string>> Remove;
 
     /// Directory to search for compilation database (compile_commands.json
     /// etc). Valid values are:
     /// - A single path to a directory (absolute, or relative to the fragment)
     /// - Ancestors: search all parent directories (the default)
     /// - std::nullopt: do not use a compilation database, just default flags.
     std::optional<Located<std::string>> CompilationDatabase;
+
+    /// Allow flags such as -fmodules or -std=c++20 to enable modules.
+    /// We don't support modules: https://github.com/clangd/clangd/issues/1293
+    /// However some configurations may happen to work anyway.
+    std::optional<Located<bool>> NaiveModules;
   };
   CompileFlagsBlock CompileFlags;
 
   /// Controls how clangd understands code outside the current file.
   /// clangd's indexes provide information about symbols that isn't available
   /// to clang's parser, such as incoming references.
   struct IndexBlock {
     /// Whether files are built in the background to produce a project index.

clang-tools-extra/clangd/ConfigYAML.cpp

void parse(Fragment::CompileFlagsBlock &F, Node &N) {
     });
     Dict.handle("Remove", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.Remove = std::move(*Values);
     });
     Dict.handle("CompilationDatabase", [&](Node &N) {
       F.CompilationDatabase = scalarValue(N, "CompilationDatabase");
     });
+    Dict.handle("NaiveModules", [&](Node &N) {
+      F.NaiveModules = boolValue(N, "NaiveModules");
+    });
     Dict.parse(N);
   }
 
   void parse(Fragment::StyleBlock &F, Node &N) {
     DictParser Dict("Style", this);
     Dict.handle("FullyQualifiedNamespaces", [&](Node &N) {
       if (auto Values = scalarValues(N))
         F.FullyQualifiedNamespaces = std::move(*Values);

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

 //===-- FindSymbolsTests.cpp -------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 #include "Annotations.h"
+#include "Config.h"
 #include "FindSymbols.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "support/Context.h"
 #include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <utility>
 
 namespace clang {
 namespace clangd {
 
 namespace {
 
 using ::testing::AllOf;
 using ::testing::ElementsAre;
TEST(DocumentSymbols, ExternContext) {
 
   EXPECT_THAT(getSymbols(TU.build()),
               ElementsAre(withName("foo"), withName("Foo"),
                           AllOf(withName("ns"),
                                 children(withName("bar"), withName("Bar")))));
 }
 
 TEST(DocumentSymbols, ExportContext) {
+  Config EnableModules;
+  EnableModules.CompileFlags.NaiveModules = true;
+  WithContextValue WithCfg(Config::Key, std::move(EnableModules));
+
   TestTU TU;
   TU.ExtraArgs = {"-std=c++20"};
   TU.Code = R"cpp(
       export module test;
       export {
       void foo();
       class Foo {};
       })cpp";

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

 //===-- ModulesTests.cpp  ---------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
+#include "Config.h"
 #include "TestFS.h"
 #include "TestTU.h"
+#include "support/Context.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include <memory>
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
-TEST(Modules, TextualIncludeInPreamble) {
+enum class ModulesMode { Disabled, Naive };
+Config makeModuleConfig(ModulesMode M) {
+  Config EnableModules;
+  EnableModules.CompileFlags.NaiveModules = M == ModulesMode::Naive;
+  return EnableModules;
+}
+
+class ModulesTest : public ::testing::TestWithParam<ModulesMode> {
+  WithContextValue WithCfg;
+
+protected:
+  ModulesTest() : WithCfg(Config::Key, makeModuleConfig(GetParam())) {}
+};
+
+TEST_P(ModulesTest, TextualIncludeInPreamble) {
   TestTU TU = TestTU::withCode(R"cpp(
     #include "Textual.h"
 
     void foo() {}
 )cpp");
   TU.ExtraArgs.push_back("-fmodule-name=M");
   TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
   TU.AdditionalFiles["Textual.h"] = "void foo();";
   TU.AdditionalFiles["m.modulemap"] = R"modulemap(
     module M {
       module Textual {
         textual header "Textual.h"
       }
     }
 )modulemap";
   // Test that we do not crash.
   TU.index();
 }
 
 // Verify that visibility of AST nodes belonging to modules, but loaded from
 // preamble PCH, is restored.
-TEST(Modules, PreambleBuildVisibility) {
+TEST_P(ModulesTest, PreambleBuildVisibility) {
   TestTU TU = TestTU::withCode(R"cpp(
     #include "module.h"
 
     foo x;
 )cpp");
   TU.OverlayRealFileSystemForModules = true;
   TU.ExtraArgs.push_back("-fmodules");
   TU.ExtraArgs.push_back("-fmodules-strict-decluse");
   TU.ExtraArgs.push_back("-Xclang");
   TU.ExtraArgs.push_back("-fmodules-local-submodule-visibility");
   TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
   TU.AdditionalFiles["module.h"] = R"cpp(
     typedef int foo;
 )cpp";
   TU.AdditionalFiles["m.modulemap"] = R"modulemap(
     module M {
       header "module.h"
     }
 )modulemap";
   EXPECT_TRUE(TU.build().getDiagnostics().empty());
 }
 
-TEST(Modules, Diagnostic) {
+TEST_P(ModulesTest, Diagnostic) {
   // Produce a diagnostic while building an implicit module. Use
   // -fmodules-strict-decluse, but any non-silenced diagnostic will do.
   TestTU TU = TestTU::withCode(R"cpp(
     /*error-ok*/
     #include "modular.h"
 
     void bar() {}
 )cpp");
TU.AdditionalFiles["m.modulemap"] = R"modulemap(
     }
 )modulemap";
 
   // Test that we do not crash.
   TU.build();
 }
 
 // Unknown module formats are a fatal failure for clang. Ensure we don't crash.
-TEST(Modules, UnknownFormat) {
+TEST_P(ModulesTest, UnknownFormat) {
   TestTU TU = TestTU::withCode(R"(#include "modular.h")");
   TU.OverlayRealFileSystemForModules = true;
   TU.ExtraArgs.push_back("-Xclang");
   TU.ExtraArgs.push_back("-fmodule-format=obj");
   TU.ExtraArgs.push_back("-fmodule-map-file=" + testPath("m.modulemap"));
   TU.ExtraArgs.push_back("-fmodules");
   TU.ExtraArgs.push_back("-fimplicit-modules");
   TU.AdditionalFiles["modular.h"] = "";
   TU.AdditionalFiles["m.modulemap"] = R"modulemap(
     module M {
       header "modular.h"
     })modulemap";
 
   // Test that we do not crash.
   TU.build();
 }
+
+MATCHER_P(diag, M, "") { return arg.Message == M; }
+
+TEST_P(ModulesTest, Cpp20Import) {
+  TestTU TU = TestTU::withCode("import foo; // error-ok");
+  TU.ExtraArgs.push_back("-Xclang");
+  TU.ExtraArgs.push_back("-std=c++20");
+
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getDiagnostics(),
+              testing::ElementsAre(GetParam() == ModulesMode::Naive
+                                       ? diag("module 'foo' not found")
+                                       : diag("unknown type name 'import'")));
+}
+
+INSTANTIATE_TEST_SUITE_P(ModulesTestModes, ModulesTest,
+                         testing::Values(ModulesMode::Disabled,
+                                         ModulesMode::Naive));
+
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 //===-- SymbolCollectorTests.cpp  -------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
+#include "Config.h"
 #include "TestFS.h"
 #include "TestTU.h"
 #include "index/SymbolCollector.h"
+#include "support/Context.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Index/IndexingOptions.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include <memory>
 #include <optional>
 #include <string>
+#include <utility>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::_;
 using ::testing::AllOf;
 using ::testing::Contains;
TEST_F(SymbolCollectorTest, MacrosInHeaders) {
   TestFileName = testPath("test.h");
   runSymbolCollector("", "#define X");
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(AllOf(qName("X"), forCodeCompletion(true))));
 }
 
 // Regression test for a crash-bug we used to have.
 TEST_F(SymbolCollectorTest, UndefOfModuleMacro) {
+  Config EnableModules;
+  EnableModules.CompileFlags.NaiveModules = true;
+  WithContextValue WithCfg(Config::Key, std::move(EnableModules));
+
   auto TU = TestTU::withCode(R"cpp(#include "bar.h")cpp");
   TU.AdditionalFiles["bar.h"] = R"cpp(
     #include "foo.h"
     #undef X
     )cpp";
   TU.AdditionalFiles["foo.h"] = "#define X 1";
   TU.AdditionalFiles["module.map"] = R"cpp(
     module foo {