[clangd] Add diagnostic augmentation hook to Modules

[kadircet]

Summary

Depends on D96245

[kadircet]

ping :)

[sammccall]

clang-tools-extra/clangd/Module.h

97

This seems too specialized to me - maybe we want to observe diagnostics only, or mutate some other property of them.

Consider just passing in the (mutable) clangd::Diag, and returning void? Maybe call it handleDiagnostic or so

Maybe also the clang::Diagnostic if you really need it.

98

random thoughts about how this gets called... (definitely not for now though!)

There are other things we might want to do while building ASTs.
e.g. *generate* diagnostics (maybe IWYU or clang-tidy can be modules)
At that point maybe we want something like

virtual unique_ptr<ParseASTHooks> Module::parseAST() { return nullptr; } // threadsafe
class ParseASTHooks { // implementations need not be threadsafe
  virtual void sawDiagnostic(clangd::Diag &D);
  virtual void finish(ParsedAST&);
};

That way it's easier to reason about call sequences. (and it eliminates the problem of calling an empty virtual method on each module for each diagnostic)

[kadircet]

clang-tools-extra/clangd/Module.h

97

passing in clangd::Diag and mutating it sounds pretty neat, thanks!

I do think it is also useful to pass clang::Diagnostic though, as it usually contains extra semantic information that might be used to generate a fix.
For example in case of missing module deps:

• it knows about the particular include/import that triggered the diagnostic
• name of the current module.

