[PATCH] D41537: Optionally add code completion results for arrow instead of dot

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for the delay. I really like the direction of this patch!
What's left is defining the semantics of corrections more thoroughly to make sure we don't have tricky corner cases that users of the API can't deal with.

PS. This patch is still lacking full context of the diff.
Please use arc <https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line> or send diff with full context <https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface>.



================
Comment at: include/clang-c/Index.h:5220
+CINDEX_LINKAGE CXString
+clang_getCompletionCorrection(CXCompletionString completion_string,
+                              unsigned correction_index,
----------------
I'm a bit vary about exposing source manager and language options in the API just for the sake of corrections.
I suggest we add an extra parameter of an existing class (`CXCodeCompleteResults`) instead and remove the corresponding helpers to get source manager and language options:
`clang_getCompletionNumCorrections(CXCompletionString completion_string, CXCodeCompleteResults* results);`



================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:564
+
+  /// \brief For this completion result correction is required.
+  std::vector<FixItHint> Corrections;
----------------
Adding some docs here would be useful. These fix-its could be interpreted too broadly at this point.
I suggest the following semantics and the comment:

```
/// \brief FixIts that *must* be applied before inserting the text for the corresponding completion item.
/// Completion items with non-empty fixits will not be returned by default, they should be explicitly requested by setting CompletionOptions::IncludeCorrections.
/// For the editors to be able to compute position of the cursor for the completion item itself, the following conditions are guaranteed to hold for RemoveRange of the stored fixits:
///  - Ranges in the fixits are guaranteed to never contain the completion point (or identifier under completion point, if any) inside them, except at the start or at the end of the range.
///  - If a fixit range starts or ends with completion point (or starts or ends after the identifier under completion point), it will contain at least one character. It allows to unambiguously recompute completion point after applying the fixit.
/// The intuition is that provided fixits change code around the identifier we complete, but are not allowed to touch the identifier itself or the completion point.
/// One example of completion items with corrections are the ones replacing '.' with '->' and vice versa:
///      std::unique_ptr<std::vector<int>> vec_ptr;
///      vec_ptr.^  // completion returns an item 'push_back', replacing '.' with '->'
///      vec_ptr->^ // completion returns an item 'release', replacing '->' with '.'let's put 
```

Do those invariants sound reasonable? Could we add asserts that they hold when constructing the completion results?


================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:565
+  /// \brief For this completion result correction is required.
+  std::vector<FixItHint> Corrections;
+
----------------
I wonder if we should call them Fixits instead?
To follow the pattern for diagnostics.


================
Comment at: tools/c-index-test/c-index-test.c:2346
+    CXSourceRange correction_range;
+    Corr = clang_getCompletionCorrection(completion_result->CompletionString, 0,
+                                         source_manager,
----------------
We should print all provided fixits in the test code


================
Comment at: tools/c-index-test/c-index-test.c:2350
+                                         &correction_range);
+    fprintf(file, " (requires correction to \"%s\")", clang_getCString(Corr));
+  }
----------------
maybe add a source range or source of the replaced text to the printf here?
e.g. "`replaces '.' with '->'`"?


================
Comment at: tools/libclang/CIndexCodeCompletion.cpp:340
+  const FixItHint &Correction = Corrections[correction_index];
+  if (replacement_range) {
+    SourceManager *SourceMgr = (SourceManager *)source_manager;
----------------
Maybe return a struct containing both the string and CXSourceRange instead of this output parameter?
It does not make sense to get a string without the corresponding source range.
How does libclang return fixits for diagnostics? We probably want to follow the same pattern here.


https://reviews.llvm.org/D41537