[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbce3ac4f224a: [clangd] Introduce ASTHooks to FeatureModules 
(authored by kadircet).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/FeatureModule.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -19,12 +19,14 @@
 
 #include "../TidyProvider.h"
 #include "Compiler.h"
+#include "FeatureModule.h"
 #include "ParsedAST.h"
 #include "TestFS.h"
 #include "index/Index.h"
 #include "support/Path.h"
 #include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 #include 
 #include 
@@ -76,6 +78,8 @@
   // to eliminate this option some day.
   bool OverlayRealFileSystemForModules = false;
 
+  FeatureModuleSet *FeatureModules = nullptr;
+
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   // The result will always have getDiagnostics() populated.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -36,6 +36,7 @@
   FS.Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
+  Inputs.FeatureModules = FeatureModules;
   auto  = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "Config.h"
 #include "Diagnostics.h"
+#include "FeatureModule.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
@@ -26,6 +27,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1346,6 +1348,31 @@
  });
 }
 
+TEST(ParsedASTTest, ModuleSawDiag) {
+  static constexpr const llvm::StringLiteral KDiagMsg = "StampedDiag";
+  struct DiagModifierModule final : public FeatureModule {
+struct Listener : public FeatureModule::ASTListener {
+  void sawDiagnostic(const clang::Diagnostic ,
+ clangd::Diag ) override {
+Diag.Message = KDiagMsg.str();
+  }
+};
+std::unique_ptr astListeners() override {
+  return std::make_unique();
+};
+  };
+  FeatureModuleSet FMS;
+  FMS.add(std::make_unique());
+
+  Annotations Code("[[test]]; /* error-ok */");
+  TestTU TU;
+  TU.Code = Code.code().str();
+  TU.FeatureModules = 
+
+  auto AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+  testing::Contains(Diag(Code.range(), KDiagMsg.str(;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -365,6 +365,27 @@
   // And immediately shut down. FeatureModule destructor verifies we blocked.
 }
 
+TEST_F(LSPTest, DiagModuleTest) {
+  static constexpr llvm::StringLiteral DiagMsg = "DiagMsg";
+  class DiagModule final : public FeatureModule {
+struct DiagHooks : public ASTListener {
+  void sawDiagnostic(const clang::Diagnostic &, clangd::Diag ) override {
+D.Message = DiagMsg.str();
+  }
+};
+
+  public:
+std::unique_ptr astListeners() override {
+  return std::make_unique();
+}
+  };
+  FeatureModules.add(std::make_unique());
+
+  auto  = start();
+  Client.didOpen("foo.cpp", "test;");
+  EXPECT_THAT(Client.diagnostics("foo.cpp"),
+  llvm::ValueIs(testing::ElementsAre(DiagMessage(DiagMsg;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- 

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 337168.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Inline helper to call-sites
- Add comments about destruction of ast listeners.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/FeatureModule.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -19,12 +19,14 @@
 
 #include "../TidyProvider.h"
 #include "Compiler.h"
+#include "FeatureModule.h"
 #include "ParsedAST.h"
 #include "TestFS.h"
 #include "index/Index.h"
 #include "support/Path.h"
 #include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 #include 
 #include 
@@ -76,6 +78,8 @@
   // to eliminate this option some day.
   bool OverlayRealFileSystemForModules = false;
 
+  FeatureModuleSet *FeatureModules = nullptr;
+
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   // The result will always have getDiagnostics() populated.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -36,6 +36,7 @@
   FS.Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
+  Inputs.FeatureModules = FeatureModules;
   auto  = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "Config.h"
 #include "Diagnostics.h"
+#include "FeatureModule.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
@@ -26,6 +27,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1346,6 +1348,31 @@
  });
 }
 
+TEST(ParsedASTTest, ModuleSawDiag) {
+  static constexpr const llvm::StringLiteral KDiagMsg = "StampedDiag";
+  struct DiagModifierModule final : public FeatureModule {
+struct Listener : public FeatureModule::ASTListener {
+  void sawDiagnostic(const clang::Diagnostic ,
+ clangd::Diag ) override {
+Diag.Message = KDiagMsg.str();
+  }
+};
+std::unique_ptr astListeners() override {
+  return std::make_unique();
+};
+  };
+  FeatureModuleSet FMS;
+  FMS.add(std::make_unique());
+
+  Annotations Code("[[test]]; /* error-ok */");
+  TestTU TU;
+  TU.Code = Code.code().str();
+  TU.FeatureModules = 
+
+  auto AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+  testing::Contains(Diag(Code.range(), KDiagMsg.str(;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -365,6 +365,27 @@
   // And immediately shut down. FeatureModule destructor verifies we blocked.
 }
 
+TEST_F(LSPTest, DiagModuleTest) {
+  static constexpr llvm::StringLiteral DiagMsg = "DiagMsg";
+  class DiagModule final : public FeatureModule {
+struct DiagHooks : public ASTListener {
+  void sawDiagnostic(const clang::Diagnostic &, clangd::Diag ) override {
+D.Message = DiagMsg.str();
+  }
+};
+
+  public:
+std::unique_ptr astListeners() override {
+  return std::make_unique();
+}
+  };
+  FeatureModules.add(std::make_unique());
+
+  auto  = start();
+  Client.didOpen("foo.cpp", "test;");
+  EXPECT_THAT(Client.diagnostics("foo.cpp"),
+  llvm::ValueIs(testing::ElementsAre(DiagMessage(DiagMsg;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ 

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/FeatureModule.h:106
+  struct ASTListener {
+virtual ~ASTListener() = default;
+

comment: listeners are destroyed once the AST is built.



Comment at: clang-tools-extra/clangd/FeatureModule.h:181
 
+/// Helper to get all the ast listeners available.
+std::vector>

this seems like a slightly weird thing to have in FeatureModule.h, I'd be 
tempted to inline it in the callsites instead, but up to you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 337101.
kadircet marked 7 inline comments as done.
kadircet added a comment.

- Pass FeatureModuleSet rather than astListeners in ParseInputs.
- Create listeners only when building ASTs.
- Use a callback function in StoreDiags to notify about diagnostics.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/FeatureModule.cpp
  clang-tools-extra/clangd/FeatureModule.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -19,12 +19,14 @@
 
 #include "../TidyProvider.h"
 #include "Compiler.h"
+#include "FeatureModule.h"
 #include "ParsedAST.h"
 #include "TestFS.h"
 #include "index/Index.h"
 #include "support/Path.h"
 #include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 #include 
 #include 
@@ -76,6 +78,8 @@
   // to eliminate this option some day.
   bool OverlayRealFileSystemForModules = false;
 
+  FeatureModuleSet *FeatureModules = nullptr;
+
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   // The result will always have getDiagnostics() populated.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -36,6 +36,7 @@
   FS.Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
+  Inputs.FeatureModules = FeatureModules;
   auto  = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -9,6 +9,7 @@
 #include "Annotations.h"
 #include "Config.h"
 #include "Diagnostics.h"
+#include "FeatureModule.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
@@ -26,6 +27,7 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -1346,6 +1348,31 @@
  });
 }
 
+TEST(ParsedASTTest, ModuleSawDiag) {
+  static constexpr const llvm::StringLiteral KDiagMsg = "StampedDiag";
+  struct DiagModifierModule final : public FeatureModule {
+struct Listener : public FeatureModule::ASTListener {
+  void sawDiagnostic(const clang::Diagnostic ,
+ clangd::Diag ) override {
+Diag.Message = KDiagMsg.str();
+  }
+};
+std::unique_ptr astListeners() override {
+  return std::make_unique();
+};
+  };
+  FeatureModuleSet FMS;
+  FMS.add(std::make_unique());
+
+  Annotations Code("[[test]]; /* error-ok */");
+  TestTU TU;
+  TU.Code = Code.code().str();
+  TU.FeatureModules = 
+
+  auto AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+  testing::Contains(Diag(Code.range(), KDiagMsg.str(;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -365,6 +365,27 @@
   // And immediately shut down. FeatureModule destructor verifies we blocked.
 }
 
+TEST_F(LSPTest, DiagModuleTest) {
+  static constexpr llvm::StringLiteral DiagMsg = "DiagMsg";
+  class DiagModule final : public FeatureModule {
+struct DiagHooks : public ASTListener {
+  void sawDiagnostic(const clang::Diagnostic &, clangd::Diag ) override {
+D.Message = DiagMsg.str();
+  }
+};
+
+  public:
+std::unique_ptr astListeners() override {
+  return std::make_unique();
+}
+  };
+  FeatureModules.add(std::make_unique());
+
+  auto  = start();
+  Client.didOpen("foo.cpp", "test;");
+  EXPECT_THAT(Client.diagnostics("foo.cpp"),
+  llvm::ValueIs(testing::ElementsAre(DiagMessage(DiagMsg;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: 

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:237
+  if (auto Hook = Mod.astHooks()) {
+Inputs.PreambleHooks.emplace_back(std::move(Hook));
+Inputs.MainFileHooks.emplace_back(Mod.astHooks());

Now we're creating two ASTListeners for each version of the file seen, but most 
won't be parsed twice (and some won't even be parsed once).

This seems like it's going to be a weird wart for e.g. hooks that expect to do 
something interesting when a file finishes, or those that allocate something 
heavyweight.

I'd suggest sinking the ModuleSet into ParsedAST::build etc, or at least moving 
the astHooks() calls into TUScheduler. (And documenting that astHooks() should 
be threadsafe)

Otherwise, need to document that sometimes hooks are created and then never 
used.



Comment at: clang-tools-extra/clangd/Compiler.h:60
   TidyProviderRef ClangTidyProvider = {};
+  // Using shared_ptr since ParseInputs are copied between threads. But none of
+  // these hooks are accessed simultaneously.

I think having these in ParseInputs is conceptually confusing and shared_ptr is 
risky.
For example we copy inputs into ASTWorker::FileInputs, which extends the 
lifetime of the listeners in a way that seems undesirable.

I think these listeners should be clearly either:
 - bound to the lifetime of the ParsedAST (and therefore owned by it)
 - bound to the *creation* of the ParsedAST (and therefore strictly scoped 
within ParsedAST::build)

You could consider the **ModuleSet** to be part of ParseInputs, though...



Comment at: clang-tools-extra/clangd/Diagnostics.h:147
   void setLevelAdjuster(LevelAdjuster Adjuster) { this->Adjuster = Adjuster; }
+  void setASTHooks(
+  llvm::ArrayRef> Hooks) {

ArrayRef> seems like we're leaking a lot. And even 
mentioning AST hooks seems dubious layering-wise.
Can we use a std::function like LevelAdjuster?



Comment at: clang-tools-extra/clangd/FeatureModule.h:101
 
+  /// Various hooks that can be invoked by ASTWorker thread while building an
+  /// AST. These hooks are always called from a single thread, so

nit: this describes the interaction more but not what it's for.
Maybe 
```
/// Extension point that allows modules to observe and modify an AST build.
/// One instance is created each time clangd produces a ParsedAST or 
PrecompiledPreamble.
/// For a given instance, lifecycle methods are always called on a single 
thread.
```



Comment at: clang-tools-extra/clangd/FeatureModule.h:113
+  /// Can be called asynchronously before building an AST.
+  virtual std::unique_ptr astHooks() { return nullptr; }
+

the "hooks" name is still used here and throughout


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 334079.
kadircet added a comment.

- Have 2 separate hooks for preamble and mainfile ASTs, as they are produced 
async.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/FeatureModule.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -25,6 +25,7 @@
 #include "support/Path.h"
 #include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 #include 
 #include 
@@ -76,6 +77,8 @@
   // to eliminate this option some day.
   bool OverlayRealFileSystemForModules = false;
 
+  std::vector> Hooks;
+
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   // The result will always have getDiagnostics() populated.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -36,6 +36,8 @@
   FS.Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
+  Inputs.PreambleHooks = Hooks;
+  Inputs.MainFileHooks = Hooks;
   auto  = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
@@ -97,6 +99,7 @@
   MockFS FS;
   auto Inputs = inputs(FS);
   StoreDiags Diags;
+  Diags.setASTHooks(Inputs.MainFileHooks);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   if (OverlayRealFileSystemForModules)
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1346,6 +1346,24 @@
  });
 }
 
+TEST(ParsedASTTest, ModuleSawDiag) {
+  static constexpr const llvm::StringLiteral KDiagMsg = "StampedDiag";
+  struct Hooks : public FeatureModule::ASTListener {
+void sawDiagnostic(const clang::Diagnostic ,
+   clangd::Diag ) override {
+  Diag.Message = KDiagMsg.str();
+}
+  };
+
+  Annotations Code("[[test]]; /* error-ok */");
+  TestTU TU;
+  TU.Code = Code.code().str();
+  TU.Hooks.emplace_back(new Hooks);
+
+  auto AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+  testing::Contains(Diag(Code.range(), KDiagMsg.str(;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -365,6 +365,27 @@
   // And immediately shut down. FeatureModule destructor verifies we blocked.
 }
 
+TEST_F(LSPTest, DiagModuleTest) {
+  static constexpr llvm::StringLiteral DiagMsg = "DiagMsg";
+  class DiagModule final : public FeatureModule {
+struct DiagHooks : public ASTListener {
+  void sawDiagnostic(const clang::Diagnostic &, clangd::Diag ) override {
+D.Message = DiagMsg.str();
+  }
+};
+
+  public:
+std::unique_ptr astHooks() override {
+  return std::make_unique();
+}
+  };
+  FeatureModules.add(std::make_unique());
+
+  auto  = start();
+  Client.didOpen("foo.cpp", "test;");
+  EXPECT_THAT(Client.diagnostics("foo.cpp"),
+  llvm::ValueIs(testing::ElementsAre(DiagMessage(DiagMsg;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -46,6 +46,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -119,13 +120,18 @@
   }
 
   // Prepare inputs and build CompilerInvocation (parsed compile command).
-  bool buildInvocation(const ThreadsafeFS ,
- 

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/FeatureModule.h:112
+  };
+  /// Can be called asynchronously before building an AST.
+  virtual std::unique_ptr astHooks() { return nullptr; }

sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > This comment hints at what I *thought* was the idea: astHooks() is called 
> > > each time we parse a file, the returned object has methods called on it 
> > > while the file is being parsed, and is then destroyed.
> > > 
> > > But the code suggests we call once globally and it has effectively the 
> > > same lifetime as the module.
> > > This seems much less useful, e.g. if we want to observe several 
> > > diagnostics, examine the preamble, and emit new diagnostics, then we have 
> > > to plumb around some notion of "AST identity" rather than just tying it 
> > > to the identity of the ParseASTHooks object itself.
> > > 
> > > (Lots of natural extensions here like providing ParseInputs to 
> > > astHooks(), but YAGNI for now)
> > > This comment hints at what I *thought* was the idea: astHooks() is called 
> > > each time we parse a file, the returned object has methods called on it 
> > > while the file is being parsed, and is then destroyed.
> > 
> > This was the intent actually (to enable modularization of other features 
> > like clangtidy, includefixer, etc. as mentioned above), but looks like i 
> > got confused while integrating this into clangdserver :/
> > 
> > While trying to resolve my confusion i actually noticed that we cannot 
> > uphold the contract of "hooks being called synchronously", because we 
> > actually have both a preamblethread and astworker that can invoke hooks 
> > (embarrassing of me to forget that ).
> > 
> > So we can:
> > - Give up on that promise and make life slightly complicated for module 
> > implementers
> > - Don't invoke hooks from preamblethread, (at the cost of limiting 
> > functionality, we can still have downstream users that act on PPCallbacks, 
> > but no direct integration with compiler, and that's where layering 
> > violations are generated :/)
> > - Handle the synchronization ourselves, only complicates TUScheduler more, 
> > rather than all the module implementers.
> > - Propogate FeatureModuleSet into TUScheduler and create 2 set of hooks on 
> > each thread :)
> > 
> > I am leaning towards 4, but unsure (mostly hesitant about dependency 
> > schema, as featuremodules also depend on TUScheduler..). WDYT?
> > we actually have both a preamblethread and astworker that can invoke hooks 
> > (embarrassing of me to forget that )
> 
> Ha! And yes, in our motivating case of fixing build system rules, the 
> diagnostics are mostly going to be in the preamble.
> 
> The options that seem most tempting to me are:
>  - don't attempt to create/run astHooks while building preambles, but *do* 
> feed the preamble's Diags into the main-file's AST hooks every time it's 
> used. (you won't have a clang::Diagnostic, so that param would have to be 
> gone/optional, and we'd scrape the messages instead of extracting args). This 
> is kind of thematic, remember how we replay PPCallbacks for clang-tidy :-). 
> This is the smallest tweak to your #2 that actually works for us, I think.
>  - create one astHooks for the preamble, and another for the AST build, and 
> try to make the interface suitable for both. This is cute but there may be 
> too much tension between the two cases. (Is this what you mean by #4?)
>  - or have separate ASTHooks & PreambleHooks interfaces and support all this 
> crap for both. (Or is *this* what you mean by #4?) Hybrid is also possible, 
> give the interfaces an inheritance relationship, or have one interface but 
> pass boolean parameters to indicate which version we're doing, or...
>  - preambles and ASTs are a tree, so... give modules a PreambleHooks factory, 
> and the factory function for ASTHooks is PreambleHooks::astHooks(). Holy 
> overengineering batman...
> 
> These are roughly in order of complexity so we should probably start toward 
> the top of the list somewhere. Up to you.
> 
> (I don't like the #1 or #3 in your list above much at all.)
> create one astHooks for the preamble, and another for the AST build, and try 
> to make the interface suitable for both. This is cute but there may be too 
> much tension between the two cases. (Is this what you mean by #4?)

yes this is what i meant by #4. 

> These are roughly in order of complexity so we should probably start toward 
> the top of the list somewhere. Up to you.

I'd lean towards the second option(creating separate ASTHooks with the same 
interface for preamble and astworker). As discussed before first one (i.e. 
scraping diag message) is a limited solution that's likely to get us into a 
corner in the future. let me know if you have any concerns around this approach.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D98499#2644313 , @kadircet wrote:

> In D98499#2626502 , @sammccall wrote:
>
>> My model for modules using this diagnostic stuff (apart from the 
>> build-system stuff which sadly can't be meaningfully upstreamed) are 
>> IncludeFixer, ClangTidy, and IWYU - worth thinking about how we'd build 
>> those on top of this model. (Doesn't mean we need to add a hook to emit 
>> diagnostics, but it probably means we should know where it would go)
>
> Agreed. I believe they all would need extra end points to ASTHooks though.

Yup. Let's not bite off more for now.

> - make ASTHooks own it and instantiate a new one on every parse (i think the 
> cleanest and most explicit)

Agreed. (And this only one that overlaps this patch a lot)




Comment at: clang-tools-extra/clangd/FeatureModule.h:112
+  };
+  /// Can be called asynchronously before building an AST.
+  virtual std::unique_ptr astHooks() { return nullptr; }

kadircet wrote:
> sammccall wrote:
> > This comment hints at what I *thought* was the idea: astHooks() is called 
> > each time we parse a file, the returned object has methods called on it 
> > while the file is being parsed, and is then destroyed.
> > 
> > But the code suggests we call once globally and it has effectively the same 
> > lifetime as the module.
> > This seems much less useful, e.g. if we want to observe several 
> > diagnostics, examine the preamble, and emit new diagnostics, then we have 
> > to plumb around some notion of "AST identity" rather than just tying it to 
> > the identity of the ParseASTHooks object itself.
> > 
> > (Lots of natural extensions here like providing ParseInputs to astHooks(), 
> > but YAGNI for now)
> > This comment hints at what I *thought* was the idea: astHooks() is called 
> > each time we parse a file, the returned object has methods called on it 
> > while the file is being parsed, and is then destroyed.
> 
> This was the intent actually (to enable modularization of other features like 
> clangtidy, includefixer, etc. as mentioned above), but looks like i got 
> confused while integrating this into clangdserver :/
> 
> While trying to resolve my confusion i actually noticed that we cannot uphold 
> the contract of "hooks being called synchronously", because we actually have 
> both a preamblethread and astworker that can invoke hooks (embarrassing of me 
> to forget that ).
> 
> So we can:
> - Give up on that promise and make life slightly complicated for module 
> implementers
> - Don't invoke hooks from preamblethread, (at the cost of limiting 
> functionality, we can still have downstream users that act on PPCallbacks, 
> but no direct integration with compiler, and that's where layering violations 
> are generated :/)
> - Handle the synchronization ourselves, only complicates TUScheduler more, 
> rather than all the module implementers.
> - Propogate FeatureModuleSet into TUScheduler and create 2 set of hooks on 
> each thread :)
> 
> I am leaning towards 4, but unsure (mostly hesitant about dependency schema, 
> as featuremodules also depend on TUScheduler..). WDYT?
> we actually have both a preamblethread and astworker that can invoke hooks 
> (embarrassing of me to forget that )

Ha! And yes, in our motivating case of fixing build system rules, the 
diagnostics are mostly going to be in the preamble.

The options that seem most tempting to me are:
 - don't attempt to create/run astHooks while building preambles, but *do* feed 
the preamble's Diags into the main-file's AST hooks every time it's used. (you 
won't have a clang::Diagnostic, so that param would have to be gone/optional, 
and we'd scrape the messages instead of extracting args). This is kind of 
thematic, remember how we replay PPCallbacks for clang-tidy :-). This is the 
smallest tweak to your #2 that actually works for us, I think.
 - create one astHooks for the preamble, and another for the AST build, and try 
to make the interface suitable for both. This is cute but there may be too much 
tension between the two cases. (Is this what you mean by #4?)
 - or have separate ASTHooks & PreambleHooks interfaces and support all this 
crap for both. (Or is *this* what you mean by #4?) Hybrid is also possible, 
give the interfaces an inheritance relationship, or have one interface but pass 
boolean parameters to indicate which version we're doing, or...
 - preambles and ASTs are a tree, so... give modules a PreambleHooks factory, 
and the factory function for ASTHooks is PreambleHooks::astHooks(). Holy 
overengineering batman...

These are roughly in order of complexity so we should probably start toward the 
top of the list somewhere. Up to you.

(I don't like the #1 or #3 in your list above much at all.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 332613.
kadircet added a comment.

- Rename ParsedASTHooks to Listeners.
- Generate list of hooks on each parse.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/FeatureModule.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -25,6 +25,7 @@
 #include "support/Path.h"
 #include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 #include 
 #include 
@@ -76,6 +77,8 @@
   // to eliminate this option some day.
   bool OverlayRealFileSystemForModules = false;
 
+  std::vector> Hooks;
+
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   // The result will always have getDiagnostics() populated.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -36,6 +36,7 @@
   FS.Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
+  Inputs.ASTHooks = Hooks;
   auto  = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
@@ -97,6 +98,7 @@
   MockFS FS;
   auto Inputs = inputs(FS);
   StoreDiags Diags;
+  Diags.setASTHooks(Inputs.ASTHooks);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   if (OverlayRealFileSystemForModules)
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1346,6 +1346,24 @@
  });
 }
 
+TEST(ParsedASTTest, ModuleSawDiag) {
+  static constexpr const llvm::StringLiteral KDiagMsg = "StampedDiag";
+  struct Hooks : public FeatureModule::ASTListener {
+void sawDiagnostic(const clang::Diagnostic ,
+   clangd::Diag ) override {
+  Diag.Message = KDiagMsg.str();
+}
+  };
+
+  Annotations Code("[[test]]; /* error-ok */");
+  TestTU TU;
+  TU.Code = Code.code().str();
+  TU.Hooks.emplace_back(new Hooks);
+
+  auto AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+  testing::Contains(Diag(Code.range(), KDiagMsg.str(;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -365,6 +365,27 @@
   // And immediately shut down. FeatureModule destructor verifies we blocked.
 }
 
+TEST_F(LSPTest, DiagModuleTest) {
+  static constexpr llvm::StringLiteral DiagMsg = "DiagMsg";
+  class DiagModule final : public FeatureModule {
+struct DiagHooks : public ASTListener {
+  void sawDiagnostic(const clang::Diagnostic &, clangd::Diag ) override {
+D.Message = DiagMsg.str();
+  }
+};
+
+  public:
+std::unique_ptr astHooks() override {
+  return std::make_unique();
+}
+  };
+  FeatureModules.add(std::make_unique());
+
+  auto  = start();
+  Client.didOpen("foo.cpp", "test;");
+  EXPECT_THAT(Client.diagnostics("foo.cpp"),
+  llvm::ValueIs(testing::ElementsAre(DiagMessage(DiagMsg;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -46,6 +46,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -119,13 +120,17 @@
   }
 
   // Prepare inputs and build CompilerInvocation (parsed compile command).
-  bool buildInvocation(const ThreadsafeFS ,
-   llvm::Optional Contents) {
-

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added a comment.

In D98499#2626502 , @sammccall wrote:

> My model for modules using this diagnostic stuff (apart from the build-system 
> stuff which sadly can't be meaningfully upstreamed) are IncludeFixer, 
> ClangTidy, and IWYU - worth thinking about how we'd build those on top of 
> this model. (Doesn't mean we need to add a hook to emit diagnostics, but it 
> probably means we should know where it would go)

Agreed. I believe they all would need extra end points to ASTHooks though.

`ClangTidy`:

- needs extra hooks to register PP callbacks, and take in a diagnostics engine
- needs a new endpoint to traverse ast and *emit* diags
- CTContext needs to be alive until the parsing is done: so we can:
  - make ASTHooks own it and instantiate a new one on every parse (i think the 
cleanest and most explicit)
  - make the module own them and control the lifetime with entry/exit calls on 
the asthooks. (there's more burden on the modules now, and they'll need extra 
synchronisation on enter/exit calls)
  - return some other object on parsing entry hook that'll be kept alive by the 
caller (needs design around semantics of that object).

`IncludeFixer`:

- needs a new hook to act as an external sema source, so that it can fix 
unresolved names.
- similar to CTContext issue, it references per-tu state like HeaderSearchInfo, 
so will need to incorporate these into entry hook, and somehow disambiguate 
hooks for different TUs (all the options proposed for tidy).

`IWYU`:

- I'd expect this to make use of ~same API as `IncludeFixer`.

`PreamblePatching`:

- can mutate compiler instance via a hook
- drop diagnostics from ParsedAST on exit (requires some mutation on parsedast 
though)




Comment at: clang-tools-extra/clangd/FeatureModule.h:112
+  };
+  /// Can be called asynchronously before building an AST.
+  virtual std::unique_ptr astHooks() { return nullptr; }

sammccall wrote:
> This comment hints at what I *thought* was the idea: astHooks() is called 
> each time we parse a file, the returned object has methods called on it while 
> the file is being parsed, and is then destroyed.
> 
> But the code suggests we call once globally and it has effectively the same 
> lifetime as the module.
> This seems much less useful, e.g. if we want to observe several diagnostics, 
> examine the preamble, and emit new diagnostics, then we have to plumb around 
> some notion of "AST identity" rather than just tying it to the identity of 
> the ParseASTHooks object itself.
> 
> (Lots of natural extensions here like providing ParseInputs to astHooks(), 
> but YAGNI for now)
> This comment hints at what I *thought* was the idea: astHooks() is called 
> each time we parse a file, the returned object has methods called on it while 
> the file is being parsed, and is then destroyed.

This was the intent actually (to enable modularization of other features like 
clangtidy, includefixer, etc. as mentioned above), but looks like i got 
confused while integrating this into clangdserver :/

While trying to resolve my confusion i actually noticed that we cannot uphold 
the contract of "hooks being called synchronously", because we actually have 
both a preamblethread and astworker that can invoke hooks (embarrassing of me 
to forget that ).

So we can:
- Give up on that promise and make life slightly complicated for module 
implementers
- Don't invoke hooks from preamblethread, (at the cost of limiting 
functionality, we can still have downstream users that act on PPCallbacks, but 
no direct integration with compiler, and that's where layering violations are 
generated :/)
- Handle the synchronization ourselves, only complicates TUScheduler more, 
rather than all the module implementers.
- Propogate FeatureModuleSet into TUScheduler and create 2 set of hooks on each 
thread :)

I am leaning towards 4, but unsure (mostly hesitant about dependency schema, as 
featuremodules also depend on TUScheduler..). WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.
Herald added a project: clang-tools-extra.

I'm getting a little nervous about the amount of stuff we're packing into 
modules without in-tree examples.
I should split out some of the "standard" features into modules as that's 
possible already.

My model for modules using this diagnostic stuff (apart from the build-system 
stuff which sadly can't be meaningfully upstreamed) are IncludeFixer, 
ClangTidy, and IWYU - worth thinking about how we'd build those on top of this 
model. (Doesn't mean we need to add a hook to emit diagnostics, but it probably 
means we should know where it would go)




Comment at: clang-tools-extra/clangd/FeatureModule.h:104
+  /// implementations don't need to be thread-safe.
+  struct ParseASTHooks {
+virtual ~ParseASTHooks() = default;

naming: giving this a plural name and having a collection of them is a little 
confusing (is `Hooks` the hooks from one module, or from all of them?). What 
about ASTListener?
(Listener suggests passive but listeners that "participate" is a common enough 
idiom I think)



Comment at: clang-tools-extra/clangd/FeatureModule.h:112
+  };
+  /// Can be called asynchronously before building an AST.
+  virtual std::unique_ptr astHooks() { return nullptr; }

This comment hints at what I *thought* was the idea: astHooks() is called each 
time we parse a file, the returned object has methods called on it while the 
file is being parsed, and is then destroyed.

But the code suggests we call once globally and it has effectively the same 
lifetime as the module.
This seems much less useful, e.g. if we want to observe several diagnostics, 
examine the preamble, and emit new diagnostics, then we have to plumb around 
some notion of "AST identity" rather than just tying it to the identity of the 
ParseASTHooks object itself.

(Lots of natural extensions here like providing ParseInputs to astHooks(), but 
YAGNI for now)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 330258.
kadircet added a comment.

- Add tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98499/new/

https://reviews.llvm.org/D98499

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/FeatureModule.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -25,6 +25,7 @@
 #include "support/Path.h"
 #include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 #include 
 #include 
@@ -76,6 +77,8 @@
   // to eliminate this option some day.
   bool OverlayRealFileSystemForModules = false;
 
+  std::vector> Hooks;
+
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   ParsedAST build() const;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -36,6 +36,7 @@
   FS.Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
+  Inputs.ASTHooks = Hooks;
   auto  = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
@@ -97,6 +98,7 @@
   MockFS FS;
   auto Inputs = inputs(FS);
   StoreDiags Diags;
+  Diags.setASTHooks(Inputs.ASTHooks);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   if (OverlayRealFileSystemForModules)
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1346,6 +1346,24 @@
  });
 }
 
+TEST(ParsedASTTest, ModuleSawDiag) {
+  static constexpr const llvm::StringLiteral KDiagMsg = "StampedDiag";
+  struct Hooks : public FeatureModule::ParseASTHooks {
+void sawDiagnostic(const clang::Diagnostic ,
+   clangd::Diag ) override {
+  Diag.Message = KDiagMsg.str();
+}
+  };
+
+  Annotations Code("[[test]]; /* error-ok */");
+  TestTU TU;
+  TU.Code = Code.code().str();
+  TU.Hooks.emplace_back(new Hooks);
+
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getDiagnostics(),
+  testing::Contains(Diag(Code.range(), KDiagMsg.str(;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -365,6 +365,27 @@
   // And immediately shut down. FeatureModule destructor verifies we blocked.
 }
 
+TEST_F(LSPTest, DiagModuleTest) {
+  static constexpr llvm::StringLiteral DiagMsg = "DiagMsg";
+  class DiagModule final : public FeatureModule {
+struct DiagHooks : public ParseASTHooks {
+  void sawDiagnostic(const clang::Diagnostic &, clangd::Diag ) override {
+D.Message = DiagMsg.str();
+  }
+};
+
+  public:
+std::unique_ptr astHooks() override {
+  return std::make_unique();
+}
+  };
+  FeatureModules.add(std::make_unique());
+
+  auto  = start();
+  Client.didOpen("foo.cpp", "test;");
+  EXPECT_THAT(Client.diagnostics("foo.cpp"),
+  llvm::ValueIs(testing::ElementsAre(DiagMessage(DiagMsg;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -46,6 +46,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -119,13 +120,17 @@
   }
 
   // Prepare inputs and build CompilerInvocation (parsed compile command).
-  bool buildInvocation(const ThreadsafeFS ,
-   llvm::Optional Contents) {
-StoreDiags CaptureInvocationDiags;
+  bool buildInvocation(
+  const ThreadsafeFS , llvm::Optional 

[PATCH] D98499: [clangd] Introduce ASTHooks to FeatureModules

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: usaxena95, arphaman, javed.absar.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.

These can be invoked at different stages while building an AST to let
FeatureModules implement features on top of it. The patch also
introduces a sawDiagnostic hook, which can mutate the final clangd::Diag
while reading a clang::Diagnostic.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98499

Files:
  clang-tools-extra/clangd/Compiler.h
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/FeatureModule.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -25,6 +25,7 @@
 #include "support/Path.h"
 #include "llvm/ADT/StringMap.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 #include 
 #include 
@@ -76,6 +77,8 @@
   // to eliminate this option some day.
   bool OverlayRealFileSystemForModules = false;
 
+  std::vector> Hooks;
+
   // By default, build() will report Error diagnostics as GTest errors.
   // Suppress this behavior by adding an 'error-ok' comment to the code.
   ParsedAST build() const;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -36,6 +36,7 @@
   FS.Files[ImportThunk] = ThunkContents;
 
   ParseInputs Inputs;
+  Inputs.ASTHooks = Hooks;
   auto  = Inputs.CompileCommand.CommandLine;
   Argv = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
@@ -97,6 +98,7 @@
   MockFS FS;
   auto Inputs = inputs(FS);
   StoreDiags Diags;
+  Diags.setASTHooks(Inputs.ASTHooks);
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");
   if (OverlayRealFileSystemForModules)
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1346,6 +1346,24 @@
  });
 }
 
+TEST(ParsedASTTest, ModuleSawDiag) {
+  static constexpr const llvm::StringLiteral KDiagMsg = "StampedDiag";
+  struct Hooks : public FeatureModule::ParseASTHooks {
+void sawDiagnostic(const clang::Diagnostic ,
+   clangd::Diag ) override {
+  Diag.Message = KDiagMsg.str();
+}
+  };
+
+  Annotations Code("[[test]]; /* error-ok */");
+  TestTU TU;
+  TU.Code = Code.code().str();
+  TU.Hooks.emplace_back(new Hooks);
+
+  auto AST = TU.build();
+  EXPECT_THAT(AST.getDiagnostics(),
+  testing::Contains(Diag(Code.range(), KDiagMsg.str(;
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -46,6 +46,7 @@
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Path.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -119,13 +120,17 @@
   }
 
   // Prepare inputs and build CompilerInvocation (parsed compile command).
-  bool buildInvocation(const ThreadsafeFS ,
-   llvm::Optional Contents) {
-StoreDiags CaptureInvocationDiags;
+  bool buildInvocation(
+  const ThreadsafeFS , llvm::Optional Contents,
+  llvm::ArrayRef> Hooks) {
 std::vector CC1Args;
 Inputs.CompileCommand = Cmd;
 Inputs.TFS = 
 Inputs.ClangTidyProvider = Opts.ClangTidyProvider;
+Inputs.ASTHooks = Hooks;
+
+StoreDiags CaptureInvocationDiags;
+CaptureInvocationDiags.setASTHooks(Hooks);
 if (Contents.hasValue()) {
   Inputs.Contents = *Contents;
   log("Imaginary source file contents:\n{0}", Inputs.Contents);
@@ -252,7 +257,15 @@
   Opts.ConfigProvider, nullptr);
   WithContext Ctx(ContextProvider(""));
   Checker C(File, Opts);
-  if (!C.buildCommand(TFS) || !C.buildInvocation(TFS, Contents) ||
+  std::vector> Hooks;
+  if (Opts.FeatureModules) {
+for (auto  : *Opts.FeatureModules) {
+  if (auto H = M.astHooks())
+Hooks.push_back(std::move(H));
+}
+  }
+
+  if (!C.buildCommand(TFS) || !C.buildInvocation(TFS, Contents,