dgoldman updated this revision to Diff 482950. dgoldman marked 5 inline comments as done. dgoldman added a comment.
Fixes for review 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/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/TUScheduler.cpp clang-tools-extra/clangd/tool/Check.cpp clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp clang-tools-extra/clangd/unittests/ParsedASTTests.cpp clang-tools-extra/clangd/unittests/PreambleTests.cpp clang-tools-extra/clangd/unittests/TestTU.cpp clang-tools-extra/clangd/unittests/TestTU.h
Index: clang-tools-extra/clangd/unittests/TestTU.h =================================================================== --- clang-tools-extra/clangd/unittests/TestTU.h +++ clang-tools-extra/clangd/unittests/TestTU.h @@ -18,6 +18,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_TESTTU_H #include "../TidyProvider.h" +#include "ASTSignals.h" #include "Compiler.h" #include "FeatureModule.h" #include "ParsedAST.h" @@ -72,6 +73,9 @@ // Parse options pass on to the ParseInputs ParseOptions ParseOpts = {}; + // ASTSignals to use. Must be manually set. + std::shared_ptr<const ASTSignals> Signals; + // Whether to use overlay the TestFS over the real filesystem. This is // required for use of implicit modules.where the module file is written to // disk and later read back. Index: clang-tools-extra/clangd/unittests/TestTU.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TestTU.cpp +++ clang-tools-extra/clangd/unittests/TestTU.cpp @@ -127,7 +127,7 @@ /*StoreInMemory=*/true, /*PreambleCallback=*/nullptr); auto AST = ParsedAST::build(testPath(Filename), Inputs, std::move(CI), - Diags.take(), Preamble); + Diags.take(), Preamble, Signals); if (!AST) { llvm::errs() << "Failed to build code:\n" << Code; std::abort(); Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/PreambleTests.cpp +++ clang-tools-extra/clangd/unittests/PreambleTests.cpp @@ -206,7 +206,7 @@ return std::nullopt; } return ParsedAST::build(testPath(TU.Filename), TU.inputs(FS), std::move(CI), - {}, BaselinePreamble); + {}, BaselinePreamble, {}); } std::string getPreamblePatch(llvm::StringRef Baseline, Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp +++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp @@ -505,7 +505,7 @@ TU.Code = ModifiedContents.str(); Inputs = TU.inputs(FS); auto PatchedAST = ParsedAST::build(testPath("foo.cpp"), Inputs, std::move(CI), - {}, EmptyPreamble); + {}, EmptyPreamble, {}); ASSERT_TRUE(PatchedAST); ASSERT_FALSE(PatchedAST->getDiagnostics()); @@ -550,7 +550,7 @@ TU.Code = ""; Inputs = TU.inputs(FS); auto PatchedAST = ParsedAST::build(testPath("foo.cpp"), Inputs, std::move(CI), - {}, BaselinePreamble); + {}, BaselinePreamble, {}); ASSERT_TRUE(PatchedAST); // Ensure source location information is correct. 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/ASTSignalsTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp +++ clang-tools-extra/clangd/unittests/ASTSignalsTests.cpp @@ -68,7 +68,44 @@ Pair(sym("Y", index::SymbolKind::Variable, "@N@tar@S@X@FI@\\0").ID, 2), Pair(_ /*a*/, 3))); + EXPECT_EQ(Signals.InsertionDirective, Symbol::IncludeDirective::Include); } + +TEST(ASTSignals, DeriveInsertionDirective) { + TestTU ObjCTU = TestTU::withCode(R"cpp( + int main() {} + )cpp"); + ObjCTU.Filename = "TestTU.m"; + ASTSignals Signals = ASTSignals::derive(ObjCTU.build()); + EXPECT_EQ(Signals.InsertionDirective, Symbol::IncludeDirective::Import); + + TestTU HeaderTU = TestTU::withCode(R"cpp( + #import "TestTU.h" + )cpp"); + HeaderTU.Filename = "TestTUHeader.h"; + HeaderTU.ExtraArgs = {"-xobjective-c++-header"}; + Signals = ASTSignals::derive(HeaderTU.build()); + EXPECT_EQ(Signals.InsertionDirective, Symbol::IncludeDirective::Import); + + // ObjC language option is not enough for headers. + HeaderTU.Code = R"cpp( + #include "TestTU.h" + )cpp"; + Signals = ASTSignals::derive(HeaderTU.build()); + EXPECT_EQ(Signals.InsertionDirective, Symbol::IncludeDirective::Include); + + HeaderTU.Code = R"cpp( + #include "TestTU.h" + + @interface Foo + @end + + Foo * getFoo(); + )cpp"; + Signals = ASTSignals::derive(HeaderTU.build()); + EXPECT_EQ(Signals.InsertionDirective, Symbol::IncludeDirective::Import); +} + } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/tool/Check.cpp =================================================================== --- clang-tools-extra/clangd/tool/Check.cpp +++ clang-tools-extra/clangd/tool/Check.cpp @@ -223,7 +223,8 @@ log("Building AST..."); AST = ParsedAST::build(File, Inputs, std::move(Invocation), - /*InvocationDiags=*/std::vector<Diag>{}, Preamble); + /*InvocationDiags=*/std::vector<Diag>{}, Preamble, + /*Signals=*/{}); if (!AST) { elog("Failed to build AST"); return false; @@ -287,7 +288,8 @@ IgnoringDiagConsumer IgnoreDiags; auto Invocation = buildCompilerInvocation(Inputs, IgnoreDiags); Duration Val = Time([&] { - ParsedAST::build(File, Inputs, std::move(Invocation), {}, Preamble); + ParsedAST::build(File, Inputs, std::move(Invocation), {}, Preamble, + /*Signals=*/{}); }); vlog(" Measured {0} ==> {1}", Checks, Val); return Val; Index: clang-tools-extra/clangd/TUScheduler.cpp =================================================================== --- clang-tools-extra/clangd/TUScheduler.cpp +++ clang-tools-extra/clangd/TUScheduler.cpp @@ -979,9 +979,12 @@ // return a compatible preamble as ASTWorker::update blocks. llvm::Optional<ParsedAST> NewAST; if (Invocation) { + std::shared_ptr<const ASTSignals> Signals; + std::shared_ptr<const PreambleData> Preamble = + getPossiblyStalePreamble(&Signals); NewAST = ParsedAST::build(FileName, FileInputs, std::move(Invocation), CompilerInvocationDiagConsumer.take(), - getPossiblyStalePreamble()); + Preamble, Signals); ++ASTBuildCount; } AST = NewAST ? std::make_unique<ParsedAST>(std::move(*NewAST)) : nullptr; @@ -1188,8 +1191,11 @@ IdleASTs.take(this, &ASTAccessForDiag); if (!AST || !InputsAreLatest) { auto RebuildStartTime = DebouncePolicy::clock::now(); - llvm::Optional<ParsedAST> NewAST = ParsedAST::build( - FileName, Inputs, std::move(Invocation), CIDiags, *LatestPreamble); + std::shared_ptr<const ASTSignals> Signals; + getPossiblyStalePreamble(&Signals); + llvm::Optional<ParsedAST> NewAST = + ParsedAST::build(FileName, Inputs, std::move(Invocation), CIDiags, + *LatestPreamble, Signals); auto RebuildDuration = DebouncePolicy::clock::now() - RebuildStartTime; ++ASTBuildCount; // Try to record the AST-build time, to inform future update debouncing. Index: clang-tools-extra/clangd/ParsedAST.h =================================================================== --- clang-tools-extra/clangd/ParsedAST.h +++ clang-tools-extra/clangd/ParsedAST.h @@ -40,6 +40,7 @@ namespace clang { class Sema; namespace clangd { +struct ASTSignals; class HeuristicResolver; /// Stores and provides access to parsed AST. @@ -52,7 +53,8 @@ build(llvm::StringRef Filename, const ParseInputs &Inputs, std::unique_ptr<clang::CompilerInvocation> CI, llvm::ArrayRef<Diag> CompilerInvocationDiags, - std::shared_ptr<const PreambleData> Preamble); + std::shared_ptr<const PreambleData> Preamble, + std::shared_ptr<const ASTSignals> Signals); ParsedAST(ParsedAST &&Other); ParsedAST &operator=(ParsedAST &&Other); @@ -85,7 +87,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<Decl *> getLocalTopLevelDecls(); + ArrayRef<Decl *> getLocalTopLevelDecls() const; const llvm::Optional<std::vector<Diag>> &getDiagnostics() const { return Diags; @@ -125,7 +127,8 @@ std::shared_ptr<const PreambleData> Preamble, std::unique_ptr<CompilerInstance> Clang, std::unique_ptr<FrontendAction> Action, syntax::TokenBuffer Tokens, - MainFileMacros Macros, std::vector<PragmaMark> Marks, + std::shared_ptr<const ASTSignals> Signals, MainFileMacros Macros, + std::vector<PragmaMark> Marks, std::vector<Decl *> LocalTopLevelDecls, llvm::Optional<std::vector<Diag>> Diags, IncludeStructure Includes, CanonicalIncludes CanonIncludes); @@ -148,6 +151,8 @@ /// - Does not have spelled or expanded tokens for files from preamble. syntax::TokenBuffer Tokens; + /// Signals for the main file, if available. + std::shared_ptr<const ASTSignals> Signals; /// All macro definitions and expansions in the main file. MainFileMacros Macros; // Pragma marks in the main file. 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" @@ -342,7 +344,8 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, std::unique_ptr<clang::CompilerInvocation> CI, llvm::ArrayRef<Diag> CompilerInvocationDiags, - std::shared_ptr<const PreambleData> Preamble) { + std::shared_ptr<const PreambleData> Preamble, + std::shared_ptr<const ASTSignals> Signals) { trace::Span Tracer("BuildAST"); SPAN_ATTACH(Tracer, "File", Filename); @@ -563,8 +566,11 @@ for (const auto &Inc : Preamble->Includes.MainFileIncludes) Inserter->addExisting(Inc); } + Symbol::IncludeDirective Directive = Symbol::IncludeDirective::Include; + if (Signals) + Directive = Signals->InsertionDirective; 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); @@ -669,9 +675,9 @@ } ParsedAST Result(Filename, Inputs.Version, std::move(Preamble), std::move(Clang), std::move(Action), std::move(Tokens), - std::move(Macros), std::move(Marks), std::move(ParsedDecls), - std::move(Diags), std::move(Includes), - std::move(CanonIncludes)); + std::move(Signals), std::move(Macros), std::move(Marks), + std::move(ParsedDecls), std::move(Diags), + std::move(Includes), std::move(CanonIncludes)); if (Result.Diags) { auto UnusedHeadersDiags = issueUnusedIncludesDiagnostics(Result, Inputs.Contents); @@ -715,7 +721,7 @@ return Clang->getPreprocessor(); } -llvm::ArrayRef<Decl *> ParsedAST::getLocalTopLevelDecls() { +llvm::ArrayRef<Decl *> ParsedAST::getLocalTopLevelDecls() const { return LocalTopLevelDecls; } @@ -766,15 +772,17 @@ std::shared_ptr<const PreambleData> Preamble, std::unique_ptr<CompilerInstance> Clang, std::unique_ptr<FrontendAction> Action, - syntax::TokenBuffer Tokens, MainFileMacros Macros, - std::vector<PragmaMark> Marks, + syntax::TokenBuffer Tokens, + std::shared_ptr<const ASTSignals> Signals, + MainFileMacros Macros, std::vector<PragmaMark> Marks, std::vector<Decl *> LocalTopLevelDecls, llvm::Optional<std::vector<Diag>> Diags, IncludeStructure Includes, CanonicalIncludes CanonIncludes) : TUPath(TUPath), Version(Version), Preamble(std::move(Preamble)), Clang(std::move(Clang)), Action(std::move(Action)), - Tokens(std::move(Tokens)), Macros(std::move(Macros)), - Marks(std::move(Marks)), Diags(std::move(Diags)), + Tokens(std::move(Tokens)), Signals(std::move(Signals)), + Macros(std::move(Macros)), Marks(std::move(Marks)), + Diags(std::move(Diags)), LocalTopLevelDecls(std::move(LocalTopLevelDecls)), Includes(std::move(Includes)), CanonIncludes(std::move(CanonIncludes)) { Resolver = std::make_unique<HeuristicResolver>(getASTContext()); 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 @@ -319,10 +319,11 @@ // different scopes from the same header, but this case should be rare and is // thus ignored. llvm::StringSet<> InsertedHeaders; + auto InsertDirective = Directive == Symbol::IncludeDirective::Import ? + tooling::IncludeDirective::Import : tooling::IncludeDirective::Include; 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 +331,7 @@ continue; if (auto Fix = insertHeader(ToInclude->first, (Sym.Scope + Sym.Name).str(), - tooling::IncludeDirective::Include)) + InsertDirective)) 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,18 +391,21 @@ Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted)); }; bool ShouldInsert = C.headerToInsertIfAllowed(Opts).has_value(); + Symbol::IncludeDirective Directive = insertionDirective(Opts); + tooling::IncludeDirective InsertDirective = + Directive == Symbol::Import ? tooling::IncludeDirective::Import + : tooling::IncludeDirective::Include; // 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)) { CodeCompletion::IncludeCandidate Include; Include.Header = ToInclude->first; if (ToInclude->second && ShouldInsert) - Include.Insertion = Includes.insert( - ToInclude->first, tooling::IncludeDirective::Include); + Include.Insertion = + Includes.insert(ToInclude->first, InsertDirective); 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 @@ -10,12 +10,47 @@ #include "AST.h" #include "FindTarget.h" #include "support/Trace.h" +#include "clang/AST/DeclObjC.h" namespace clang { namespace clangd { +namespace { +bool preferImports(const ParsedAST &AST) { + // Always prefer #include for non-ObjC code. + if (!AST.getLangOpts().ObjC) + return false; + // If this is not a header file and has ObjC set as the language, prefer + // #import. + if (!isHeaderFile(AST.tuPath(), AST.getLangOpts())) + return true; + + // 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 : AST.getIncludeStructure().MainFileIncludes) + if (Inc.Directive == tok::pp_import) + return true; + + // 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 : AST.getLocalTopLevelDecls()) + if (isa<ObjCContainerDecl, ObjCIvarDecl, ObjCMethodDecl, ObjCPropertyDecl>( + D)) + return true; + + return false; +} +} // namespace + ASTSignals ASTSignals::derive(const ParsedAST &AST) { trace::Span Span("ASTSignals::derive"); ASTSignals Signals; + Signals.InsertionDirective = preferImports(AST) + ? Symbol::IncludeDirective::Import + : Symbol::IncludeDirective::Include; const SourceManager &SM = AST.getSourceManager(); findExplicitReferences( AST.getASTContext(),
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits