[clangd] parse all make_unique-like functions in preamble

I am working on support for forwarding parameter names in make_unique-like functions, first for inlay hints, later maybe for signature help. For that to work generically, I'd like to parse all of these functions in the preamble. Not sure how this impacts performance on large codebases though. Reviewed By: sammccall Differential Revision: https://reviews.llvm.org/D124688
llvm · May 16, 2022 · 71cb8c8 · 71cb8c8
1 parent 436bbce
commit 71cb8c8
Show file tree

Hide file tree

Showing 9 changed files with 103 additions and 17 deletions.
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -148,7 +148,9 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB,
     : FeatureModules(Opts.FeatureModules), CDB(CDB), TFS(TFS),
       DynamicIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex() : nullptr),
       ClangTidyProvider(Opts.ClangTidyProvider),
-      UseDirtyHeaders(Opts.UseDirtyHeaders), WorkspaceRoot(Opts.WorkspaceRoot),
+      UseDirtyHeaders(Opts.UseDirtyHeaders),
+      PreambleParseForwardingFunctions(Opts.PreambleParseForwardingFunctions),
+      WorkspaceRoot(Opts.WorkspaceRoot),
       Transient(Opts.ImplicitCancellation ? TUScheduler::InvalidateOnUpdate
                                           : TUScheduler::NoInvalidation),
       DirtyFS(std::make_unique<DraftStoreFS>(TFS, DraftMgr)) {
@@ -217,6 +219,7 @@ void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                WantDiagnostics WantDiags, bool ForceRebuild) {
   std::string ActualVersion = DraftMgr.addDraft(File, Version, Contents);
   ParseOptions Opts;
+  Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions;
 
   // Compile command is set asynchronously during update, as it can be slow.
   ParseInputs Inputs;
@@ -910,7 +913,7 @@ void ClangdServer::getAST(PathRef File, llvm::Optional<Range> R,
           // It's safe to pass in the TU, as dumpAST() does not
           // deserialize the preamble.
           auto Node = DynTypedNode::create(
-                *Inputs->AST.getASTContext().getTranslationUnitDecl());
+              *Inputs->AST.getASTContext().getTranslationUnitDecl());
           return CB(dumpAST(Node, Inputs->AST.getTokens(),
                             Inputs->AST.getASTContext()));
         }

diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
@@ -164,6 +164,9 @@ class ClangdServer {
     /// If true, use the dirty buffer contents when building Preambles.
     bool UseDirtyHeaders = false;
 
+    // If true, parse emplace-like functions in the preamble.
+    bool PreambleParseForwardingFunctions = false;
+
     explicit operator TUScheduler::Options() const;
   };
   // Sensible default options for use in tests.
