Author: sammccall Date: Wed Nov 14 01:05:19 2018 New Revision: 346836 URL: http://llvm.org/viewvc/llvm-project?rev=346836&view=rev Log: [clangd] Improve code completion for ObjC methods
Summary: Previously code completion did not work well for Objective-C methods which contained multiple arguments as clangd did not expect to see multiple typed-text chunks when handling code completion. Note that even with this change, we do not consider selector fragments from previous arguments to be part of the signature (although we could in the future). Reviewers: sammccall Reviewed By: sammccall Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, jfb, kadircet, cfe-commits Differential Revision: https://reviews.llvm.org/D53934 Modified: clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp clang-tools-extra/trunk/unittests/clangd/CodeCompletionStringsTests.cpp Modified: clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp?rev=346836&r1=346835&r2=346836&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp (original) +++ clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp Wed Nov 14 01:05:19 2018 @@ -76,6 +76,7 @@ std::string getDeclComment(const ASTCont void getSignature(const CodeCompletionString &CCS, std::string *Signature, std::string *Snippet, std::string *RequiredQualifiers) { unsigned ArgCount = 0; + bool HadObjCArguments = false; for (const auto &Chunk : CCS) { // Informative qualifier chunks only clutter completion results, skip // them. @@ -85,13 +86,36 @@ void getSignature(const CodeCompletionSt switch (Chunk.Kind) { case CodeCompletionString::CK_TypedText: // The typed-text chunk is the actual name. We don't record this chunk. - // In general our string looks like <qualifiers><name><signature>. - // So once we see the name, any text we recorded so far should be - // reclassified as qualifiers. - if (RequiredQualifiers) - *RequiredQualifiers = std::move(*Signature); - Signature->clear(); - Snippet->clear(); + // C++: + // In general our string looks like <qualifiers><name><signature>. + // So once we see the name, any text we recorded so far should be + // reclassified as qualifiers. + // + // Objective-C: + // Objective-C methods may have multiple typed-text chunks, so we must + // treat them carefully. For Objective-C methods, all typed-text chunks + // will end in ':' (unless there are no arguments, in which case we + // can safely treat them as C++). + if (!StringRef(Chunk.Text).endswith(":")) { // Treat as C++. + if (RequiredQualifiers) + *RequiredQualifiers = std::move(*Signature); + Signature->clear(); + Snippet->clear(); + } else { // Objective-C method with args. + // If this is the first TypedText to the Objective-C method, discard any + // text that we've previously seen (such as previous parameter selector, + // which will be marked as Informative text). + // + // TODO: Make previous parameters part of the signature for Objective-C + // methods. + if (!HadObjCArguments) { + HadObjCArguments = true; + Signature->clear(); + } else { // Subsequent argument, considered part of snippet/signature. + *Signature += Chunk.Text; + *Snippet += Chunk.Text; + } + } break; case CodeCompletionString::CK_Text: *Signature += Chunk.Text; Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=346836&r1=346835&r2=346836&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Wed Nov 14 01:05:19 2018 @@ -67,6 +67,7 @@ MATCHER(InsertInclude, "") { } MATCHER_P(SnippetSuffix, Text, "") { return arg.SnippetSuffix == Text; } MATCHER_P(Origin, OriginSet, "") { return arg.Origin == OriginSet; } +MATCHER_P(Signature, S, "") { return arg.Signature == S; } // Shorthand for Contains(Named(Name)). Matcher<const std::vector<CodeCompletion> &> Has(std::string Name) { @@ -105,7 +106,8 @@ CodeCompleteResult completions(ClangdSer CodeCompleteResult completions(ClangdServer &Server, StringRef Text, std::vector<Symbol> IndexSymbols = {}, - clangd::CodeCompleteOptions Opts = {}) { + clangd::CodeCompleteOptions Opts = {}, + PathRef FilePath = "foo.cpp") { std::unique_ptr<SymbolIndex> OverrideIndex; if (!IndexSymbols.empty()) { assert(!Opts.Index && "both Index and IndexSymbols given!"); @@ -113,7 +115,7 @@ CodeCompleteResult completions(ClangdSer Opts.Index = OverrideIndex.get(); } - auto File = testPath("foo.cpp"); + auto File = testPath(FilePath); Annotations Test(Text); runAddDocument(Server, File, Test.code()); auto CompletionList = @@ -125,12 +127,14 @@ CodeCompleteResult completions(ClangdSer // If IndexSymbols is non-empty, an index will be built and passed to opts. CodeCompleteResult completions(StringRef Text, std::vector<Symbol> IndexSymbols = {}, - clangd::CodeCompleteOptions Opts = {}) { + clangd::CodeCompleteOptions Opts = {}, + PathRef FilePath = "foo.cpp") { MockFSProvider FS; MockCompilationDatabase CDB; IgnoreDiagnostics DiagConsumer; ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest()); - return completions(Server, Text, std::move(IndexSymbols), std::move(Opts)); + return completions(Server, Text, std::move(IndexSymbols), std::move(Opts), + FilePath); } std::string replace(StringRef Haystack, StringRef Needle, StringRef Repl) { @@ -2204,6 +2208,84 @@ TEST(CompletionTest, NoCompletionsForNew {cls("naber"), cls("nx::naber")}, Opts); EXPECT_THAT(Results.Completions, UnorderedElementsAre()); } + +TEST(CompletionTest, ObjectiveCMethodNoArguments) { + auto Results = completions(R"objc( + @interface Foo + @property(nonatomic, setter=setXToIgnoreComplete:) int value; + @end + Foo *foo = [Foo new]; int y = [foo v^] + )objc", + /*IndexSymbols=*/{}, + /*Opts=*/{}, + "Foo.m"); + + auto C = Results.Completions; + EXPECT_THAT(C, ElementsAre(Named("value"))); + EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method))); + EXPECT_THAT(C, ElementsAre(ReturnType("int"))); + EXPECT_THAT(C, ElementsAre(Signature(""))); + EXPECT_THAT(C, ElementsAre(SnippetSuffix(""))); +} + +TEST(CompletionTest, ObjectiveCMethodOneArgument) { + auto Results = completions(R"objc( + @interface Foo + - (int)valueForCharacter:(char)c; + @end + Foo *foo = [Foo new]; int y = [foo v^] + )objc", + /*IndexSymbols=*/{}, + /*Opts=*/{}, + "Foo.m"); + + auto C = Results.Completions; + EXPECT_THAT(C, ElementsAre(Named("valueForCharacter:"))); + EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method))); + EXPECT_THAT(C, ElementsAre(ReturnType("int"))); + EXPECT_THAT(C, ElementsAre(Signature("(char)"))); + EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(char)}"))); +} + +TEST(CompletionTest, ObjectiveCMethodTwoArgumentsFromBeginning) { + auto Results = completions(R"objc( + @interface Foo + + (id)fooWithValue:(int)value fooey:(unsigned int)fooey; + @end + id val = [Foo foo^] + )objc", + /*IndexSymbols=*/{}, + /*Opts=*/{}, + "Foo.m"); + + auto C = Results.Completions; + EXPECT_THAT(C, ElementsAre(Named("fooWithValue:"))); + EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method))); + EXPECT_THAT(C, ElementsAre(ReturnType("id"))); + EXPECT_THAT(C, ElementsAre(Signature("(int) fooey:(unsigned int)"))); + EXPECT_THAT(C, + ElementsAre(SnippetSuffix("${1:(int)} fooey:${2:(unsigned int)}"))); +} + +TEST(CompletionTest, ObjectiveCMethodTwoArgumentsFromMiddle) { + auto Results = completions(R"objc( + @interface Foo + + (id)fooWithValue:(int)value fooey:(unsigned int)fooey; + @end + id val = [Foo fooWithValue:10 f^] + )objc", + /*IndexSymbols=*/{}, + /*Opts=*/{}, + "Foo.m"); + + auto C = Results.Completions; + EXPECT_THAT(C, ElementsAre(Named("fooey:"))); + EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method))); + EXPECT_THAT(C, ElementsAre(ReturnType("id"))); + EXPECT_THAT(C, ElementsAre(Signature("(unsigned int)"))); + EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(unsigned int)}"))); +} + } // namespace } // namespace clangd } // namespace clang Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompletionStringsTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompletionStringsTests.cpp?rev=346836&r1=346835&r2=346836&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/CodeCompletionStringsTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/CodeCompletionStringsTests.cpp Wed Nov 14 01:05:19 2018 @@ -109,6 +109,53 @@ TEST_F(CompletionStringTest, IgnoreInfor EXPECT_EQ(Snippet, ""); } +TEST_F(CompletionStringTest, ObjectiveCMethodNoArguments) { + Builder.AddResultTypeChunk("void"); + Builder.AddTypedTextChunk("methodName"); + + auto *CCS = Builder.TakeString(); + computeSignature(*CCS); + EXPECT_EQ(Signature, ""); + EXPECT_EQ(Snippet, ""); +} + +TEST_F(CompletionStringTest, ObjectiveCMethodOneArgument) { + Builder.AddResultTypeChunk("void"); + Builder.AddTypedTextChunk("methodWithArg:"); + Builder.AddPlaceholderChunk("(type)"); + + auto *CCS = Builder.TakeString(); + computeSignature(*CCS); + EXPECT_EQ(Signature, "(type)"); + EXPECT_EQ(Snippet, "${1:(type)}"); +} + +TEST_F(CompletionStringTest, ObjectiveCMethodTwoArgumentsFromBeginning) { + Builder.AddResultTypeChunk("int"); + Builder.AddTypedTextChunk("withFoo:"); + Builder.AddPlaceholderChunk("(type)"); + Builder.AddChunk(CodeCompletionString::CK_HorizontalSpace); + Builder.AddTypedTextChunk("bar:"); + Builder.AddPlaceholderChunk("(type2)"); + + auto *CCS = Builder.TakeString(); + computeSignature(*CCS); + EXPECT_EQ(Signature, "(type) bar:(type2)"); + EXPECT_EQ(Snippet, "${1:(type)} bar:${2:(type2)}"); +} + +TEST_F(CompletionStringTest, ObjectiveCMethodTwoArgumentsFromMiddle) { + Builder.AddResultTypeChunk("int"); + Builder.AddInformativeChunk("withFoo:"); + Builder.AddTypedTextChunk("bar:"); + Builder.AddPlaceholderChunk("(type2)"); + + auto *CCS = Builder.TakeString(); + computeSignature(*CCS); + EXPECT_EQ(Signature, "(type2)"); + EXPECT_EQ(Snippet, "${1:(type2)}"); +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits