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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
