Author: ioeric Date: Thu Feb 22 02:14:05 2018 New Revision: 325764 URL: http://llvm.org/viewvc/llvm-project?rev=325764&view=rev Log: [clangd] Not collect include headers for dynamic index for now.
Summary: The new behaviors introduced by this patch: o When include collection is enabled, we always set IncludeHeader field in Symbol even if it's the same as FileURI in decl. o Disable include collection in FileIndex which is currently only used to build dynamic index. We should revisit when we actually want to use FileIndex to global index. o Code-completion only uses IncludeHeader to insert headers but not FileURI in CanonicalDeclaration. This ensures that inserted headers are always canonicalized. Note that include insertion can still be triggered for symbols that are already included if they are merged from dynamic index and static index, but we would only use includes that are already canonicalized (e.g. from static index). Reason for change: Collecting header includes in dynamic index enables inserting includes for headers that are not indexed but opened in the editor. Comparing to inserting includes for symbols in global/static index, this is nice-to-have but would probably require non-trivial amount of work to get right. For example: o Currently it's not easy to fully support CanonicalIncludes in dynamic index, given the way we run dynamic index. o It's also harder to reason about the correctness of include canonicalization for dynamic index (i.e. symbols in the current file/TU) than static index where symbols are collected offline and sanity check is possible before shipping to production. o We have less control/flexibility over symbol info in the dynamic index (e.g. URIs, path normalization), which could be used to help make decision when inserting includes. As header collection (especially canonicalization) is relatively new, and enabling it for dynamic index would immediately affect current users with only dynamic index support, I propose we disable it for dynamic index for now to avoid compromising other hot features like code completion and only support it for static index where include insertion would likely to bring more value. Reviewers: ilya-biryukov, sammccall, hokein Subscribers: klimek, jkorous-apple, cfe-commits Differential Revision: https://reviews.llvm.org/D43550 Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp clang-tools-extra/trunk/clangd/index/FileIndex.cpp clang-tools-extra/trunk/clangd/index/Index.h clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=325764&r1=325763&r2=325764&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original) +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Thu Feb 22 02:14:05 2018 @@ -286,13 +286,9 @@ struct CompletionCandidate { I.documentation = D->Documentation; if (I.detail.empty()) I.detail = D->CompletionDetail; - // We only insert #include for items with details, since we can't tell - // whether the file URI of the canonical declaration would be the - // canonical #include without checking IncludeHeader in the detail. // FIXME: delay creating include insertion command to // "completionItem/resolve", when it is supported - if (!D->IncludeHeader.empty() || - !IndexResult->CanonicalDeclaration.FileURI.empty()) { + if (!D->IncludeHeader.empty()) { // LSP favors additionalTextEdits over command. But we are still using // command here because it would be expensive to calculate #include // insertion edits for all candidates, and the include insertion edit @@ -301,9 +297,7 @@ struct CompletionCandidate { // Command title is not added since this is not a user-facing command. Cmd.command = ExecuteCommandParams::CLANGD_INSERT_HEADER_INCLUDE; IncludeInsertion Insertion; - Insertion.header = D->IncludeHeader.empty() - ? IndexResult->CanonicalDeclaration.FileURI - : D->IncludeHeader; + Insertion.header = D->IncludeHeader; Insertion.textDocument.uri = URIForFile(FileName); Cmd.includeInsertion = std::move(Insertion); I.command = std::move(Cmd); Modified: clang-tools-extra/trunk/clangd/index/FileIndex.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.cpp?rev=325764&r1=325763&r2=325764&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/FileIndex.cpp (original) +++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp Thu Feb 22 02:14:05 2018 @@ -15,15 +15,6 @@ namespace clang { namespace clangd { namespace { -const CanonicalIncludes *canonicalIncludesForSystemHeaders() { - static const auto *Includes = [] { - auto *I = new CanonicalIncludes(); - addSystemHeadersMapping(I); - return I; - }(); - return Includes; -} - /// Retrieves namespace and class level symbols in \p Decls. std::unique_ptr<SymbolSlab> indexAST(ASTContext &Ctx, std::shared_ptr<Preprocessor> PP, @@ -32,12 +23,14 @@ std::unique_ptr<SymbolSlab> indexAST(AST // Although we do not index symbols in main files (e.g. cpp file), information // in main files like definition locations of class declarations will still be // collected; thus, the index works for go-to-definition. - // FIXME(ioeric): handle IWYU pragma for dynamic index. We might want to make - // SymbolCollector always provides include canonicalization (e.g. IWYU, STL). // FIXME(ioeric): get rid of `IndexMainFiles` as this is always set to false. CollectorOpts.IndexMainFiles = false; - CollectorOpts.CollectIncludePath = true; - CollectorOpts.Includes = canonicalIncludesForSystemHeaders(); + // FIXME(ioeric): we might also want to collect include headers. We would need + // to make sure all includes are canonicalized (with CanonicalIncludes), which + // is not trivial given the current way of collecting symbols: we only have + // AST at this point, but we also need preprocessor callbacks (e.g. + // CommentHandler for IWYU pragma) to canonicalize includes. + CollectorOpts.CollectIncludePath = false; auto Collector = std::make_shared<SymbolCollector>(std::move(CollectorOpts)); Collector->setPreprocessor(std::move(PP)); Modified: clang-tools-extra/trunk/clangd/index/Index.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.h?rev=325764&r1=325763&r2=325764&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Index.h (original) +++ clang-tools-extra/trunk/clangd/index/Index.h Thu Feb 22 02:14:05 2018 @@ -162,8 +162,8 @@ struct Symbol { /// directly. When this is a URI, the exact #include path needs to be /// calculated according to the URI scheme. /// - /// If empty, FileURI in CanonicalDeclaration should be used to calculate - /// the #include path. + /// This is a canonical include for the symbol and can be different from + /// FileURI in the CanonicalDeclaration. llvm::StringRef IncludeHeader; }; Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=325764&r1=325763&r2=325764&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original) +++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Thu Feb 22 02:14:05 2018 @@ -150,8 +150,9 @@ bool shouldCollectIncludePath(index::Sym } } -/// Gets a canonical include (<header> or "header") for header of \p Loc. -/// Returns None if the header has no canonical include. +/// Gets a canonical include (URI of the header or <header> or "header") for +/// header of \p Loc. +/// Returns None if fails to get include header for \p Loc. /// FIXME: we should handle .inc files whose symbols are expected be exported by /// their containing headers. llvm::Optional<std::string> @@ -167,10 +168,8 @@ getIncludeHeader(const SourceManager &SM ? Mapped.str() : ("\"" + Mapped + "\"").str(); } - // If the header path is the same as the file path of the declaration, we skip - // storing the #include path; users can use the URI in declaration location to - // calculate the #include path. - return llvm::None; + + return toURI(SM, SM.getFilename(Loc), Opts); } // Return the symbol location of the given declaration `D`. Modified: clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp?rev=325764&r1=325763&r2=325764&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp Thu Feb 22 02:14:05 2018 @@ -181,19 +181,19 @@ TEST(FileIndexTest, IgnoreClassMembers) EXPECT_THAT(match(M, Req), UnorderedElementsAre("X")); } -#ifndef LLVM_ON_WIN32 -TEST(FileIndexTest, CanonicalizeSystemHeader) { +TEST(FileIndexTest, NoIncludeCollected) { FileIndex M; - std::string File = testPath("bits/basic_string"); - M.update(File, build(File, "class string {};").getPointer()); + M.update("f", build("f", "class string {};").getPointer()); FuzzyFindRequest Req; Req.Query = ""; + bool SeenSymbol = false; M.fuzzyFind(Req, [&](const Symbol &Sym) { - EXPECT_EQ(Sym.Detail->IncludeHeader, "<string>"); + EXPECT_TRUE(Sym.Detail->IncludeHeader.empty()); + SeenSymbol = true; }); + EXPECT_TRUE(SeenSymbol); } -#endif } // namespace } // namespace clangd Modified: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp?rev=325764&r1=325763&r2=325764&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp Thu Feb 22 02:14:05 2018 @@ -585,7 +585,7 @@ TEST_F(SymbolCollectorTest, IncludeHeade runSymbolCollector("class Foo {};", /*Main=*/""); EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI), - IncludeHeader("")))); + IncludeHeader(TestHeaderURI)))); } #ifndef LLVM_ON_WIN32 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
