[clangd][ObjC] Fix issue completing a method decl by name

[dgoldman]

Summary

When completing an Objective-C method declaration by name, we need to
preserve the leading text as a qualifier so we insert it properly
before the first typed text chunk.

[dgoldman]

Minor comment update

[sammccall]

The test results look good, but I have trouble following the code, I think more comments might help :-)

clang-tools-extra/clangd/CodeCompletionStrings.cpp

140

can we say something more concrete than "the prefix of our snippet"? this is the -(void) part right?

151

is this TODO handled now?

155

brief comment here should explain the significance of !HadInformativeArguments (what it means if we know we're handling objc)

Maybe even worth an example here, not obvious to me what the data is that we're shuffling between variables here.

203

informative chunks? Not sure how we'd know they're arguments here.

[dgoldman]

Address review comments

clang-tools-extra/clangd/CodeCompletionStrings.cpp

151

No, this does not change any behavior for parameters (we don't show the full ObjcMethodDecl signature including previous parameter types if you complete it from the middle)

[dgoldman]

Friendly ping

[sammccall]

Thanks for the detailed comments, this makes a lot more sense to me now!

clang-tools-extra/clangd/CodeCompletionStrings.cpp

160

Does completing the no-args declaration - (void) foo work as expected?

We never get to this part of the code in this case because endswith(":") is never true. The comment above says "safe to treat as c++" but not sure this is true for declaration, just method calls. Maybe it works for some other reason though?

In any case, it's OK if this case doesn't work (this patch still improves things a lot). We should probably have a test showing what the state is.

[dgoldman]

Add another test for a simple method decl (no arg selector)

[dgoldman]

clang-tools-extra/clangd/CodeCompletionStrings.cpp

160

Yeah, it does. I added a test case for it, it does work, the current behavior is fine, the issue for ObjC is specifically selectors with arguments.

clang-tools-extra/clangd/CodeCompletionStrings.cpp

if (CompletingPattern) {
     //    }
     CursorSnippetArg =
         llvm::count_if(CCS, [](const CodeCompletionString::Chunk &C) {
           return C.Kind == CodeCompletionString::CK_Placeholder;
         });
   }
   unsigned SnippetArg = 0;
   bool HadObjCArguments = false;
+  bool HadInformativeChunks = 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.
       // 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++).
+      //   Objective-C methods expressions may have multiple typed-text chunks,
+      //   so we must treat them carefully. For Objective-C methods, all
+      //   typed-text and informative chunks will end in ':' (unless there are
+      //   no arguments, in which case we can safely treat them as C++).
+      //
+      //   Completing a method declaration itself (not a method expression) is
+      //   similar except that we use the `RequiredQualifiers` to store the
+      //   text before the selector, e.g. `- (void)`.
       if (!llvm::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;
+          // If we have no previous informative chunks (informative selector
+          // fragments in practice), we treat any previous chunks as
+          // `RequiredQualifiers` so they will be added as a prefix during the
+          // completion.
+          //
+          // e.g. to complete `- (void)doSomething:(id)argument`:
+          // - Completion name: `doSomething:`
+          // - RequiredQualifiers: `- (void)`
+          // - Snippet/Signature suffix: `(id)argument`
+          //
+          // This differs from the case when we're completing a method
+          // expression with a previous informative selector fragment.
+          //
+          // e.g. to complete `[self doSomething:nil ^somethingElse:(id)]`:
+          // - Previous Informative Chunk: `doSomething:`
+          // - Completion name: `somethingElse:`
+          // - Snippet/Signature suffix: `(id)`
+          if (!HadInformativeChunks) {
+            if (RequiredQualifiers)
+              *RequiredQualifiers = std::move(*Signature);
+            Snippet->clear();
+          }
           Signature->clear();
         } else { // Subsequent argument, considered part of snippet/signature.
           *Signature += Chunk.Text;
           *Snippet += Chunk.Text;
         }
       }
       break;
     case CodeCompletionString::CK_Text:
case CodeCompletionString::CK_Placeholder:
       ++SnippetArg;
       *Snippet +=
           "${" +
           std::to_string(SnippetArg == CursorSnippetArg ? 0 : SnippetArg) + ':';
       appendEscapeSnippet(Chunk.Text, Snippet);
       *Snippet += '}';
       break;
     case CodeCompletionString::CK_Informative:
+      HadInformativeChunks = true;
       // For example, the word "const" for a const method, or the name of
       // the base class for methods that are part of the base class.
       *Signature += Chunk.Text;
       // Don't put the informative chunks in the snippet.
       break;
     case CodeCompletionString::CK_ResultType:
       // This is not part of the signature.
       break;

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

TEST(CompletionTest, ObjectiveCMethodTwoArgumentsFromMiddle) {
   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)}")));
 }
 
+TEST(CompletionTest, ObjectiveCSimpleMethodDeclaration) {
+  auto Results = completions(R"objc(
+      @interface Foo
+      - (void)foo;
+      @end
+      @implementation Foo
+      fo^
+      @end
+    )objc",
+                             /*IndexSymbols=*/{},
+                             /*Opts=*/{}, "Foo.m");
+
+  auto C = Results.Completions;
+  EXPECT_THAT(C, ElementsAre(Named("foo")));
+  EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+  EXPECT_THAT(C, ElementsAre(Qualifier("- (void)")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclaration) {
+  auto Results = completions(R"objc(
+      @interface Foo
+      - (int)valueForCharacter:(char)c secondArgument:(id)object;
+      @end
+      @implementation Foo
+      valueFor^
+      @end
+    )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(Qualifier("- (int)")));
+  EXPECT_THAT(C, ElementsAre(Signature("(char)c secondArgument:(id)object")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclarationPrefixTyped) {
+  auto Results = completions(R"objc(
+      @interface Foo
+      - (int)valueForCharacter:(char)c;
+      @end
+      @implementation Foo
+      - (int)valueFor^
+      @end
+    )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(Signature("(char)c")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodDeclarationFromMiddle) {
+  auto Results = completions(R"objc(
+      @interface Foo
+      - (int)valueForCharacter:(char)c secondArgument:(id)object;
+      @end
+      @implementation Foo
+      - (int)valueForCharacter:(char)c second^
+      @end
+    )objc",
+                             /*IndexSymbols=*/{},
+                             /*Opts=*/{}, "Foo.m");
+
+  auto C = Results.Completions;
+  EXPECT_THAT(C, ElementsAre(Named("secondArgument:")));
+  EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+  EXPECT_THAT(C, ElementsAre(Signature("(id)object")));
+}
+
 TEST(CompletionTest, CursorInSnippets) {
   clangd::CodeCompleteOptions Options;
   Options.EnableSnippets = true;
   auto Results = completions(
       R"cpp(
     void while_foo(int a, int b);
     void test() {
       whil^