diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -164,6 +164,9 @@ /// If true, use the dirty buffer contents when building Preambles. bool UseDirtyHeaders = false; + // If true, parse emplace-like functions in the preamble. + bool PreambleParseForwardingFunctions = false; + explicit operator TUScheduler::Options() const; }; // Sensible default options for use in tests. @@ -416,6 +419,8 @@ bool UseDirtyHeaders = false; + bool PreambleParseForwardingFunctions = false; + // GUARDED_BY(CachedCompletionFuzzyFindRequestMutex) llvm::StringMap> CachedCompletionFuzzyFindRequestByFile; diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -148,7 +148,9 @@ : FeatureModules(Opts.FeatureModules), CDB(CDB), TFS(TFS), DynamicIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex() : nullptr), ClangTidyProvider(Opts.ClangTidyProvider), - UseDirtyHeaders(Opts.UseDirtyHeaders), WorkspaceRoot(Opts.WorkspaceRoot), + UseDirtyHeaders(Opts.UseDirtyHeaders), + PreambleParseForwardingFunctions(Opts.PreambleParseForwardingFunctions), + WorkspaceRoot(Opts.WorkspaceRoot), Transient(Opts.ImplicitCancellation ? TUScheduler::InvalidateOnUpdate : TUScheduler::NoInvalidation), DirtyFS(std::make_unique(TFS, DraftMgr)) { @@ -217,6 +219,7 @@ WantDiagnostics WantDiags, bool ForceRebuild) { std::string ActualVersion = DraftMgr.addDraft(File, Version, Contents); ParseOptions Opts; + Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions; // Compile command is set asynchronously during update, as it can be slow. ParseInputs Inputs; @@ -910,7 +913,7 @@ // It's safe to pass in the TU, as dumpAST() does not // deserialize the preamble. auto Node = DynTypedNode::create( - *Inputs->AST.getASTContext().getTranslationUnitDecl()); + *Inputs->AST.getASTContext().getTranslationUnitDecl()); return CB(dumpAST(Node, Inputs->AST.getTokens(), Inputs->AST.getASTContext())); } diff --git a/clang-tools-extra/clangd/Compiler.h b/clang-tools-extra/clangd/Compiler.h --- a/clang-tools-extra/clangd/Compiler.h +++ b/clang-tools-extra/clangd/Compiler.h @@ -39,7 +39,7 @@ // Options to run clang e.g. when parsing AST. struct ParseOptions { - // (empty at present, formerly controlled recovery AST, include-fixer etc) + bool PreambleParseForwardingFunctions = false; }; /// Information required to run clang, e.g. to parse AST or do code completion. diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -66,9 +66,10 @@ public: CppFilePreambleCallbacks( PathRef File, PreambleParsedCallback ParsedCallback, - PreambleBuildStats *Stats, + PreambleBuildStats *Stats, bool ParseForwardingFunctions, std::function BeforeExecuteCallback) : File(File), ParsedCallback(ParsedCallback), Stats(Stats), + ParseForwardingFunctions(ParseForwardingFunctions), BeforeExecuteCallback(std::move(BeforeExecuteCallback)) {} IncludeStructure takeIncludes() { return std::move(Includes); } @@ -136,15 +137,48 @@ return IWYUHandler.get(); } + static bool isLikelyForwardingFunction(FunctionTemplateDecl *FT) { + const auto *FD = FT->getTemplatedDecl(); + const auto NumParams = FD->getNumParams(); + // Check whether its last parameter is a parameter pack... + if (NumParams > 0) { + const auto *LastParam = FD->getParamDecl(NumParams - 1); + if (const auto *PET = dyn_cast(LastParam->getType())) { + // ... of the type T&&... or T... + const auto BaseType = PET->getPattern().getNonReferenceType(); + if (const auto *TTPT = + dyn_cast(BaseType.getTypePtr())) { + // ... whose template parameter comes from the function directly + if (FT->getTemplateParameters()->getDepth() == TTPT->getDepth()) { + return true; + } + } + } + } + return false; + } + bool shouldSkipFunctionBody(Decl *D) override { - // Generally we skip function bodies in preambles for speed. - // We can make exceptions for functions that are cheap to parse and - // instantiate, widely used, and valuable (e.g. commonly produce errors). - if (const auto *FT = llvm::dyn_cast(D)) { - if (const auto *II = FT->getDeclName().getAsIdentifierInfo()) - // std::make_unique is trivial, and we diagnose bad constructor calls. - if (II->isStr("make_unique") && FT->isInStdNamespace()) + // Usually we don't need to look inside the bodies of header functions + // to understand the program. However when forwarding function like + // emplace() forward their arguments to some other function, the + // interesting overload resolution happens inside the forwarding + // function's body. To provide more meaningful diagnostics, + // code completion, and parameter hints we should parse (and later + // instantiate) the bodies. + if (auto *FT = llvm::dyn_cast(D)) { + if (ParseForwardingFunctions) { + // Don't skip parsing the body if it looks like a forwarding function + if (isLikelyForwardingFunction(FT)) return false; + } else { + // By default, only take care of make_unique + // std::make_unique is trivial, and we diagnose bad constructor calls. + if (const auto *II = FT->getDeclName().getAsIdentifierInfo()) { + if (II->isStr("make_unique") && FT->isInStdNamespace()) + return false; + } + } } return true; } @@ -161,6 +195,7 @@ const clang::LangOptions *LangOpts = nullptr; const SourceManager *SourceMgr = nullptr; PreambleBuildStats *Stats; + bool ParseForwardingFunctions; std::function BeforeExecuteCallback; }; @@ -484,11 +519,13 @@ // to read back. We rely on dynamic index for the comments instead. CI.getPreprocessorOpts().WriteCommentListToPCH = false; - CppFilePreambleCallbacks CapturedInfo(FileName, PreambleCallback, Stats, - [&ASTListeners](CompilerInstance &CI) { - for (const auto &L : ASTListeners) - L->beforeExecute(CI); - }); + CppFilePreambleCallbacks CapturedInfo( + FileName, PreambleCallback, Stats, + Inputs.Opts.PreambleParseForwardingFunctions, + [&ASTListeners](CompilerInstance &CI) { + for (const auto &L : ASTListeners) + L->beforeExecute(CI); + }); auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory); llvm::SmallString<32> AbsFileName(FileName); VFS->makeAbsolute(AbsFileName); diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp --- a/clang-tools-extra/clangd/tool/Check.cpp +++ b/clang-tools-extra/clangd/tool/Check.cpp @@ -128,6 +128,8 @@ Inputs.CompileCommand = Cmd; Inputs.TFS = &TFS; Inputs.ClangTidyProvider = Opts.ClangTidyProvider; + Inputs.Opts.PreambleParseForwardingFunctions = + Opts.PreambleParseForwardingFunctions; if (Contents.hasValue()) { Inputs.Contents = *Contents; log("Imaginary source file contents:\n{0}", Inputs.Contents); diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -497,6 +497,14 @@ Hidden, init(ClangdServer::Options().UseDirtyHeaders)}; +opt PreambleParseForwardingFunctions{ + "parse-forwarding-functions", + cat(Misc), + desc("Parse all emplace-like functions in included headers"), + Hidden, + init(ParseOptions().PreambleParseForwardingFunctions), +}; + #if defined(__GLIBC__) && CLANGD_MALLOC_TRIM opt EnableMallocTrim{ "malloc-trim", @@ -934,6 +942,7 @@ Opts.ClangTidyProvider = ClangTidyOptProvider; } Opts.UseDirtyHeaders = UseDirtyHeaders; + Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions; Opts.QueryDriverGlobs = std::move(QueryDriverGlobs); Opts.TweakFilter = [&](const Tweak &T) { if (T.hidden() && !HiddenFeatures) diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -389,7 +389,7 @@ namespace std { // These mocks aren't quite right - we omit unique_ptr for simplicity. // forward is included to show its body is not needed to get the diagnostic. - template T&& forward(T& t) { return static_cast(t); } + template T&& forward(T& t); template T* make_unique(A&&... args) { return new T(std::forward(args)...); } @@ -402,6 +402,32 @@ "no matching constructor for initialization of 'S'"))); } +TEST(DiagnosticTest, MakeShared) { + // We usually miss diagnostics from header functions as we don't parse them. + // std::make_shared is only parsed when --parse-forwarding-functions is set + Annotations Main(R"cpp( + struct S { S(char*); }; + auto x = std::[[make_shared]](42); // error-ok + )cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.HeaderCode = R"cpp( + namespace std { + // These mocks aren't quite right - we omit shared_ptr for simplicity. + // forward is included to show its body is not needed to get the diagnostic. + template T&& forward(T& t); + template T* make_shared(A&&... args) { + return new T(std::forward(args)...); + } + } + )cpp"; + TU.ParseOpts.PreambleParseForwardingFunctions = true; + EXPECT_THAT(*TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), + "in template: " + "no matching constructor for initialization of 'S'"))); +} + TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) { Annotations Main(R"cpp( template struct Foo { diff --git a/clang-tools-extra/clangd/unittests/TestTU.h b/clang-tools-extra/clangd/unittests/TestTU.h --- a/clang-tools-extra/clangd/unittests/TestTU.h +++ b/clang-tools-extra/clangd/unittests/TestTU.h @@ -66,6 +66,9 @@ // Simulate a header guard of the header (using an #import directive). bool ImplicitHeaderGuard = true; + // Parse options pass on to the ParseInputs + ParseOptions ParseOpts = {}; + // 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. diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp --- a/clang-tools-extra/clangd/unittests/TestTU.cpp +++ b/clang-tools-extra/clangd/unittests/TestTU.cpp @@ -109,6 +109,7 @@ ParsedAST TestTU::build() const { MockFS FS; auto Inputs = inputs(FS); + Inputs.Opts = ParseOpts; StoreDiags Diags; auto CI = buildCompilerInvocation(Inputs, Diags); assert(CI && "Failed to build compilation invocation.");