dgoldman updated this revision to Diff 486291. dgoldman marked 4 inline comments as done. dgoldman added a comment.
Update comment + test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139458/new/ https://reviews.llvm.org/D139458 Files: clang-tools-extra/clangd/AST.cpp clang-tools-extra/clangd/AST.h clang-tools-extra/clangd/ASTSignals.cpp clang-tools-extra/clangd/ASTSignals.h clang-tools-extra/clangd/CodeComplete.cpp clang-tools-extra/clangd/IncludeFixer.cpp clang-tools-extra/clangd/IncludeFixer.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/ParsedAST.h clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp clang-tools-extra/clangd/unittests/ASTTests.cpp clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -2718,17 +2718,21 @@ Sym.CanonicalDeclaration.FileURI = DeclFile.c_str(); Sym.IncludeHeaders.emplace_back("\"bar.h\"", 1000, Symbol::Include | Symbol::Import); - - auto Results = completions("Fun^", {Sym}).Completions; + CodeCompleteOptions Opts; + // Should only take effect in import contexts. + Opts.ImportInsertions = true; + auto Results = completions("Fun^", {Sym}, Opts).Completions; assert(!Results.empty()); EXPECT_THAT(Results[0], AllOf(named("Func"), insertIncludeText("#include \"bar.h\"\n"))); - Results = completions("Fun^", {Sym}, {}, "Foo.m").Completions; + ASTSignals Signals; + Signals.InsertionDirective = Symbol::IncludeDirective::Import; + Opts.MainFileSignals = &Signals; + Results = completions("Fun^", {Sym}, Opts, "Foo.m").Completions; assert(!Results.empty()); - // TODO: Once #import integration support is done this should be #import. EXPECT_THAT(Results[0], - AllOf(named("Func"), insertIncludeText("#include \"bar.h\"\n"))); + AllOf(named("Func"), insertIncludeText("#import \"bar.h\"\n"))); Sym.IncludeHeaders[0].SupportedDirectives = Symbol::Import; Results = completions("Fun^", {Sym}).Completions; Index: clang-tools-extra/clangd/unittests/ASTTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ASTTests.cpp +++ clang-tools-extra/clangd/unittests/ASTTests.cpp @@ -11,6 +11,7 @@ #include "Annotations.h" #include "ParsedAST.h" #include "TestTU.h" +#include "index/Symbol.h" #include "clang/AST/ASTTypeTraits.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" @@ -618,6 +619,45 @@ hasReservedScope(*findUnqualifiedDecl(AST, "secret").getDeclContext())); } +TEST(ClangdAST, PreferredIncludeDirective) { + auto ComputePreferredDirective = [](TestTU &TU) { + auto AST = TU.build(); + return preferredIncludeDirective(AST.tuPath(), AST.getLangOpts(), + AST.getIncludeStructure().MainFileIncludes, + AST.getLocalTopLevelDecls()); + }; + TestTU ObjCTU = TestTU::withCode(R"cpp( + int main() {} + )cpp"); + ObjCTU.Filename = "TestTU.m"; + EXPECT_EQ(ComputePreferredDirective(ObjCTU), + Symbol::IncludeDirective::Import); + + TestTU HeaderTU = TestTU::withCode(R"cpp( + #import "TestTU.h" + )cpp"); + HeaderTU.Filename = "TestTUHeader.h"; + HeaderTU.ExtraArgs = {"-xobjective-c++-header"}; + EXPECT_EQ(ComputePreferredDirective(HeaderTU), + Symbol::IncludeDirective::Import); + + // ObjC language option is not enough for headers. + HeaderTU.Code = R"cpp( + #include "TestTU.h" + )cpp"; + EXPECT_EQ(ComputePreferredDirective(HeaderTU), + Symbol::IncludeDirective::Include); + + HeaderTU.Code = R"cpp( + @interface Foo + @end + + Foo * getFoo(); + )cpp"; + EXPECT_EQ(ComputePreferredDirective(HeaderTU), + Symbol::IncludeDirective::Import); +} + } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp +++ clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp @@ -68,6 +68,7 @@ Pair(sym("Y", index::SymbolKind::Variable, "@N@tar@S@X@FI@\\0").ID, 2), Pair(_ /*a*/, 3))); + EXPECT_EQ(Signals.InsertionDirective, Symbol::IncludeDirective::Include); } } // namespace } // namespace clangd Index: clang-tools-extra/clangd/ParsedAST.h =================================================================== --- clang-tools-extra/clangd/ParsedAST.h +++ clang-tools-extra/clangd/ParsedAST.h @@ -86,6 +86,7 @@ /// The result does not include the decls that come from the preamble. /// (These should be const, but RecursiveASTVisitor requires Decl*). ArrayRef<Decl *> getLocalTopLevelDecls(); + ArrayRef<const Decl *> getLocalTopLevelDecls() const; const llvm::Optional<std::vector<Diag>> &getDiagnostics() const { return Diags; Index: clang-tools-extra/clangd/ParsedAST.cpp =================================================================== --- clang-tools-extra/clangd/ParsedAST.cpp +++ clang-tools-extra/clangd/ParsedAST.cpp @@ -11,6 +11,7 @@ #include "../clang-tidy/ClangTidyDiagnosticConsumer.h" #include "../clang-tidy/ClangTidyModuleRegistry.h" #include "AST.h" +#include "ASTSignals.h" #include "Compiler.h" #include "Config.h" #include "Diagnostics.h" @@ -25,6 +26,7 @@ #include "TidyProvider.h" #include "index/CanonicalIncludes.h" #include "index/Index.h" +#include "index/Symbol.h" #include "support/Logger.h" #include "support/Trace.h" #include "clang/AST/ASTContext.h" @@ -559,12 +561,18 @@ auto Inserter = std::make_shared<IncludeInserter>( Filename, Inputs.Contents, Style, BuildDir.get(), &Clang->getPreprocessor().getHeaderSearchInfo()); + ArrayRef<Inclusion> MainFileIncludes; if (Preamble) { + MainFileIncludes = Preamble->Includes.MainFileIncludes; for (const auto &Inc : Preamble->Includes.MainFileIncludes) Inserter->addExisting(Inc); } + // FIXME: Consider piping through ASTSignals to fetch this to handle the + // case where a header file contains ObjC decls but no #imports. + Symbol::IncludeDirective Directive = preferredIncludeDirective( + Filename, Clang->getLangOpts(), MainFileIncludes, {}); FixIncludes.emplace(Filename, Inserter, *Inputs.Index, - /*IndexRequestLimit=*/5); + /*IndexRequestLimit=*/5, Directive); ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl, const clang::Diagnostic &Info) { return FixIncludes->fix(DiagLevl, Info); @@ -719,6 +727,10 @@ return LocalTopLevelDecls; } +llvm::ArrayRef<const Decl *> ParsedAST::getLocalTopLevelDecls() const { + return LocalTopLevelDecls; +} + const MainFileMacros &ParsedAST::getMacros() const { return Macros; } const std::vector<PragmaMark> &ParsedAST::getMarks() const { return Marks; } Index: clang-tools-extra/clangd/IncludeFixer.h =================================================================== --- clang-tools-extra/clangd/IncludeFixer.h +++ clang-tools-extra/clangd/IncludeFixer.h @@ -34,9 +34,10 @@ class IncludeFixer { public: IncludeFixer(llvm::StringRef File, std::shared_ptr<IncludeInserter> Inserter, - const SymbolIndex &Index, unsigned IndexRequestLimit) + const SymbolIndex &Index, unsigned IndexRequestLimit, + Symbol::IncludeDirective Directive) : File(File), Inserter(std::move(Inserter)), Index(Index), - IndexRequestLimit(IndexRequestLimit) {} + IndexRequestLimit(IndexRequestLimit), Directive(Directive) {} /// Returns include insertions that can potentially recover the diagnostic. /// If Info is a note and fixes are returned, they should *replace* the note. @@ -80,6 +81,7 @@ const SymbolIndex &Index; const unsigned IndexRequestLimit; // Make at most 5 index requests. mutable unsigned IndexRequestCount = 0; + const Symbol::IncludeDirective Directive; // These collect the last unresolved name so that we can associate it with the // diagnostic. Index: clang-tools-extra/clangd/IncludeFixer.cpp =================================================================== --- clang-tools-extra/clangd/IncludeFixer.cpp +++ clang-tools-extra/clangd/IncludeFixer.cpp @@ -321,8 +321,7 @@ llvm::StringSet<> InsertedHeaders; for (const auto &Sym : Syms) { for (const auto &Inc : getRankedIncludes(Sym)) { - // FIXME: We should support #import directives here. - if ((Inc.Directive & clang::clangd::Symbol::Include) == 0) + if ((Inc.Directive & Directive) == 0) continue; if (auto ToInclude = Inserted(Sym, Inc.Header)) { if (ToInclude->second) { @@ -330,7 +329,9 @@ continue; if (auto Fix = insertHeader(ToInclude->first, (Sym.Scope + Sym.Name).str(), - tooling::IncludeDirective::Include)) + Directive == Symbol::Import + ? tooling::IncludeDirective::Import + : tooling::IncludeDirective::Include)) Fixes.push_back(std::move(*Fix)); } } else { Index: clang-tools-extra/clangd/CodeComplete.cpp =================================================================== --- clang-tools-extra/clangd/CodeComplete.cpp +++ clang-tools-extra/clangd/CodeComplete.cpp @@ -177,6 +177,12 @@ return Result; } +Symbol::IncludeDirective insertionDirective(const CodeCompleteOptions &Opts) { + if (!Opts.ImportInsertions || !Opts.MainFileSignals) + return Symbol::IncludeDirective::Include; + return Opts.MainFileSignals->InsertionDirective; +} + // Identifier code completion result. struct RawIdentifier { llvm::StringRef Name; @@ -267,9 +273,9 @@ if (SM.isInMainFile(SM.getExpansionLoc(RD->getBeginLoc()))) return std::nullopt; } + Symbol::IncludeDirective Directive = insertionDirective(Opts); for (const auto &Inc : RankedIncludeHeaders) - // FIXME: We should support #import directives here. - if ((Inc.Directive & clang::clangd::Symbol::Include) != 0) + if ((Inc.Directive & Directive) != 0) return Inc.Header; return None; } @@ -385,10 +391,10 @@ Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted)); }; bool ShouldInsert = C.headerToInsertIfAllowed(Opts).has_value(); + Symbol::IncludeDirective Directive = insertionDirective(Opts); // Calculate include paths and edits for all possible headers. for (const auto &Inc : C.RankedIncludeHeaders) { - // FIXME: We should support #import directives here. - if ((Inc.Directive & clang::clangd::Symbol::Include) == 0) + if ((Inc.Directive & Directive) == 0) continue; if (auto ToInclude = Inserted(Inc.Header)) { @@ -396,7 +402,9 @@ Include.Header = ToInclude->first; if (ToInclude->second && ShouldInsert) Include.Insertion = Includes.insert( - ToInclude->first, tooling::IncludeDirective::Include); + ToInclude->first, Directive == Symbol::Import + ? tooling::IncludeDirective::Import + : tooling::IncludeDirective::Include); Completion.Includes.push_back(std::move(Include)); } else log("Failed to generate include insertion edits for adding header " Index: clang-tools-extra/clangd/ASTSignals.h =================================================================== --- clang-tools-extra/clangd/ASTSignals.h +++ clang-tools-extra/clangd/ASTSignals.h @@ -10,6 +10,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_ASTSIGNALS_H #include "ParsedAST.h" +#include "index/Symbol.h" #include "index/SymbolID.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/StringMap.h" @@ -29,6 +30,9 @@ /// Namespaces whose symbols are used in the file, and the number of such /// distinct symbols. llvm::StringMap<unsigned> RelatedNamespaces; + /// Preferred preprocessor directive to use for inclusions by the file. + Symbol::IncludeDirective InsertionDirective = + Symbol::IncludeDirective::Include; static ASTSignals derive(const ParsedAST &AST); }; Index: clang-tools-extra/clangd/ASTSignals.cpp =================================================================== --- clang-tools-extra/clangd/ASTSignals.cpp +++ clang-tools-extra/clangd/ASTSignals.cpp @@ -9,13 +9,18 @@ #include "ASTSignals.h" #include "AST.h" #include "FindTarget.h" +#include "Headers.h" #include "support/Trace.h" +#include "clang/AST/DeclObjC.h" namespace clang { namespace clangd { ASTSignals ASTSignals::derive(const ParsedAST &AST) { trace::Span Span("ASTSignals::derive"); ASTSignals Signals; + Signals.InsertionDirective = preferredIncludeDirective( + AST.tuPath(), AST.getLangOpts(), + AST.getIncludeStructure().MainFileIncludes, AST.getLocalTopLevelDecls()); const SourceManager &SM = AST.getSourceManager(); findExplicitReferences( AST.getASTContext(), Index: clang-tools-extra/clangd/AST.h =================================================================== --- clang-tools-extra/clangd/AST.h +++ clang-tools-extra/clangd/AST.h @@ -13,6 +13,8 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_ #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_ +#include "Headers.h" +#include "index/Symbol.h" #include "index/SymbolID.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclObjC.h" @@ -117,6 +119,18 @@ /// return nullptr as protocols don't have an implementation. const ObjCImplDecl *getCorrespondingObjCImpl(const ObjCContainerDecl *D); +/// Infer the include directive to use for the given \p FileName. It aims for +/// #import for ObjC files and #include for the rest. +/// +/// - For source files we use LangOpts directly to infer ObjC-ness. +/// - For header files we also check for symbols declared by the file and +/// existing include directives, as the language can be set to ObjC++ as a +/// fallback in the absence of compile flags. +Symbol::IncludeDirective +preferredIncludeDirective(llvm::StringRef FileName, const LangOptions &LangOpts, + ArrayRef<Inclusion> MainFileIncludes, + ArrayRef<const Decl *> TopLevelDecls); + /// Returns a QualType as string. The result doesn't contain unwritten scopes /// like anonymous/inline namespace. std::string printType(const QualType QT, const DeclContext &CurContext, Index: clang-tools-extra/clangd/AST.cpp =================================================================== --- clang-tools-extra/clangd/AST.cpp +++ clang-tools-extra/clangd/AST.cpp @@ -377,6 +377,38 @@ return nullptr; } +Symbol::IncludeDirective +preferredIncludeDirective(llvm::StringRef FileName, const LangOptions &LangOpts, + ArrayRef<Inclusion> MainFileIncludes, + ArrayRef<const Decl *> TopLevelDecls) { + // Always prefer #include for non-ObjC code. + if (!LangOpts.ObjC) + return Symbol::IncludeDirective::Include; + // If this is not a header file and has ObjC set as the language, prefer + // #import. + if (!isHeaderFile(FileName, LangOpts)) + return Symbol::IncludeDirective::Import; + + // Headers lack proper compile flags most of the time, so we might treat a + // header as ObjC accidentally. Perform some extra checks to make sure this + // works. + + // Any file with a #import, should keep #import-ing. + for (auto &Inc : MainFileIncludes) + if (Inc.Directive == tok::pp_import) + return Symbol::IncludeDirective::Import; + + // Any file declaring an ObjC decl should also be #import-ing. + // No need to look over the references, as the file doesn't have any #imports, + // it must be declaring interesting ObjC-like decls. + for (const Decl *D : TopLevelDecls) + if (isa<ObjCContainerDecl, ObjCIvarDecl, ObjCMethodDecl, ObjCPropertyDecl>( + D)) + return Symbol::IncludeDirective::Import; + + return Symbol::IncludeDirective::Include; +} + std::string printType(const QualType QT, const DeclContext &CurContext, const llvm::StringRef Placeholder) { std::string Result;
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits