From e09c75049854fee251e99c4c3c55f3f391f52a10 Mon Sep 17 00:00:00 2001 From: David Goldman Date: Wed, 31 Aug 2022 13:33:09 -0400 Subject: [PATCH] [clangd][ObjC] Improve completions for protocols + category names - Render protocols as interfaces to differentiate them from classes since a protocol and class can have the same name. Take this one step further though, and only recommend protocols in ObjC protocol completions. - Properly call `includeSymbolFromIndex` even with a cached speculative fuzzy find request - Don't use the index to provide completions for category names, symbols there don't make sense Differential Revision: https://reviews.llvm.org/D132962 --- clang-tools-extra/clangd/CodeComplete.cpp | 41 +++++++----- .../clangd/unittests/CodeCompleteTests.cpp | 75 ++++++++++++++++++++-- clang-tools-extra/clangd/unittests/TestIndex.cpp | 5 ++ clang-tools-extra/clangd/unittests/TestIndex.h | 2 + 4 files changed, 103 insertions(+), 20 deletions(-) diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp index f102358..9d9b0e7 100644 --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -94,10 +94,14 @@ CompletionItemKind toCompletionItemKind(index::SymbolKind Kind) { case SK::Struct: return CompletionItemKind::Struct; case SK::Class: - case SK::Protocol: case SK::Extension: case SK::Union: return CompletionItemKind::Class; + case SK::Protocol: + // Use interface instead of class for differentiation of classes and + // protocols with the same name (e.g. @interface NSObject vs. @protocol + // NSObject). + return CompletionItemKind::Interface; case SK::TypeAlias: // We use the same kind as the VSCode C++ extension. // FIXME: pick a better option when we have one. @@ -712,13 +716,13 @@ bool contextAllowsIndex(enum CodeCompletionContext::Kind K) { case CodeCompletionContext::CCC_Type: case CodeCompletionContext::CCC_ParenthesizedExpression: case CodeCompletionContext::CCC_ObjCInterfaceName: - case CodeCompletionContext::CCC_ObjCCategoryName: case CodeCompletionContext::CCC_Symbol: case CodeCompletionContext::CCC_SymbolOrNewName: return true; case CodeCompletionContext::CCC_OtherWithMacros: case CodeCompletionContext::CCC_DotMemberAccess: case CodeCompletionContext::CCC_ArrowMemberAccess: + case CodeCompletionContext::CCC_ObjCCategoryName: case CodeCompletionContext::CCC_ObjCPropertyAccess: case CodeCompletionContext::CCC_MacroName: case CodeCompletionContext::CCC_MacroNameUse: @@ -1343,6 +1347,22 @@ bool allowIndex(CodeCompletionContext &CC) { llvm_unreachable("invalid NestedNameSpecifier kind"); } +// Should we include a symbol from the index given the completion kind? +// FIXME: Ideally we can filter in the fuzzy find request itself. +bool includeSymbolFromIndex(CodeCompletionContext::Kind Kind, + const Symbol &Sym) { + // Objective-C protocols are only useful in ObjC protocol completions, + // in other places they're confusing, especially when they share the same + // identifier with a class. + if (Sym.SymInfo.Kind == index::SymbolKind::Protocol && + Sym.SymInfo.Lang == index::SymbolLanguage::ObjC) + return Kind == CodeCompletionContext::CCC_ObjCProtocolName; + else if (Kind == CodeCompletionContext::CCC_ObjCProtocolName) + // Don't show anything else in ObjC protocol completions. + return false; + return true; +} + std::future startAsyncFuzzyFind(const SymbolIndex &Index, const FuzzyFindRequest &Req) { return runAsync([&Index, Req]() { @@ -1675,14 +1695,6 @@ private: return Output; } - bool includeSymbolFromIndex(const Symbol &Sym) { - if (CCContextKind == CodeCompletionContext::CCC_ObjCProtocolName) { - return Sym.SymInfo.Lang == index::SymbolLanguage::ObjC && - Sym.SymInfo.Kind == index::SymbolKind::Protocol; - } - return true; - } - SymbolSlab queryIndex() { trace::Span Tracer("Query index"); SPAN_ATTACH(Tracer, "limit", int64_t(Opts.Limit)); @@ -1716,11 +1728,8 @@ private: // Run the query against the index. SymbolSlab::Builder ResultsBuilder; - if (Opts.Index->fuzzyFind(Req, [&](const Symbol &Sym) { - if (includeSymbolFromIndex(Sym)) - ResultsBuilder.insert(Sym); - })) - Incomplete = true; + Incomplete |= Opts.Index->fuzzyFind( + Req, [&](const Symbol &Sym) { ResultsBuilder.insert(Sym); }); return std::move(ResultsBuilder).build(); } @@ -1783,6 +1792,8 @@ private: for (const auto &IndexResult : IndexResults) { if (UsedIndexResults.count(&IndexResult)) continue; + if (!includeSymbolFromIndex(CCContextKind, IndexResult)) + continue; AddToBundles(/*SemaResult=*/nullptr, &IndexResult, nullptr); } // Emit identifier results. diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp index 5050ab2..f2d8328 100644 --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -1493,12 +1493,16 @@ TEST(SignatureHelpTest, StalePreamble) { class IndexRequestCollector : public SymbolIndex { public: + IndexRequestCollector(std::vector Syms = {}) : Symbols(Syms) {} + bool fuzzyFind(const FuzzyFindRequest &Req, llvm::function_ref Callback) const override { std::unique_lock Lock(Mut); Requests.push_back(Req); ReceivedRequestCV.notify_one(); + for (const auto &Sym : Symbols) + Callback(Sym); return true; } @@ -1533,6 +1537,7 @@ public: } private: + std::vector Symbols; // We need a mutex to handle async fuzzy find requests. mutable std::condition_variable ReceivedRequestCV; mutable std::mutex Mut; @@ -3208,14 +3213,74 @@ TEST(CompletionTest, ObjectiveCProtocolFromIndex) { Symbol FoodClass = objcClass("FoodClass"); Symbol SymFood = objcProtocol("Food"); Symbol SymFooey = objcProtocol("Fooey"); + auto Results = completions("id", {SymFood, FoodClass, SymFooey}, + /*Opts=*/{}, "Foo.m"); + + // Should only give protocols for ObjC protocol completions. + EXPECT_THAT(Results.Completions, + UnorderedElementsAre( + AllOf(named("Food"), kind(CompletionItemKind::Interface)), + AllOf(named("Fooey"), kind(CompletionItemKind::Interface)))); + + Results = completions("Fo^", {SymFood, FoodClass, SymFooey}, + /*Opts=*/{}, "Foo.m"); + // Shouldn't give protocols for non protocol completions. + EXPECT_THAT( + Results.Completions, + ElementsAre(AllOf(named("FoodClass"), kind(CompletionItemKind::Class)))); +} + +TEST(CompletionTest, ObjectiveCProtocolFromIndexSpeculation) { + MockFS FS; + MockCompilationDatabase CDB; + ClangdServer Server(CDB, FS, ClangdServer::optsForTest()); + + auto File = testPath("Foo.m"); + Annotations Test(R"cpp( + @protocol Food + @end + id foo; + Foo$2^ bar; + )cpp"); + runAddDocument(Server, File, Test.code()); + clangd::CodeCompleteOptions Opts = {}; + + Symbol FoodClass = objcClass("FoodClass"); + IndexRequestCollector Requests({FoodClass}); + Opts.Index = &Requests; + + auto CompleteAtPoint = [&](StringRef P) { + return cantFail(runCodeComplete(Server, File, Test.point(P), Opts)) + .Completions; + }; + + auto C = CompleteAtPoint("1"); + auto Reqs1 = Requests.consumeRequests(1); + ASSERT_EQ(Reqs1.size(), 1u); + EXPECT_THAT(C, ElementsAre(AllOf(named("Food"), + kind(CompletionItemKind::Interface)))); + + C = CompleteAtPoint("2"); + auto Reqs2 = Requests.consumeRequests(1); + // Speculation succeeded. Used speculative index result, but filtering now to + // now include FoodClass. + ASSERT_EQ(Reqs2.size(), 1u); + EXPECT_EQ(Reqs2[0], Reqs1[0]); + EXPECT_THAT(C, ElementsAre(AllOf(named("FoodClass"), + kind(CompletionItemKind::Class)))); +} + +TEST(CompletionTest, ObjectiveCCategoryFromIndexIgnored) { + Symbol FoodCategory = objcCategory("FoodClass", "Extension"); auto Results = completions(R"objc( - id + @interface Foo + @end + @interface Foo (^) + @end )objc", - {SymFood, FoodClass, SymFooey}, + {FoodCategory}, /*Opts=*/{}, "Foo.m"); - - auto C = Results.Completions; - EXPECT_THAT(C, UnorderedElementsAre(named("Food"), named("Fooey"))); + EXPECT_THAT(Results.Completions, IsEmpty()); } TEST(CompletionTest, CursorInSnippets) { diff --git a/clang-tools-extra/clangd/unittests/TestIndex.cpp b/clang-tools-extra/clangd/unittests/TestIndex.cpp index 6f7bd3a..c247a9c 100644 --- a/clang-tools-extra/clangd/unittests/TestIndex.cpp +++ b/clang-tools-extra/clangd/unittests/TestIndex.cpp @@ -99,6 +99,11 @@ Symbol objcClass(llvm::StringRef Name) { return objcSym(Name, index::SymbolKind::Class, "objc(cs)"); } +Symbol objcCategory(llvm::StringRef Name, llvm::StringRef CategoryName) { + std::string USRPrefix = ("objc(cy)" + Name + "@").str(); + return objcSym(CategoryName, index::SymbolKind::Extension, USRPrefix); +} + Symbol objcProtocol(llvm::StringRef Name) { return objcSym(Name, index::SymbolKind::Protocol, "objc(pl)"); } diff --git a/clang-tools-extra/clangd/unittests/TestIndex.h b/clang-tools-extra/clangd/unittests/TestIndex.h index 6a4d2cb..0cd8a71 100644 --- a/clang-tools-extra/clangd/unittests/TestIndex.h +++ b/clang-tools-extra/clangd/unittests/TestIndex.h @@ -39,6 +39,8 @@ Symbol objcSym(llvm::StringRef Name, index::SymbolKind Kind, llvm::StringRef USRPrefix); // Create an @interface or @implementation. Symbol objcClass(llvm::StringRef Name); +// Create an @interface or @implementation category. +Symbol objcCategory(llvm::StringRef Name, llvm::StringRef CategoryName); // Create an @protocol. Symbol objcProtocol(llvm::StringRef Name); -- 2.7.4