[clangd] Support textEdit in addition to insertText.

[kadircet]

Summary

Completion replies contains textEdits as well. Note that this change
relies on https://reviews.llvm.org/D50443.

[ilya-biryukov]

clangd/CodeComplete.cpp

289 ↗

(On Diff #159712)

We shouldn't have duplicate/overlapping fix-its, right? Maybe use std::sort?

291 ↗

(On Diff #159712)

use built-in tuples comparison std::tie(X.line, X.column) < std::tie(Y.line, Y.column)?

1057 ↗

(On Diff #159712)

Maybe add a comment describing the cases where isValid() is false?
Does it happen when we complete with empty identifier?

1310 ↗

(On Diff #159712)

Maybe keep the reserve call? (we could reserve one extra element, but that's fine)

clangd/CodeComplete.h

126 ↗

(On Diff #159712)

Could we avoid adding textEdit here?
It's an intermediate representation that is only used in clangd, and we should be able materialize the actual textEdit when converting to LSP.

unittests/clangd/SourceCodeTests.cpp

40 ↗

(On Diff #159712)

we convert those uint64_t into int when setting the positions anyway.
Maybe use int here too?

[kadircet]

• Resolve discussions.

[kadircet]

clangd/CodeComplete.cpp

1310 ↗

(On Diff #159712)

Actually we could have much more than one extra element, not for the current situation but may be in the future when we have more fixit completions.

[ilya-biryukov]

LGTM. Thanks for the change!
Could we add an option to clangd to switch it on? (VSCode does not work, but our hacked-up ycm integration seems to work, right?)

clangd/CodeComplete.cpp

1310 ↗

(On Diff #159712)

We can't have more than one edit adjacent to the completion identifier, right? Otherwise they'll conflict.
It is possible to have multiple edits which are consectutive and the last of them is adjacent to the completion identifier, though. But I don't think we handle those cases here anyway.

But I'm not too worried about leaving out the reserve call either. At the very worst we could waste some memory on a single completion item, but we shouldn't keep too many around at the same time anyway, so feel free to ignore this one.

[kadircet]

• Rebase.
• Resolve discussions.

clang-tools-extra/trunk/clangd/CodeComplete.h

struct CodeCompletion {
   std::string Header;
   // Present if Header is set and should be inserted to use this item.
   llvm::Optional<TextEdit> HeaderInsertion;
 
   /// Holds information about small corrections that needs to be done. Like
   /// converting '->' to '.' on member access.
   std::vector<TextEdit> FixIts;
 
+  /// Holds the range of the token we are going to replace with this completion.
+  Range CompletionTokenRange;
+
   // Scores are used to rank completion items.
   struct Scores {
     // The score that items are ranked by.
     float Total = 0.f;
 
     // The finalScore with the fuzzy name match score excluded.
     // When filtering client-side, editors should calculate the new fuzzy score,
     // whose scale is 0-1 (with 1 = prefix match, special case 2 = exact match),

clang-tools-extra/trunk/clangd/CodeComplete.cpp

 #include "Logger.h"
 #include "Quality.h"
 #include "SourceCode.h"
 #include "Trace.h"
 #include "URI.h"
 #include "index/Index.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Index/USRGeneration.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/Support/Format.h"
if (C.SemaResult) {
                   splitQualifiedName(printQualifiedName(*ND)).first;
       }
       Completion.Kind =
           toCompletionItemKind(C.SemaResult->Kind, C.SemaResult->Declaration);
       for (const auto &FixIt : C.SemaResult->FixIts) {
         Completion.FixIts.push_back(
             toTextEdit(FixIt, ASTCtx.getSourceManager(), ASTCtx.getLangOpts()));
       }
+      std::sort(Completion.FixIts.begin(), Completion.FixIts.end(),
+                [](const TextEdit &X, const TextEdit &Y) {
+                  return std::tie(X.range.start.line, X.range.start.character) <
+                         std::tie(Y.range.start.line, Y.range.start.character);
+                });
     }
     if (C.IndexResult) {
       Completion.Origin |= C.IndexResult->Origin;
       if (Completion.Scope.empty())
         Completion.Scope = C.IndexResult->Scope;
       if (Completion.Kind == CompletionItemKind::Missing)
         Completion.Kind = toCompletionItemKind(C.IndexResult->SymInfo.Kind);
       if (Completion.Name.empty())
CodeCompleteResult run(const SemaCompleteInput &SemaCCInput) && {
     // Indexes may choose to impose their own limits even if we don't have one.
     return Output;
   }
 
 private:
   // This is called by run() once Sema code completion is done, but before the
   // Sema data structures are torn down. It does all the real work.
   CodeCompleteResult runWithSema() {
+    const auto &CodeCompletionRange = CharSourceRange::getCharRange(
+        Recorder->CCSema->getPreprocessor().getCodeCompletionTokenRange());
+    Range TextEditRange;
+    // When we are getting completions with an empty identifier, for example
+    //    std::vector<int> asdf;
+    //    asdf.^;
+    // Then the range will be invalid and we will be doing insertion, use
+    // current cursor position in such cases as range.
+    if (CodeCompletionRange.isValid()) {
+      TextEditRange = halfOpenToRange(Recorder->CCSema->getSourceManager(),
+                                      CodeCompletionRange);
+    } else {
+      const auto &Pos = sourceLocToPosition(
+          Recorder->CCSema->getSourceManager(),
+          Recorder->CCSema->getPreprocessor().getCodeCompletionLoc());
+      TextEditRange.start = TextEditRange.end = Pos;
+    }
     Filter = FuzzyMatcher(
         Recorder->CCSema->getPreprocessor().getCodeCompletionFilter());
     QueryScopes = getQueryScopes(Recorder->CCContext,
                                  Recorder->CCSema->getSourceManager());
     // Sema provides the needed context to query the index.
     // FIXME: in addition to querying for extra/overlapping symbols, we should
     //        explicitly request symbols corresponding to Sema results.
     //        We can use their signals even if the index can't suggest them.
     // We must copy index results to preserve them, but there are at most Limit.
     auto IndexResults = (Opts.Index && allowIndex(Recorder->CCContext))
                             ? queryIndex()
                             : SymbolSlab();
     // Merge Sema and Index results, score them, and pick the winners.
     auto Top = mergeResults(Recorder->Results, IndexResults);
     // Convert the results to final form, assembling the expensive strings.
     CodeCompleteResult Output;
     for (auto &C : Top) {
       Output.Completions.push_back(toCodeCompletion(C.first));
       Output.Completions.back().Score = C.second;
+      Output.Completions.back().CompletionTokenRange = TextEditRange;
     }
     Output.HasMore = Incomplete;
     Output.Context = Recorder->CCContext.getKind();
     return Output;
   }
 
   SymbolSlab queryIndex() {
     trace::Span Tracer("Query index");
CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const {
   LSP.kind = Kind;
   LSP.detail = BundleSize > 1 ? llvm::formatv("[{0} overloads]", BundleSize)
                               : ReturnType;
   if (!Header.empty())
     LSP.detail += "\n" + Header;
   LSP.documentation = Documentation;
   LSP.sortText = sortText(Score.Total, Name);
   LSP.filterText = Name;
-  // FIXME(kadircet): Use LSP.textEdit instead of insertText, because it causes
-  // undesired behaviours. Like completing "this.^" into "this-push_back".
-  LSP.insertText = RequiredQualifier + Name;
+  LSP.textEdit = {CompletionTokenRange, RequiredQualifier + Name};
+  // Merge continious additionalTextEdits into main edit. The main motivation
+  // behind this is to help LSP clients, it seems most of them are confused when
+  // they are provided with additionalTextEdits that are consecutive to main
+  // edit.
+  // Note that we store additional text edits from back to front in a line. That
+  // is mainly to help LSP clients again, so that changes do not effect each
+  // other.
+  for (const auto &FixIt : FixIts) {
+    if (IsRangeConsecutive(FixIt.range, LSP.textEdit->range)) {
+      LSP.textEdit->newText = FixIt.newText + LSP.textEdit->newText;
+      LSP.textEdit->range.start = FixIt.range.start;
+    } else {
+      LSP.additionalTextEdits.push_back(FixIt);
+    }
+  }
   if (Opts.EnableSnippets)
-    LSP.insertText += SnippetSuffix;
+    LSP.textEdit->newText += SnippetSuffix;
+  // FIXME(kadircet): Do not even fill insertText after making sure textEdit is
+  // compatible with most of the editors.
+  LSP.insertText = LSP.textEdit->newText;
   LSP.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet
                                              : InsertTextFormat::PlainText;
-  LSP.additionalTextEdits.reserve(FixIts.size() + (HeaderInsertion ? 1 : 0));
-  for (const auto &FixIt : FixIts)
-    LSP.additionalTextEdits.push_back(FixIt);
   if (HeaderInsertion)
     LSP.additionalTextEdits.push_back(*HeaderInsertion);
   return LSP;
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const CodeCompletion &C) {
   // For now just lean on CompletionItem.
   return OS << C.render(CodeCompleteOptions());

clang-tools-extra/trunk/clangd/SourceCode.h

 ///   - Symlinks resolved
 ///   - No "." or ".." component
 ///   - No duplicate or trailing directory separator
 ///
 /// This function should be used when sending paths to clients, so that paths
 /// are normalized as much as possible.
 llvm::Optional<std::string> getRealPath(const FileEntry *F,
                                         const SourceManager &SourceMgr);
+
+bool IsRangeConsecutive(const Range &Left, const Range &Right);
 } // namespace clangd
 } // namespace clang
 #endif

clang-tools-extra/trunk/clangd/SourceCode.cpp

TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
                     const LangOptions &L) {
   TextEdit Result;
   Result.range =
       halfOpenToRange(M, Lexer::makeFileCharRange(FixIt.RemoveRange, M, L));
   Result.newText = FixIt.CodeToInsert;
   return Result;
 }
 
+bool IsRangeConsecutive(const Range &Left, const Range &Right) {
+  return Left.end.line == Right.start.line &&
+         Left.end.character == Right.start.character;
+}
+
 } // namespace clangd
 } // namespace clang

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

TEST(CompletionTest, FixItForDotToArrow) {
   for (const auto &C : Results.Completions) {
     EXPECT_TRUE(C.FixIts.empty() || C.Name == "AuxFunction");
     if (!C.FixIts.empty()) {
       EXPECT_THAT(C.FixIts, ElementsAre(ReplacementEdit));
     }
   }
 }
 
+TEST(CompletionTest, RenderWithFixItMerged) {
+  TextEdit FixIt;
+  FixIt.range.end.character = 5;
+  FixIt.newText = "->";
+
+  CodeCompletion C;
+  C.Name = "x";
+  C.RequiredQualifier = "Foo::";
+  C.FixIts = {FixIt};
+  C.CompletionTokenRange.start.character = 5;
+
+  CodeCompleteOptions Opts;
+  Opts.IncludeFixIts = true;
+
+  auto R = C.render(Opts);
+  EXPECT_TRUE(R.textEdit);
+  EXPECT_EQ(R.textEdit->newText, "->Foo::x");
+  EXPECT_TRUE(R.additionalTextEdits.empty());
+}
+
+TEST(CompletionTest, RenderWithFixItNonMerged) {
+  TextEdit FixIt;
+  FixIt.range.end.character = 4;
+  FixIt.newText = "->";
+
+  CodeCompletion C;
+  C.Name = "x";
+  C.RequiredQualifier = "Foo::";
+  C.FixIts = {FixIt};
+  C.CompletionTokenRange.start.character = 5;
+
+  CodeCompleteOptions Opts;
+  Opts.IncludeFixIts = true;
+
+  auto R = C.render(Opts);
+  EXPECT_TRUE(R.textEdit);
+  EXPECT_EQ(R.textEdit->newText, "Foo::x");
+  EXPECT_THAT(R.additionalTextEdits, UnorderedElementsAre(FixIt));
+}
+
+TEST(CompletionTest, CompletionTokenRange) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  constexpr const char *TestCodes[] = {
+      R"cpp(
+        class Auxilary {
+         public:
+          void AuxFunction();
+        };
+        void f() {
+          Auxilary x;
+          x.[[Aux]]^;
+        }
+      )cpp",
+      R"cpp(
+        class Auxilary {
+         public:
+          void AuxFunction();
+        };
+        void f() {
+          Auxilary x;
+          x.[[]]^;
+        }
+      )cpp"};
+  for (const auto &Text : TestCodes) {
+    Annotations TestCode(Text);
+    auto Results = completions(Server, TestCode.code(), TestCode.point());
+
+    EXPECT_EQ(Results.Completions.size(), 1u);
+    EXPECT_THAT(Results.Completions.front().CompletionTokenRange, TestCode.range());
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/unittests/clangd/SourceCodeTests.cpp

 /// A helper to make tests easier to read.
 Position position(int line, int character) {
   Position Pos;
   Pos.line = line;
   Pos.character = character;
   return Pos;
 }
 
+Range range(const std::pair<int, int> p1, const std::pair<int, int> p2) {
+  Range range;
+  range.start = position(p1.first, p1.second);
+  range.end = position(p2.first, p2.second);
+  return range;
+}
+
 TEST(SourceCodeTests, PositionToOffset) {
   // line out of bounds
   EXPECT_THAT_EXPECTED(positionToOffset(File, position(-1, 2)), Failed());
   // first line
   EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, -1)),
                        Failed()); // out of range
   EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 0)),
                        HasValue(0)); // first character
TEST(SourceCodeTests, OffsetToPosition) {
   EXPECT_THAT(offsetToPosition(File, 22), Pos(2, 4)) << "before astral char";
   EXPECT_THAT(offsetToPosition(File, 24), Pos(2, 6)) << "in astral char";
   EXPECT_THAT(offsetToPosition(File, 26), Pos(2, 6)) << "after astral char";
   EXPECT_THAT(offsetToPosition(File, 28), Pos(2, 8)) << "end of last line";
   EXPECT_THAT(offsetToPosition(File, 29), Pos(2, 9)) << "EOF";
   EXPECT_THAT(offsetToPosition(File, 30), Pos(2, 9)) << "out of bounds";
 }
 
+TEST(SourceCodeTests, IsRangeConsecutive) {
+  EXPECT_TRUE(IsRangeConsecutive(range({2, 2}, {2, 3}), range({2, 3}, {2, 4})));
+  EXPECT_FALSE(
+      IsRangeConsecutive(range({0, 2}, {0, 3}), range({2, 3}, {2, 4})));
+  EXPECT_FALSE(
+      IsRangeConsecutive(range({2, 2}, {2, 3}), range({2, 4}, {2, 5})));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang