This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG2629035a0090: [clangd] Don't assert when completing a
lambda variable inside the lambda. (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D73960?vs=242311&id=242344#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73960/new/
https://reviews.llvm.org/D73960
Files:
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
Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -2664,6 +2664,17 @@
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() {
Index: clang-tools-extra/clangd/Quality.cpp
===================================================================
--- clang-tools-extra/clangd/Quality.cpp
+++ clang-tools-extra/clangd/Quality.cpp
@@ -275,8 +275,9 @@
}
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;
}
Index: clang-tools-extra/clangd/CodeComplete.cpp
===================================================================
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -489,6 +489,9 @@
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 @@
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;
}
Index: clang-tools-extra/clangd/AST.h
===================================================================
--- clang-tools-extra/clangd/AST.h
+++ clang-tools-extra/clangd/AST.h
@@ -148,6 +148,21 @@
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
Index: clang-tools-extra/clangd/AST.cpp
===================================================================
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -473,5 +473,12 @@
});
}
+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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