The former can be inferred from diagnostic range and reading the source code (we'll need to pass in sourcemanager somehow though, to ensure we read the same version of the file), and the latter can be inferred by looking at the compile commands (or implementations can query the build system with TUs name etc.). But this is one particular case, it might tie our hands when we want to generate fixes that require deeper semantic analysis (like type info) in the future. So i think we should really be passing clang::Diagnostic too.

So I'll update this to take in a read-only clang::Diagnostic, Level and a mutable clangd::Diag, does that SG?

98

SG indeed, I'd be happy to do that in a follow-up change.

Though this might still have the problem of calling an empty virtual method with each diag, for example if a module doesn't do anything per-diagnostic but only does something on finish, but it definitely limits the scope.

[sammccall]

Is this entirely obsoleted by the approach in D98499?

[kadircet]

Is this entirely obsoleted by the approach in D98499?

yes it is!

clang-tools-extra/clangd/ClangdServer.cpp

void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
   ParseInputs Inputs;
   Inputs.TFS = &TFS;
   Inputs.Contents = std::string(Contents);
   Inputs.Version = Version.str();
   Inputs.ForceRebuild = ForceRebuild;
   Inputs.Opts = std::move(Opts);
   Inputs.Index = Index;
   Inputs.ClangTidyProvider = ClangTidyProvider;
+  Inputs.Modules = Modules;
   bool NewFile = WorkScheduler->update(File, Inputs, WantDiags);
   // If we loaded Foo.h, we want to make sure Foo.cpp is indexed.
   if (NewFile && BackgroundIdx)
     BackgroundIdx->boostRelated(File);
 }
 
 std::function<Context(PathRef)>
 ClangdServer::createConfiguredContextProvider(const config::Provider *Provider,

clang-tools-extra/clangd/Compiler.h

 // CodeComplete run their own compile actions that share these low-level pieces.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H
 
 #include "GlobalCompilationDatabase.h"
+#include "Module.h"
 #include "TidyProvider.h"
 #include "index/Index.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Tooling/CompilationDatabase.h"
 
 namespace clang {
struct ParseInputs {
   std::string Version = "null";
   // Prevent reuse of the cached preamble/AST. Slow! Useful to workaround
   // clangd's assumption that missing header files will stay missing.
   bool ForceRebuild = false;
   // Used to recover from diagnostics (e.g. find missing includes for symbol).
   const SymbolIndex *Index = nullptr;
   ParseOptions Opts = ParseOptions();
   TidyProviderRef ClangTidyProvider = {};
+  const ModuleSet *Modules = nullptr;
 };
 
 /// Builds compiler invocation that could be used to build AST or preamble.
 std::unique_ptr<CompilerInvocation>
 buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,
                         std::vector<std::string> *CC1Args = nullptr);
 
 /// Creates a compiler instance, configured so that:

clang-tools-extra/clangd/Diagnostics.h

 //===--- Diagnostics.h -------------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H
 
+#include "Module.h"
 #include "Protocol.h"
 #include "support/Path.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/None.h"
public:
   using LevelAdjuster = std::function<DiagnosticsEngine::Level(
       DiagnosticsEngine::Level, const clang::Diagnostic &)>;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
   /// If set, this allows the client of this class to adjust the level of
   /// diagnostics, such as promoting warnings to errors, or ignoring
   /// diagnostics.
   void setLevelAdjuster(LevelAdjuster Adjuster) { this->Adjuster = Adjuster; }
+  void setDiagAugmenters(const ModuleSet *Modules) { this->Modules = Modules; }
 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
   LevelAdjuster Adjuster = nullptr;
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
   llvm::Optional<FullSourceLoc> LastDiagLoc; // Valid only when LastDiag is set.
   bool LastDiagOriginallyError = false;      // Valid only when LastDiag is set.
   SourceManager *OrigSrcMgr = nullptr;
+  const ModuleSet *Modules = nullptr;
 
   llvm::DenseSet<std::pair<unsigned, unsigned>> IncludedErrorLocations;
   bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 /// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
 bool isBuiltinDiagnosticSuppressed(unsigned ID,
                                    const llvm::StringSet<> &Suppressed);

clang-tools-extra/clangd/Diagnostics.cpp

if (!isNote(DiagLevel)) {
 
     if (!Info.getFixItHints().empty())
       AddFix(true /* try to invent a message instead of repeating the diag */);
     if (Fixer) {
       auto ExtraFixes = Fixer(DiagLevel, Info);
       LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(),
                              ExtraFixes.end());
     }
+    if (Modules) {
+      for (auto &M : *Modules) {
+        if (auto Action = M.augmentDiagFix(DiagLevel, Info))
+          LastDiag->Actions.emplace_back(std::move(*Action));
+      }
+    }
   } else {
     // Handle a note to an existing diagnostic.
 
     // If a diagnostic was suppressed due to the suppression filter,
     // also suppress notes associated with it.
     if (LastPrimaryDiagnosticWasSuppressed) {
       return;
     }

clang-tools-extra/clangd/Module.h

 //===--- Module.h - Plugging features into clangd -----------------*-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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_MODULE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_MODULE_H
 
+#include "Protocol.h"
 #include "support/Function.h"
 #include "support/Threading.h"
+#include "clang/Basic/Diagnostic.h"
 #include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/JSON.h"
 #include <memory>
 #include <type_traits>
 #include <vector>
 
 namespace clang {
public:
 
   /// Waits until the module is idle (no background work) or a deadline expires.
   /// In general all modules should eventually go idle, though it may take a
   /// long time (e.g. background indexing).
   /// Modules should go idle quickly if stop() has been called.
   /// Called by the server when shutting down, and also by tests.
   virtual bool blockUntilIdle(Deadline) { return true; }
 
+  /// Called by auxilary threads as diagnostics encountered to generate fixes.
+  /// So this can be called concurrently from multiple threads. If a module
+  /// provides a code action with a custom, it should also register itself as
+  /// that commands handler.
+  virtual llvm::Optional<CodeAction>
+  augmentDiagFix(DiagnosticsEngine::Level L, const clang::Diagnostic &Diag) {
+    return llvm::None;
+  }
+
 protected:
   /// Accessors for modules to access shared server facilities they depend on.
   Facilities &facilities();
   /// The scheduler is used to run tasks on worker threads and access ASTs.
   TUScheduler &scheduler() { return facilities().Scheduler; }
   /// The index is used to get information about the whole codebase.
   const SymbolIndex *index() { return facilities().Index; }
   /// The filesystem is used to read source files on disk.

clang-tools-extra/clangd/ParsedAST.cpp

ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
   const PrecompiledPreamble *PreamblePCH =
       Preamble ? &Preamble->Preamble : nullptr;
 
   // This is on-by-default in windows to allow parsing SDK headers, but it
   // breaks many features. Disable it for the main-file (not preamble).
   CI->getLangOpts()->DelayedTemplateParsing = false;
 
   StoreDiags ASTDiags;
+  ASTDiags.setDiagAugmenters(Inputs.Modules);
 
   llvm::Optional<PreamblePatch> Patch;
   if (Preamble) {
     Patch = PreamblePatch::create(Filename, Inputs, *Preamble);
     Patch->apply(*CI);
   }
   auto Clang = prepareCompilerInstance(
       std::move(CI), PreamblePCH,

clang-tools-extra/clangd/Preamble.cpp

buildPreamble(PathRef FileName, CompilerInvocation CI,
   // without those.
   auto ContentsBuffer =
       llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
   auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
 
   trace::Span Tracer("BuildPreamble");
   SPAN_ATTACH(Tracer, "File", FileName);
   StoreDiags PreambleDiagnostics;
+  PreambleDiagnostics.setDiagAugmenters(Inputs.Modules);
   llvm::IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
       CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
                                           &PreambleDiagnostics, false);
 
   // Skip function bodies when building the preamble to speed up building
   // the preamble and make it smaller.
   assert(!CI.getFrontendOpts().SkipFunctionBodies);
   CI.getFrontendOpts().SkipFunctionBodies = true;

clang-tools-extra/clangd/TUScheduler.cpp

auto Task = [=]() mutable {
     }
 
     log("ASTWorker building file {0} version {1} with command {2}\n[{3}]\n{4}",
         FileName, Inputs.Version, Inputs.CompileCommand.Heuristic,
         Inputs.CompileCommand.Directory,
         printArgv(Inputs.CompileCommand.CommandLine));
 
     StoreDiags CompilerInvocationDiagConsumer;
+    CompilerInvocationDiagConsumer.setDiagAugmenters(Inputs.Modules);
     std::vector<std::string> CC1Args;
     std::unique_ptr<CompilerInvocation> Invocation = buildCompilerInvocation(
         Inputs, CompilerInvocationDiagConsumer, &CC1Args);
     // Log cc1 args even (especially!) if creating invocation failed.
     if (!CC1Args.empty())
       vlog("Driver produced command: cc1 {0}", printArgv(CC1Args));
     std::vector<Diag> CompilerInvocationDiags =
         CompilerInvocationDiagConsumer.take();
static constexpr trace::Metric ASTAccessForRead(
       "ast_access_read", trace::Metric::Counter, "result");
   auto Task = [=, Action = std::move(Action)]() mutable {
     if (auto Reason = isCancelled())
       return Action(llvm::make_error<CancelledError>(Reason));
     llvm::Optional<std::unique_ptr<ParsedAST>> AST =
         IdleASTs.take(this, &ASTAccessForRead);
     if (!AST) {
       StoreDiags CompilerInvocationDiagConsumer;
+      CompilerInvocationDiagConsumer.setDiagAugmenters(FileInputs.Modules);
       std::unique_ptr<CompilerInvocation> Invocation =
           buildCompilerInvocation(FileInputs, CompilerInvocationDiagConsumer);
       // Try rebuilding the AST.
       vlog("ASTWorker rebuilding evicted AST to run {0}: {1} version {2}", Name,
            FileName, FileInputs.Version);
       // FIXME: We might need to build a patched ast once preamble thread starts
       // running async. Currently getPossiblyStalePreamble below will always
       // return a compatible preamble as ASTWorker::update blocks.