[clang-tools-extra] r354268 - [clangd] Cache include fixes for diagnostics caused by the same unresolved name or incomplete type.

Eric Liu via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 18 05:12:11 PST 2019


Author: ioeric
Date: Mon Feb 18 05:12:10 2019
New Revision: 354268

URL: http://llvm.org/viewvc/llvm-project?rev=354268&view=rev
Log:
[clangd] Cache include fixes for diagnostics caused by the same unresolved name or incomplete type.

Summary:
Multiple diagnostics can be caused by the same unresolved name or incomplete type,
especially if the code is copy-pasted without #includes. The cache can avoid making
repetitive index requests, and thus reduce latency and allow more diagnostics to be
fixed (we limit the number of index requests for each parse).

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, jdoerfert, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D58239

Modified:
    clang-tools-extra/trunk/clangd/IncludeFixer.cpp
    clang-tools-extra/trunk/clangd/IncludeFixer.h
    clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp

Modified: clang-tools-extra/trunk/clangd/IncludeFixer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.cpp?rev=354268&r1=354267&r2=354268&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/IncludeFixer.cpp (original)
+++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp Mon Feb 18 05:12:10 2019
@@ -27,6 +27,7 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
@@ -57,8 +58,6 @@ private:
 
 std::vector<Fix> IncludeFixer::fix(DiagnosticsEngine::Level DiagLevel,
                                    const clang::Diagnostic &Info) const {
-  if (IndexRequestCount >= IndexRequestLimit)
-    return {}; // Avoid querying index too many times in a single parse.
   switch (Info.getID()) {
   case diag::err_incomplete_type:
   case diag::err_incomplete_member_access:
@@ -118,26 +117,21 @@ std::vector<Fix> IncludeFixer::fixIncomp
   auto ID = getSymbolID(TD);
   if (!ID)
     return {};
-  ++IndexRequestCount;
-  // FIXME: consider batching the requests for all diagnostics.
-  // FIXME: consider caching the lookup results.
-  LookupRequest Req;
-  Req.IDs.insert(*ID);
-  llvm::Optional<Symbol> Matched;
-  Index.lookup(Req, [&](const Symbol &Sym) {
-    if (Matched)
-      return;
-    Matched = Sym;
-  });
-
-  if (!Matched || Matched->IncludeHeaders.empty() || !Matched->Definition ||
-      Matched->CanonicalDeclaration.FileURI != Matched->Definition.FileURI)
+  llvm::Optional<const SymbolSlab *> Symbols = lookupCached(*ID);
+  if (!Symbols)
     return {};
-  return fixesForSymbols({*Matched});
+  const SymbolSlab &Syms = **Symbols;
+  std::vector<Fix> Fixes;
+  if (!Syms.empty()) {
+    auto &Matched = *Syms.begin();
+    if (!Matched.IncludeHeaders.empty() && Matched.Definition &&
+        Matched.CanonicalDeclaration.FileURI == Matched.Definition.FileURI)
+      Fixes = fixesForSymbols(Syms);
+  }
+  return Fixes;
 }
 
-std::vector<Fix>
-IncludeFixer::fixesForSymbols(llvm::ArrayRef<Symbol> Syms) const {
+std::vector<Fix> IncludeFixer::fixesForSymbols(const SymbolSlab &Syms) const {
   auto Inserted = [&](const Symbol &Sym, llvm::StringRef Header)
       -> llvm::Expected<std::pair<std::string, bool>> {
     auto ResolvedDeclaring =
@@ -289,6 +283,24 @@ std::vector<Fix> IncludeFixer::fixUnreso
   Req.RestrictForCodeCompletion = true;
   Req.Limit = 100;
 
+  if (llvm::Optional<const SymbolSlab *> Syms = fuzzyFindCached(Req))
+    return fixesForSymbols(**Syms);
+
+  return {};
+}
+
+
+llvm::Optional<const SymbolSlab *>
+IncludeFixer::fuzzyFindCached(const FuzzyFindRequest &Req) const {
+  auto ReqStr = llvm::formatv("{0}", toJSON(Req)).str();
+  auto I = FuzzyFindCache.find(ReqStr);
+  if (I != FuzzyFindCache.end())
+    return &I->second;
+
+  if (IndexRequestCount >= IndexRequestLimit)
+    return llvm::None;
+  IndexRequestCount++;
+
   SymbolSlab::Builder Matches;
   Index.fuzzyFind(Req, [&](const Symbol &Sym) {
     if (Sym.Name != Req.Query)
@@ -297,7 +309,37 @@ std::vector<Fix> IncludeFixer::fixUnreso
       Matches.insert(Sym);
   });
   auto Syms = std::move(Matches).build();
-  return fixesForSymbols(std::vector<Symbol>(Syms.begin(), Syms.end()));
+  auto E = FuzzyFindCache.try_emplace(ReqStr, std::move(Syms));
+  return &E.first->second;
+}
+
+llvm::Optional<const SymbolSlab *>
+IncludeFixer::lookupCached(const SymbolID &ID) const {
+  LookupRequest Req;
+  Req.IDs.insert(ID);
+
+  auto I = LookupCache.find(ID);
+  if (I != LookupCache.end())
+    return &I->second;
+
+  if (IndexRequestCount >= IndexRequestLimit)
+    return llvm::None;
+  IndexRequestCount++;
+
+  // FIXME: consider batching the requests for all diagnostics.
+  SymbolSlab::Builder Matches;
+  Index.lookup(Req, [&](const Symbol &Sym) { Matches.insert(Sym); });
+  auto Syms = std::move(Matches).build();
+
+  std::vector<Fix> Fixes;
+  if (!Syms.empty()) {
+    auto &Matched = *Syms.begin();
+    if (!Matched.IncludeHeaders.empty() && Matched.Definition &&
+        Matched.CanonicalDeclaration.FileURI == Matched.Definition.FileURI)
+      Fixes = fixesForSymbols(Syms);
+  }
+  auto E = LookupCache.try_emplace(ID, std::move(Syms));
+  return &E.first->second;
 }
 
 } // namespace clangd

Modified: clang-tools-extra/trunk/clangd/IncludeFixer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.h?rev=354268&r1=354267&r2=354268&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/IncludeFixer.h (original)
+++ clang-tools-extra/trunk/clangd/IncludeFixer.h Mon Feb 18 05:12:10 2019
@@ -21,6 +21,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include <memory>
 
@@ -51,7 +52,7 @@ private:
   std::vector<Fix> fixIncompleteType(const Type &T) const;
 
   /// Generates header insertion fixes for all symbols. Fixes are deduplicated.
-  std::vector<Fix> fixesForSymbols(llvm::ArrayRef<Symbol> Syms) const;
+  std::vector<Fix> fixesForSymbols(const SymbolSlab &Syms) const;
 
   struct UnresolvedName {
     std::string Name;   // E.g. "X" in foo::X.
@@ -79,6 +80,17 @@ private:
   // These collect the last unresolved name so that we can associate it with the
   // diagnostic.
   llvm::Optional<UnresolvedName> LastUnresolvedName;
+
+  // There can be multiple diagnostics that are caused by the same unresolved
+  // name or incomplete type in one parse, especially when code is
+  // copy-and-pasted without #includes. We cache the index results based on
+  // index requests.
+  mutable llvm::StringMap<SymbolSlab> FuzzyFindCache;
+  mutable llvm::DenseMap<SymbolID, SymbolSlab> LookupCache;
+  // Returns None if the number of index requests has reached the limit.
+  llvm::Optional<const SymbolSlab *>
+  fuzzyFindCached(const FuzzyFindRequest &Req) const;
+  llvm::Optional<const SymbolSlab *> lookupCached(const SymbolID &ID) const;
 };
 
 } // namespace clangd

Modified: clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp?rev=354268&r1=354267&r2=354268&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp Mon Feb 18 05:12:10 2019
@@ -449,6 +449,44 @@ TEST(IncludeFixerTest, NoCrashMemebrAcce
       UnorderedElementsAre(Diag(Test.range(), "no member named 'xy' in 'X'")));
 }
 
+TEST(IncludeFixerTest, UseCachedIndexResults) {
+  // As index results for the identical request are cached, more than 5 fixes
+  // are generated.
+  Annotations Test(R"cpp(
+$insert[[]]void foo() {
+  $x1[[X]] x;
+  $x2[[X]] x;
+  $x3[[X]] x;
+  $x4[[X]] x;
+  $x5[[X]] x;
+  $x6[[X]] x;
+  $x7[[X]] x;
+}
+
+class X;
+void bar(X *x) {
+  x$a1[[->]]f();
+  x$a2[[->]]f();
+  x$a3[[->]]f();
+  x$a4[[->]]f();
+  x$a5[[->]]f();
+  x$a6[[->]]f();
+  x$a7[[->]]f();
+}
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index =
+      buildIndexWithSymbol(SymbolWithHeader{"X", "unittest:///a.h", "\"a.h\""});
+  TU.ExternalIndex = Index.get();
+
+  auto Parsed = TU.build();
+  for (const auto &D : Parsed.getDiagnostics()) {
+    EXPECT_EQ(D.Fixes.size(), 1u);
+    EXPECT_EQ(D.Fixes[0].Message,
+              std::string("Add include \"a.h\" for symbol X"));
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang




More information about the cfe-commits mailing list