[clang-tools-extra] 2629035 - [clangd] Don't assert when completing a lambda variable inside the lambda.

[Sam McCall via cfe-commits]

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() {