@@ -416,6 +419,8 @@ class ClangdServer {
 
   bool UseDirtyHeaders = false;
 
+  bool PreambleParseForwardingFunctions = false;
+
   // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
   llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
       CachedCompletionFuzzyFindRequestByFile;

diff --git a/clang-tools-extra/clangd/Compiler.h b/clang-tools-extra/clangd/Compiler.h
@@ -39,7 +39,7 @@ class IgnoreDiagnostics : public DiagnosticConsumer {
 
 // Options to run clang e.g. when parsing AST.
 struct ParseOptions {
-  // (empty at present, formerly controlled recovery AST, include-fixer etc)
+  bool PreambleParseForwardingFunctions = false;
 };
 
 /// Information required to run clang, e.g. to parse AST or do code completion.

diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
@@ -66,9 +66,10 @@ class CppFilePreambleCallbacks : public PreambleCallbacks {
 public:
   CppFilePreambleCallbacks(
       PathRef File, PreambleParsedCallback ParsedCallback,
-      PreambleBuildStats *Stats,
+      PreambleBuildStats *Stats, bool ParseForwardingFunctions,
       std::function<void(CompilerInstance &)> BeforeExecuteCallback)
       : File(File), ParsedCallback(ParsedCallback), Stats(Stats),
+        ParseForwardingFunctions(ParseForwardingFunctions),
         BeforeExecuteCallback(std::move(BeforeExecuteCallback)) {}
 
   IncludeStructure takeIncludes() { return std::move(Includes); }
@@ -136,15 +137,48 @@ class CppFilePreambleCallbacks : public PreambleCallbacks {
     return IWYUHandler.get();
   }
 
+  static bool isLikelyForwardingFunction(FunctionTemplateDecl *FT) {
+    const auto *FD = FT->getTemplatedDecl();
+    const auto NumParams = FD->getNumParams();
+    // Check whether its last parameter is a parameter pack...
+    if (NumParams > 0) {
+      const auto *LastParam = FD->getParamDecl(NumParams - 1);
+      if (const auto *PET = dyn_cast<PackExpansionType>(LastParam->getType())) {
+        // ... of the type T&&... or T...
+        const auto BaseType = PET->getPattern().getNonReferenceType();
+        if (const auto *TTPT =
+                dyn_cast<TemplateTypeParmType>(BaseType.getTypePtr())) {
+          // ... whose template parameter comes from the function directly
+          if (FT->getTemplateParameters()->getDepth() == TTPT->getDepth()) {
+            return true;
+          }
+        }
+      }
+    }
+    return false;
+  }
+
   bool shouldSkipFunctionBody(Decl *D) override {
-    // Generally we skip function bodies in preambles for speed.
-    // We can make exceptions for functions that are cheap to parse and
-    // instantiate, widely used, and valuable (e.g. commonly produce errors).
-    if (const auto *FT = llvm::dyn_cast<clang::FunctionTemplateDecl>(D)) {
-      if (const auto *II = FT->getDeclName().getAsIdentifierInfo())
-        // std::make_unique is trivial, and we diagnose bad constructor calls.
-        if (II->isStr("make_unique") && FT->isInStdNamespace())
+    // Usually we don't need to look inside the bodies of header functions
+    // to understand the program. However when forwarding function like
+    // emplace() forward their arguments to some other function, the
+    // interesting overload resolution happens inside the forwarding
+    // function's body. To provide more meaningful diagnostics,
+    // code completion, and parameter hints we should parse (and later
+    // instantiate) the bodies.
+    if (auto *FT = llvm::dyn_cast<clang::FunctionTemplateDecl>(D)) {
+      if (ParseForwardingFunctions) {
+        // Don't skip parsing the body if it looks like a forwarding function
+        if (isLikelyForwardingFunction(FT))
           return false;
+      } else {
+        // By default, only take care of make_unique
+        // std::make_unique is trivial, and we diagnose bad constructor calls.
+        if (const auto *II = FT->getDeclName().getAsIdentifierInfo()) {
+          if (II->isStr("make_unique") && FT->isInStdNamespace())
+            return false;
+        }
+      }
     }
     return true;
   }
@@ -161,6 +195,7 @@ class CppFilePreambleCallbacks : public PreambleCallbacks {
   const clang::LangOptions *LangOpts = nullptr;
   const SourceManager *SourceMgr = nullptr;
   PreambleBuildStats *Stats;
+  bool ParseForwardingFunctions;
   std::function<void(CompilerInstance &)> BeforeExecuteCallback;
 };
 
@@ -484,11 +519,13 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
   // to read back. We rely on dynamic index for the comments instead.
   CI.getPreprocessorOpts().WriteCommentListToPCH = false;
 
-  CppFilePreambleCallbacks CapturedInfo(FileName, PreambleCallback, Stats,
-                                        [&ASTListeners](CompilerInstance &CI) {
-                                          for (const auto &L : ASTListeners)
-                                            L->beforeExecute(CI);
-                                        });
+  CppFilePreambleCallbacks CapturedInfo(
+      FileName, PreambleCallback, Stats,
+      Inputs.Opts.PreambleParseForwardingFunctions,
+      [&ASTListeners](CompilerInstance &CI) {
+        for (const auto &L : ASTListeners)
+          L->beforeExecute(CI);
+      });
   auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
   llvm::SmallString<32> AbsFileName(FileName);
   VFS->makeAbsolute(AbsFileName);

diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp
@@ -128,6 +128,8 @@ class Checker {
     Inputs.CompileCommand = Cmd;
     Inputs.TFS = &TFS;
     Inputs.ClangTidyProvider = Opts.ClangTidyProvider;
+    Inputs.Opts.PreambleParseForwardingFunctions =
+        Opts.PreambleParseForwardingFunctions;
     if (Contents.hasValue()) {
       Inputs.Contents = *Contents;
       log("Imaginary source file contents:\n{0}", Inputs.Contents);

diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -497,6 +497,14 @@ opt<bool> UseDirtyHeaders{"use-dirty-headers", cat(Misc),
                           Hidden,
                           init(ClangdServer::Options().UseDirtyHeaders)};
 
+opt<bool> PreambleParseForwardingFunctions{
+    "parse-forwarding-functions",
+    cat(Misc),
+    desc("Parse all emplace-like functions in included headers"),
+    Hidden,
+    init(ParseOptions().PreambleParseForwardingFunctions),
+};
+
 #if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
 opt<bool> EnableMallocTrim{
     "malloc-trim",
@@ -934,6 +942,7 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
     Opts.ClangTidyProvider = ClangTidyOptProvider;
   }
   Opts.UseDirtyHeaders = UseDirtyHeaders;
+  Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions;
   Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
   Opts.TweakFilter = [&](const Tweak &T) {
     if (T.hidden() && !HiddenFeatures)

diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -389,7 +389,7 @@ TEST(DiagnosticTest, MakeUnique) {
     namespace std {
     // These mocks aren't quite right - we omit unique_ptr for simplicity.
     // forward is included to show its body is not needed to get the diagnostic.
-    template <typename T> T&& forward(T& t) { return static_cast<T&&>(t); }
+    template <typename T> T&& forward(T& t);
     template <typename T, typename... A> T* make_unique(A&&... args) {
        return new T(std::forward<A>(args)...);
     }
@@ -402,6 +402,32 @@ TEST(DiagnosticTest, MakeUnique) {
                        "no matching constructor for initialization of 'S'")));
 }
 
+TEST(DiagnosticTest, MakeShared) {
+  // We usually miss diagnostics from header functions as we don't parse them.
+  // std::make_shared is only parsed when --parse-forwarding-functions is set
+  Annotations Main(R"cpp(
+    struct S { S(char*); };
+    auto x = std::[[make_shared]]<S>(42); // error-ok
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.HeaderCode = R"cpp(
+    namespace std {
+    // These mocks aren't quite right - we omit shared_ptr for simplicity.
+    // forward is included to show its body is not needed to get the diagnostic.
+    template <typename T> T&& forward(T& t);
+    template <typename T, typename... A> T* make_shared(A&&... args) {
+       return new T(std::forward<A>(args)...);
+    }
+    }
+  )cpp";
+  TU.ParseOpts.PreambleParseForwardingFunctions = true;
+  EXPECT_THAT(*TU.build().getDiagnostics(),
+              UnorderedElementsAre(
+                  Diag(Main.range(),
+                       "in template: "
+                       "no matching constructor for initialization of 'S'")));
+}
+
 TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
   Annotations Main(R"cpp(
     template <typename T> struct Foo {

diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -109,6 +109,7 @@ TestTU::preamble(PreambleParsedCallback PreambleCallback) const {
 ParsedAST TestTU::build() const {
   MockFS FS;
   auto Inputs = inputs(FS);
+  Inputs.Opts = ParseOpts;
   StoreDiags Diags;
   auto CI = buildCompilerInvocation(Inputs, Diags);
   assert(CI && "Failed to build compilation invocation.");

diff --git a/clang-tools-extra/clangd/unittests/TestTU.h b/clang-tools-extra/clangd/unittests/TestTU.h
@@ -66,6 +66,9 @@ struct TestTU {
   // Simulate a header guard of the header (using an #import directive).
   bool ImplicitHeaderGuard = true;
 
+  // Parse options pass on to the ParseInputs
+  ParseOptions ParseOpts = {};
+
   // Whether to use overlay the TestFS over the real filesystem. This is
   // required for use of implicit modules.where the module file is written to
   // disk and later read back.