[PATCH] D37101: [clangd] [WIP] Add support for snippet completions

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

Thanks for taking your time to implement this!

We need to add some test cases for new completion features. However, adding them may be a bit of a pain, so feel free to ask for best practices if you'll run into trouble while doing that.
Please also see my inline comments.



================
Comment at: clangd/ClangdUnit.cpp:293
+    // earlier in the sequence.
+    std::stable_sort(Results, Results + NumResults,
+                     [](const CodeCompletionResult &lhs,
----------------
Is this required for snippets?
I suggest we move the removal of `sortText` into a separate commit/discussion.


================
Comment at: clangd/ClangdUnit.cpp:295
+                     [](const CodeCompletionResult &lhs,
+                        const CodeCompletionResult &rhs) -> bool {
+                       return lhs.Priority > rhs.Priority;
----------------
Code style: use `UpperCamelCase` for parameter names


================
Comment at: clangd/ClangdUnit.cpp:299
     for (unsigned I = 0; I != NumResults; ++I) {
-      CodeCompletionResult &Result = Results[I];
-      CodeCompletionString *CCS = Result.CreateCodeCompletionString(
-          S, Context, *Allocator, CCTUInfo,
-          CodeCompleteOpts.IncludeBriefComments);
-      if (CCS) {
+      if (Results[I].Availability == CXAvailability_NotAvailable ||
+          Results[I].Availability == CXAvailability_NotAccessible) {
----------------
Could you please also move this to a separate review?
I think we should show inaccessible items, but they should have a much lower priority than accessible ones. Others may disagree, but let's have this discussion in a separate thread.


================
Comment at: clangd/ClangdUnit.cpp:305
         CompletionItem Item;
-        for (CodeCompletionString::Chunk C : *CCS) {
-          switch (C.Kind) {
-          case CodeCompletionString::CK_ResultType:
-            Item.detail = C.Text;
-            break;
-          case CodeCompletionString::CK_Optional:
-            break;
-          default:
-            Item.label += C.Text;
-            break;
-          }
-        }
-        assert(CCS->getTypedText());
-        Item.kind = getKind(Result.CursorKind);
-        // Priority is a 16-bit integer, hence at most 5 digits.
-        assert(CCS->getPriority() < 99999 && "Expecting code completion result "
-                                             "priority to have at most "
-                                             "5-digits");
-        llvm::raw_string_ostream(Item.sortText)
-            << llvm::format("%05d%s", CCS->getPriority(), CCS->getTypedText());
-        Item.insertText = Item.filterText = CCS->getTypedText();
-        if (CCS->getBriefComment())
-          Item.documentation = CCS->getBriefComment();
+        this->ProcessCodeCompleteResult(S, Context, Results[I], Item);
         Items->push_back(std::move(Item));
----------------
Code style: we usually don't use explicit `this->` qualifiers.


================
Comment at: clangd/ClangdUnit.cpp:316
+private:
+  void ProcessCodeCompleteResult(Sema &S, CodeCompletionContext Context,
+                                 CodeCompletionResult &Result,
----------------
Maybe return `CompletionItem` instead of modifying a reference parameter?


================
Comment at: clangd/ClangdUnit.cpp:316
+private:
+  void ProcessCodeCompleteResult(Sema &S, CodeCompletionContext Context,
+                                 CodeCompletionResult &Result,
----------------
ilya-biryukov wrote:
> Maybe return `CompletionItem` instead of modifying a reference parameter?
Any reason why `Context` is passed by value?  Maybe pass by `const &`?


================
Comment at: clangd/ClangdUnit.cpp:317
+  void ProcessCodeCompleteResult(Sema &S, CodeCompletionContext Context,
+                                 CodeCompletionResult &Result,
+                                 CompletionItem &Item) {
----------------
This function does not modify `Result`. Maybe pass by `const &` instead of `&`?


================
Comment at: clangd/ClangdUnit.cpp:321
+
+    // Fill in the label, detail, documentation and insertText fields of the
+    // CompletionItem.
----------------
Maybe split writes into `Item.label` and other `Item` fields?
That would simplify the function, i.e. something like this:

```
// Set Item.label approriately.
switch (Result.Kind) {
  case RK_Declaration: Item.label = Result.Declaration->getNameAsString();
  //... other enum value
}


// Set other Item fields.
// Note that is also works as expected for RK_Pattern.
auto Completion = Result.CreateCodeCompletionString(/*....*/);
ProcessCodeCompleteString(/*...*/, Item);
// ....
```


================
Comment at: clangd/ClangdUnit.cpp:362
+    // Always a snippet for now.
+    Item.insertTextFormat = InsertTextFormat::Snippet;
+  }
----------------
Maybe only set it to `Snippet` if we encountered `CK_Placeholder`?
That would allow clients of `ClangdServer` API to ignore snippet items if they don't support it. (We have one client like that internally)


================
Comment at: clangd/ClangdUnit.cpp:369
+    for (unsigned j = 0; j < CCS.getAnnotationCount(); ++j) {
+      // Things like __attribute__((nonnull(1,3))). Stash this information in
+      // the documentation field.
----------------
Maybe add some description to the docs? For example.
```
Attributes: [[noreturn]], __attibute__((nonnull(1,3))

// Here goes documentation
```


================
Comment at: clangd/ClangdUnit.cpp:381
+        // a declarator or macro.
+        Item.label += Chunk.Text;
+        Item.insertText += Chunk.Text;
----------------
We set the label in `ProcessCodeCompleteResult`, why is there a need to append to it here?
Maybe we should set it here and not touch it in `ProcessCodeCompleteResult` at all?


================
Comment at: clangd/ClangdUnit.cpp:387
+        // e.g., parentheses or a comma in a function call.
+        Item.label += Chunk.Text;
+        Item.insertText += Chunk.Text;
----------------
Again, do we need to set `label` in `ProcessCodeCompleteResult` or in `ProcessCodeCompleteString`?


================
Comment at: clangd/ClangdUnit.cpp:400
+        Item.insertText +=
+            "${" + std::to_string(ArgCount) + ":" + Chunk.Text + "}";
+        Item.label += Chunk.Text;
----------------
What if `Chunk.Text` contains `}`?


================
Comment at: clangd/ClangdUnit.cpp:406
+        // but should not be inserted into the buffer.
+        Item.documentation += "[" + std::string(Chunk.Text) + "] ";
+        break;
----------------
Let's also add some description to `Item.documentation`. What kind of things go into `CK_Informative` ?



================
Comment at: clangd/ClangdUnit.cpp:413
+        break;
+      case CodeCompletionString::CK_CurrentParameter:
+        // A piece of text that describes the parameter that corresponds
----------------
How is `CK_CurrentParameter` different from `CK_Placeholder`? Maybe handle them both simultaneously and add a comment on why we don't care about their differences?
I mean something like:
```
// We don't distinguish between CK_Placeholder and CK_CurrentParameter because ....
case CodeCompletionString::CK_Placeholder:
case CodeCompletionString::CK_CurrentParameter:
//... Code adding ${N:something}

```


================
Comment at: clangd/ClangdUnit.cpp:452
+        Item.insertText += Chunk.Text;
+        Item.label += Chunk.Text;
+        break;
----------------
Does adding vertical space to `label` plays well with editors? Maybe we should replace with horizontal space instead?


https://reviews.llvm.org/D37101