[clangd] Don't assert when completing a lambda variable inside the lambda.

[sammccall]

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.

[merge_guards_bot]

Unit tests: pass. 62439 tests passed, 0 failed and 845 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 1 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/CodeComplete.cpp

1007–1012

nit: maybe decrease # of Candidate.getFunctions via

if(!doc.empty())
  return Result;
if(const auto *FD = Candidate->getFunction()) {
....
}

clang-tools-extra/clangd/AST.h

 /// Note that this is just textual and might be incorrect. e.g. when there are
 /// two namespaces ns1::a and ns2::a, the function will early exit if "a::" is
 /// present in \p VisibleNamespaces, no matter whether it is from ns1:: or ns2::
 std::string getQualification(ASTContext &Context,
                              const DeclContext *DestContext,
                              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
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_AST_H_

clang-tools-extra/clangd/AST.cpp

return getQualification(
           std::string NS;
           llvm::raw_string_ostream OS(NS);
           NNS->print(OS, Context.getPrintingPolicy());
           return OS.str() == Namespace;
         });
       });
 }
 
+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

clang-tools-extra/clangd/CodeComplete.cpp

 };
 
 // Determine the symbol ID for a Sema code completion result, if possible.
 llvm::Optional<SymbolID> getSymbolID(const CodeCompletionResult &R,
                                      const SourceManager &SM) {
   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:
     return clang::clangd::getSymbolID(R.Macro->getName(), R.MacroDefInfo, SM);
   case CodeCompletionResult::RK_Keyword:
     return None;
   }
   llvm_unreachable("unknown CodeCompletionResult kind");
ScoredSignature processOverloadCandidate(const OverloadCandidate &Candidate,
     if (ReturnType) {
       Signature.label += " -> ";
       Signature.label += ReturnType;
     }
     dlog("Signal for {0}: {1}", Signature, Signal);
     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;
   }
 
   SignatureHelp &SigHelp;
   std::shared_ptr<clang::GlobalCodeCompletionAllocator> Allocator;
   CodeCompletionTUInfo CCTUInfo;
   const SymbolIndex *Index;
 }; // SignatureHelpCollector

clang-tools-extra/clangd/Quality.cpp

computeScope(const NamedDecl *D) {
   bool InClass = false;
   for (; !DC->isFileContext(); DC = DC->getParent()) {
     if (DC->isFunctionOrMethod())
       return SymbolRelevanceSignals::FunctionScope;
     InClass = InClass || DC->isRecord();
   }
   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;
 }
 
 void SymbolRelevanceSignals::merge(const Symbol &IndexResult) {
   SymbolURI = IndexResult.CanonicalDeclaration.FileURI;
   SymbolScope = IndexResult.Scope;
   IsInstanceMember |= isInstanceMember(IndexResult.SymInfo);

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

auto Completions = completions(R"cpp(
 
     auto x = deque().^
   )cpp")
                          .Completions;
   EXPECT_THAT(Completions,
               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() {
       int xyz;
       int abc;
       ^
     }
   )cpp");