Author: Sam McCall Date: 2020-02-04T17:24:26+01:00 New Revision: 2629035a009095f62f48413e175437261165ecd7
URL: https://github.com/llvm/llvm-project/commit/2629035a009095f62f48413e175437261165ecd7 DIFF: https://github.com/llvm/llvm-project/commit/2629035a009095f62f48413e175437261165ecd7.diff LOG: [clangd] Don't assert when completing a lambda variable inside the lambda. Summary: This is a fairly ugly hack - we back off several features for any variable whose type isn't deduced, to avoid computing/caching linkage. Better suggestions welcome. Fixes https://github.com/clangd/clangd/issues/274 Reviewers: kadircet, kbobyrev Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D73960 Added: Modified: clang-tools-extra/clangd/AST.cpp clang-tools-extra/clangd/AST.h clang-tools-extra/clangd/CodeComplete.cpp clang-tools-extra/clangd/Quality.cpp clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp index c800ee870dc9..836eb6a36459 100644 --- a/clang-tools-extra/clangd/AST.cpp +++ b/clang-tools-extra/clangd/AST.cpp @@ -473,5 +473,12 @@ std::string getQualification(ASTContext &Context, }); } +bool hasUnstableLinkage(const Decl *D) { + // Linkage of a ValueDecl depends on the type. + // If that's not deduced yet, deducing it may change the linkage. + auto *VD = llvm::dyn_cast_or_null<ValueDecl>(D); + return VD && !VD->getType().isNull() && VD->getType()->isUndeducedType(); +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h index 6cae719986a0..a40aeaf32a77 100644 --- a/clang-tools-extra/clangd/AST.h +++ b/clang-tools-extra/clangd/AST.h @@ -148,6 +148,21 @@ std::string getQualification(ASTContext &Context, const NamedDecl *ND, llvm::ArrayRef<std::string> VisibleNamespaces); +/// Whether we must avoid computing linkage for D during code completion. +/// Clang aggressively caches linkage computation, which is stable after the AST +/// is built. Unfortunately the AST is incomplete during code completion, so +/// linkage may still change. +/// +/// Example: `auto x = []{^}` at file scope. +/// During code completion, the initializer for x hasn't been parsed yet. +/// x has type `undeduced auto`, and external linkage. +/// If we compute linkage at this point, the external linkage will be cached. +/// +/// After code completion the initializer is attached, and x has a lambda type. +/// This means x has "unique external" linkage. If we computed linkage above, +/// the cached value is incorrect. (clang catches this with an assertion). +bool hasUnstableLinkage(const Decl *D); + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp index 830d4b224413..4d4d15b0efae 100644 --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -489,6 +489,9 @@ llvm::Optional<SymbolID> getSymbolID(const CodeCompletionResult &R, switch (R.Kind) { case CodeCompletionResult::RK_Declaration: case CodeCompletionResult::RK_Pattern: { + // Computing USR caches linkage, which may change after code completion. + if (hasUnstableLinkage(R.Declaration)) + return llvm::None; return clang::clangd::getSymbolID(R.Declaration); } case CodeCompletionResult::RK_Macro: @@ -1001,10 +1004,12 @@ class SignatureHelpCollector final : public CodeCompleteConsumer { ScoredSignature Result; Result.Signature = std::move(Signature); Result.Quality = Signal; - Result.IDForDoc = - Result.Signature.documentation.empty() && Candidate.getFunction() - ? clangd::getSymbolID(Candidate.getFunction()) - : None; + const FunctionDecl *Func = Candidate.getFunction(); + if (Func && Result.Signature.documentation.empty()) { + // Computing USR caches linkage, which may change after code completion. + if (!hasUnstableLinkage(Func)) + Result.IDForDoc = clangd::getSymbolID(Func); + } return Result; } diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp index bd25256904cd..d80790fc9808 100644 --- a/clang-tools-extra/clangd/Quality.cpp +++ b/clang-tools-extra/clangd/Quality.cpp @@ -275,8 +275,9 @@ computeScope(const NamedDecl *D) { } if (InClass) return SymbolRelevanceSignals::ClassScope; - // This threshold could be tweaked, e.g. to treat module-visible as global. - if (D->getLinkageInternal() < ExternalLinkage) + // ExternalLinkage threshold could be tweaked, e.g. module-visible as global. + // Avoid caching linkage if it may change after enclosing code completion. + if (hasUnstableLinkage(D) || D->getLinkageInternal() < ExternalLinkage) return SymbolRelevanceSignals::FileScope; return SymbolRelevanceSignals::GlobalScope; } diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp index 1a9950aeb8aa..8863d9f278d7 100644 --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -2664,6 +2664,17 @@ TEST(CompletionTest, DerivedMethodsAreAlwaysVisible) { ElementsAre(AllOf(ReturnType("int"), Named("size")))); } +TEST(CompletionTest, NoCrashWithIncompleteLambda) { + auto Completions = completions("auto&& x = []{^").Completions; + // The completion of x itself can cause a problem: in the code completion + // callback, its type is not known, which affects the linkage calculation. + // A bad linkage value gets cached, and subsequently updated. + EXPECT_THAT(Completions, Contains(Named("x"))); + + auto Signatures = signatures("auto x() { x(^").signatures; + EXPECT_THAT(Signatures, Contains(Sig("x() -> auto"))); +} + TEST(NoCompileCompletionTest, Basic) { auto Results = completionsNoCompile(R"cpp( void func() { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
