Reland "[clangd] Add feature modules registry" (llvm#154836)

Reland llvm#153756

LLVM_INSTANTIATE_REGISTRY(..) and FeatureModuleRegistry::entries() were
in different translation units, that rises a warning when compiling with
clang and leads to compilation failure if -Werror flag is set.
Instead of iterating directly in the main function, a static method
FeatureModuleSet::fromRegistry() was added and it is called in the main
function.

Author: ArcsinX

clang-tools-extra/clangd/FeatureModule.cpp

@@ -22,6 +22,10 @@ FeatureModule::Facilities &FeatureModule::facilities() {
   return *Fac;
 }
 
+void FeatureModuleSet::add(std::unique_ptr<FeatureModule> M) {
+  Modules.push_back(std::move(M));
+}
+
 bool FeatureModuleSet::addImpl(void *Key, std::unique_ptr<FeatureModule> M,
                                const char *Source) {
   if (!Map.try_emplace(Key, M.get()).second) {
@@ -33,5 +37,16 @@ bool FeatureModuleSet::addImpl(void *Key, std::unique_ptr<FeatureModule> M,
   return true;
 }
 
+FeatureModuleSet FeatureModuleSet::fromRegistry() {
+  FeatureModuleSet ModuleSet;
+  for (FeatureModuleRegistry::entry E : FeatureModuleRegistry::entries()) {
+    vlog("Adding feature module '{0}' ({1})", E.getName(), E.getDesc());
+    ModuleSet.add(E.instantiate());
+  }
+  return ModuleSet;
+}
+
 } // namespace clangd
 } // namespace clang
+
+LLVM_INSTANTIATE_REGISTRY(clang::clangd::FeatureModuleRegistry)

clang-tools-extra/clangd/FeatureModule.h

@@ -15,6 +15,7 @@
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/Registry.h"
 #include <memory>
 #include <optional>
 #include <type_traits>
@@ -143,9 +144,14 @@ class FeatureModule {
 
 /// A FeatureModuleSet is a collection of feature modules installed in clangd.
 ///
-/// Modules can be looked up by type, or used via the FeatureModule interface.
-/// This allows individual modules to expose a public API.
-/// For this reason, there can be only one feature module of each type.
+/// Modules added with explicit type specification can be looked up by type, or
+/// used via the FeatureModule interface. This allows individual modules to
+/// expose a public API. For this reason, there can be only one feature module
+/// of each type.
+///
+/// Modules added using a base class pointer can be used only via the
+/// FeatureModule interface and can't be looked up by type, thus custom public
+/// API (if provided by the module) can't be used.
 ///
 /// The set owns the modules. It is itself owned by main, not ClangdServer.
 class FeatureModuleSet {
@@ -164,6 +170,8 @@ class FeatureModuleSet {
 public:
   FeatureModuleSet() = default;
 
+  static FeatureModuleSet fromRegistry();
+
   using iterator = llvm::pointee_iterator<decltype(Modules)::iterator>;
   using const_iterator =
       llvm::pointee_iterator<decltype(Modules)::const_iterator>;
@@ -172,6 +180,7 @@ class FeatureModuleSet {
   const_iterator begin() const { return const_iterator(Modules.begin()); }
   const_iterator end() const { return const_iterator(Modules.end()); }
 
+  void add(std::unique_ptr<FeatureModule> M);
   template <typename Mod> bool add(std::unique_ptr<Mod> M) {
     return addImpl(&ID<Mod>::Key, std::move(M), LLVM_PRETTY_FUNCTION);
   }
@@ -185,6 +194,13 @@ class FeatureModuleSet {
 
 template <typename Mod> int FeatureModuleSet::ID<Mod>::Key;
 
+using FeatureModuleRegistry = llvm::Registry<FeatureModule>;
+
 } // namespace clangd
 } // namespace clang
+
+namespace llvm {
+extern template class Registry<clang::clangd::FeatureModule>;
+} // namespace llvm
+
 #endif

clang-tools-extra/clangd/tool/ClangdMain.cpp

@@ -13,6 +13,7 @@
 #include "Config.h"
 #include "ConfigProvider.h"
 #include "Feature.h"
+#include "FeatureModule.h"
 #include "IncludeCleaner.h"
 #include "PathMapping.h"
 #include "Protocol.h"
@@ -1017,6 +1018,10 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
                : static_cast<int>(ErrorResultCode::CheckFailed);
   }
 
+  FeatureModuleSet ModuleSet = FeatureModuleSet::fromRegistry();
+  if (ModuleSet.begin() != ModuleSet.end())
+    Opts.FeatureModules = &ModuleSet;
+
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();

clang-tools-extra/clangd/unittests/CMakeLists.txt

@@ -54,6 +54,7 @@ add_unittest(ClangdUnitTests ClangdTests
   DumpASTTests.cpp
   ExpectedTypeTest.cpp
   FeatureModulesTests.cpp
+  FeatureModulesRegistryTests.cpp
   FileDistanceTests.cpp
   FileIndexTests.cpp
   FindSymbolsTests.cpp

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

@@ -0,0 +1,92 @@
+//===--- FeatureModulesRegistryTests.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 "FeatureModule.h"
+#include "refactor/Tweak.h"
+#include "support/Logger.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using testing::ElementsAre;
+
+namespace llvm {
+raw_ostream &operator<<(raw_ostream &OS,
+                        const clang::clangd::FeatureModuleRegistry::entry &E) {
+  OS << "(name = " << E.getName() << ", description = '" << E.getDesc() << "')";
+  return OS;
+}
+
+raw_ostream &operator<<(
+    raw_ostream &OS,
+    const iterator_range<Registry<clang::clangd::FeatureModule>::iterator>
+        &Rng) {
+  OS << "{ ";
+  bool First = true;
+  for (clang::clangd::FeatureModuleRegistry::entry E : Rng) {
+    if (First)
+      First = false;
+    else
+      OS << ", ";
+    OS << E;
+  }
+  OS << " }";
+  return OS;
+}
+
+raw_ostream &operator<<(raw_ostream &OS, const clang::clangd::Tweak &T) {
+  OS << "(id = " << T.id() << ", "
+     << "title = " << T.title() << ")";
+  return OS;
+}
+} // namespace llvm
+
+namespace clang::clangd {
+namespace {
+
+class Dummy final : public FeatureModule {
+  static constexpr const char *TweakID = "DummyTweak";
+  struct DummyTweak final : public Tweak {
+    const char *id() const override { return TweakID; }
+    bool prepare(const Selection &) override { return true; }
+    Expected<Effect> apply(const Selection &) override {
+      return error("not implemented");
+    }
+    std::string title() const override { return id(); }
+    llvm::StringLiteral kind() const override {
+      return llvm::StringLiteral("");
+    };
+  };
+
+  void contributeTweaks(std::vector<std::unique_ptr<Tweak>> &Out) override {
+    Out.emplace_back(new DummyTweak);
+  }
+};
+
+static FeatureModuleRegistry::Add<Dummy>
+    X("dummy", "Dummy feature module with dummy tweak");
+
+MATCHER_P(moduleName, Name, "") { return arg.getName() == Name; }
+MATCHER_P(tweakID, ID, "") { return arg->id() == llvm::StringRef(ID); }
+
+// In this test, it is assumed that for unittests executable, all feature
+// modules are added to the registry only here (in this file). To implement
+// modules for clangd tool, one need to link them directly to the clangd
+// executable in clangd/tool/CMakeLists.txt.
+TEST(FeatureModulesRegistryTest, DummyModule) {
+  EXPECT_THAT(FeatureModuleRegistry::entries(),
+              ElementsAre(moduleName("dummy")));
+  FeatureModuleSet Set = FeatureModuleSet::fromRegistry();
+  ASSERT_EQ(Set.end() - Set.begin(), 1u);
+  std::vector<std::unique_ptr<Tweak>> Tweaks;
+  Set.begin()->contributeTweaks(Tweaks);
+  EXPECT_THAT(Tweaks, ElementsAre(tweakID("DummyTweak")));
+}
+
+} // namespace
+} // namespace clang::clangd