diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h --- a/clang-tools-extra/clangd/AST.h +++ b/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,20 @@ /// return nullptr as protocols don't have an implementation. const ObjCImplDecl *getCorrespondingObjCImpl(const ObjCContainerDecl *D); +/// Returns the Include Directive which should be used for include insertions +/// for the given main file. +/// +/// For source files, we use #import if the ObjC language flag is enabled, +/// otherwise we use #include. +/// +/// For header files with the ObjC language flag enabled, we search for #imports +/// and ObjC decls, only use #import if one of those are found. This helps +/// prevent treating C++ headers as ObjC when the language flag isn't accurate. +Symbol::IncludeDirective +preferredIncludeDirective(llvm::StringRef Filename, const LangOptions &LangOpts, + ArrayRef MainFileIncludes, + ArrayRef 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, diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp --- a/clang-tools-extra/clangd/AST.cpp +++ b/clang-tools-extra/clangd/AST.cpp @@ -377,6 +377,38 @@ return nullptr; } +Symbol::IncludeDirective +preferredIncludeDirective(llvm::StringRef Filename, const LangOptions &LangOpts, + ArrayRef MainFileIncludes, + ArrayRef 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 (Decl *D : TopLevelDecls) + if (isa( + 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; diff --git a/clang-tools-extra/clangd/ASTSignals.h b/clang-tools-extra/clangd/ASTSignals.h --- a/clang-tools-extra/clangd/ASTSignals.h +++ b/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 RelatedNamespaces; + /// Preferred preprocessor directive to use for inclusions by the file. + Symbol::IncludeDirective InsertionDirective = + Symbol::IncludeDirective::Include; static ASTSignals derive(const ParsedAST &AST); }; diff --git a/clang-tools-extra/clangd/ASTSignals.cpp b/clang-tools-extra/clangd/ASTSignals.cpp --- a/clang-tools-extra/clangd/ASTSignals.cpp +++ b/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(), diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/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 " diff --git a/clang-tools-extra/clangd/IncludeFixer.h b/clang-tools-extra/clangd/IncludeFixer.h --- a/clang-tools-extra/clangd/IncludeFixer.h +++ b/clang-tools-extra/clangd/IncludeFixer.h @@ -34,9 +34,10 @@ class IncludeFixer { public: IncludeFixer(llvm::StringRef File, std::shared_ptr 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. diff --git a/clang-tools-extra/clangd/IncludeFixer.cpp b/clang-tools-extra/clangd/IncludeFixer.cpp --- a/clang-tools-extra/clangd/IncludeFixer.cpp +++ b/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 { diff --git a/clang-tools-extra/clangd/ParsedAST.h b/clang-tools-extra/clangd/ParsedAST.h --- a/clang-tools-extra/clangd/ParsedAST.h +++ b/clang-tools-extra/clangd/ParsedAST.h @@ -85,7 +85,7 @@ /// This function returns top-level decls present in the main file of the AST. /// The result does not include the decls that come from the preamble. /// (These should be const, but RecursiveASTVisitor requires Decl*). - ArrayRef getLocalTopLevelDecls(); + ArrayRef getLocalTopLevelDecls() const; const llvm::Optional> &getDiagnostics() const { return Diags; diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/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( Filename, Inputs.Contents, Style, BuildDir.get(), &Clang->getPreprocessor().getHeaderSearchInfo()); + ArrayRef 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); @@ -715,7 +723,7 @@ return Clang->getPreprocessor(); } -llvm::ArrayRef ParsedAST::getLocalTopLevelDecls() { +llvm::ArrayRef ParsedAST::getLocalTopLevelDecls() const { return LocalTopLevelDecls; } diff --git a/clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp b/clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp --- a/clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp +++ b/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 diff --git a/clang-tools-extra/clangd/unittests/ASTTests.cpp b/clang-tools-extra/clangd/unittests/ASTTests.cpp --- a/clang-tools-extra/clangd/unittests/ASTTests.cpp +++ b/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,46 @@ hasReservedScope(*findUnqualifiedDecl(AST, "secret").getDeclContext())); } +Symbol::IncludeDirective preferredIncludeDirective(TestTU &TU) { + auto AST = TU.build(); + return preferredIncludeDirective(AST.tuPath(), AST.getLangOpts(), + AST.getIncludeStructure().MainFileIncludes, + AST.getLocalTopLevelDecls()); +} + +TEST(ClangdAST, PreferredIncludeDirective) { + TestTU ObjCTU = TestTU::withCode(R"cpp( + int main() {} + )cpp"); + ObjCTU.Filename = "TestTU.m"; + EXPECT_EQ(preferredIncludeDirective(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(preferredIncludeDirective(HeaderTU), + Symbol::IncludeDirective::Import); + + // ObjC language option is not enough for headers. + HeaderTU.Code = R"cpp( + #include "TestTU.h" + )cpp"; + EXPECT_EQ(preferredIncludeDirective(HeaderTU), + Symbol::IncludeDirective::Include); + + HeaderTU.Code = R"cpp( + @interface Foo + @end + + Foo * getFoo(); + )cpp"; + EXPECT_EQ(preferredIncludeDirective(HeaderTU), + Symbol::IncludeDirective::Import); +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/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;