[clangd] Improve code completion for ObjC methods

[dgoldman]

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).

[sammccall]

Thanks for fixing this!

unittests/clangd/CodeCompleteTests.cpp

2195

"Context" is an odd name for this.

2202

this will crash if there aren't any completions, and do something unpredictable if there are multiple.
either grab the completions and then ASSERT_THAT(completions, ElementsAre(_)) or write a getOnlyCompletion() helper that does the needed logging.
(Note an assertion on size isn't as good, as it only prints the size on failure)

[dgoldman]

CodeCompleteTests fixes

[dgoldman]

Since I don't have commit access, @sammccall will land this.

clangd/CodeCompletionStrings.cpp

std::string getDeclComment(const ASTContext &Ctx, const NamedDecl &Decl) {
   assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));
   std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
   return looksLikeDocComment(Doc) ? Doc : "";
 }
 
 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.
     if (isInformativeQualifierChunk(Chunk))
       continue;
 
     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.
+      // 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;
       *Snippet += Chunk.Text;
       break;
     case CodeCompletionString::CK_Optional:
       break;
     case CodeCompletionString::CK_Placeholder:

unittests/clangd/CodeCompleteTests.cpp

CodeCompleteResult completions(ClangdServer &Server, StringRef TestCode,
   auto File = testPath("foo.cpp");
   runAddDocument(Server, File, TestCode);
   auto CompletionList = cantFail(runCodeComplete(Server, File, point, Opts));
   return CompletionList;
 }
 
 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!");
     OverrideIndex = memIndex(std::move(IndexSymbols));
     Opts.Index = OverrideIndex.get();
   }
 
-  auto File = testPath("foo.cpp");
+  auto File = testPath(FilePath);
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
   auto CompletionList =
       cantFail(runCodeComplete(Server, File, Test.point(), Opts));
   return CompletionList;
 }
 
 // Builds a server and runs code completion.
 // 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) {
   std::string Result;
   raw_string_ostream OS(Result);
   std::pair<StringRef, StringRef> Split;
   for (Split = Haystack.split(Needle); !Split.second.empty();
        Split = Split.first.split(Needle))
TEST(CompletionTest, NoCompletionsForNewNames) {
   clangd::CodeCompleteOptions Opts;
   Opts.AllScopes = true;
   auto Results = completions(R"cpp(
       void f() { int n^ }
     )cpp",
                              {cls("naber"), cls("nx::naber")}, Opts);
   EXPECT_THAT(Results.Completions, UnorderedElementsAre());
 }
+
+TEST(CompletionTest, ObjectiveCMethodNoArguments) {
+  std::string Context = R"objc(
+    @interface Foo
+    @property(nonatomic) int value;
+    @end
+    Foo *foo = [Foo new]; int y = [foo v^]
+  )objc";
+
+  auto A = completions(Context, {}, {}, "Foo.m").Completions.front();
+  EXPECT_EQ(A.Name, "value");
+  EXPECT_EQ(A.Signature, "");
+  EXPECT_EQ(A.Kind, CompletionItemKind::Method);
+  EXPECT_EQ(A.ReturnType, "int");
+  EXPECT_EQ(A.SnippetSuffix, "");
+}
+
+TEST(CompletionTest, ObjectiveCMethodOneArgument) {
+  std::string Context = R"objc(
+    @interface Foo
+    - (int)valueForCharacter:(char)c;
+    @end
+    Foo *foo = [Foo new]; int y = [foo v^]
+  )objc";
+
+  auto A = completions(Context, {}, {}, "Foo.m").Completions.front();
+  EXPECT_EQ(A.Name, "valueForCharacter:");
+  EXPECT_EQ(A.Signature, "(char)");
+  EXPECT_EQ(A.Kind, CompletionItemKind::Method);
+  EXPECT_EQ(A.ReturnType, "int");
+  EXPECT_EQ(A.SnippetSuffix, "${1:(char)}");
+}
+
+TEST(CompletionTest, ObjectiveCMethodTwoArgumentsFromBeginning) {
+  std::string Context = R"objc(
+    @interface Foo
+    + (id)fooWithValue:(int)value fooey:(unsigned int)fooey;
+    @end
+    id val = [Foo foo^]
+  )objc";
+
+  auto A = completions(Context, {}, {}, "Foo.m").Completions.front();
+  EXPECT_EQ(A.Name, "fooWithValue:");
+  EXPECT_EQ(A.Signature, "(int) fooey:(unsigned int)");
+  EXPECT_EQ(A.Kind, CompletionItemKind::Method);
+  EXPECT_EQ(A.ReturnType, "id");
+  EXPECT_EQ(A.SnippetSuffix, "${1:(int)} fooey:${2:(unsigned int)}");
+}
+
+TEST(CompletionTest, ObjectiveCMethodTwoArgumentsFromMiddle) {
+  std::string Context = R"objc(
+    @interface Foo
+    + (id)fooWithValue:(int)value fooey:(unsigned int)fooey;
+    @end
+    id val = [Foo fooWithValue:10 f^]
+  )objc";
+
+  auto A = completions(Context, {}, {}, "Foo.m").Completions.front();
+  EXPECT_EQ(A.Name, "fooey:");
+  EXPECT_EQ(A.Signature, "(unsigned int)");
+  EXPECT_EQ(A.Kind, CompletionItemKind::Method);
+  EXPECT_EQ(A.ReturnType, "id");
+  EXPECT_EQ(A.SnippetSuffix, "${1:(unsigned int)}");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

unittests/clangd/CodeCompletionStringsTests.cpp

TEST_F(CompletionStringTest, IgnoreInformativeQualifier) {
   Builder.AddTypedTextChunk("X");
   Builder.AddInformativeChunk("info ok");
   Builder.AddInformativeChunk("info no no::");
   computeSignature(*Builder.TakeString());
   EXPECT_EQ(Signature, "info ok");
   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