[clangd] Send highlighting diff beyond the end of the file.

Summary: This would make the client life (tracking the changes) easier.

Reviewers: jvikstrom

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66541

llvm-svn: 369884

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 928bd250d21eeac9139881f2ec0fb53e80bf9117..ee17cb0823467dd5567b74c6af0002f5eeb9c131 100644 (file)
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1195,7 +1195,7 @@ bool ClangdLSPServer::shouldRunCompletion(
 }
 
 void ClangdLSPServer::onHighlightingsReady(
-    PathRef File, std::vector<HighlightingToken> Highlightings, int NumLines) {
+    PathRef File, std::vector<HighlightingToken> Highlightings) {
   std::vector<HighlightingToken> Old;
   std::vector<HighlightingToken> HighlightingsCopy = Highlightings;
   {
@@ -1205,8 +1205,7 @@ void ClangdLSPServer::onHighlightingsReady(
   }
   // LSP allows us to send incremental edits of highlightings. Also need to diff
   // to remove highlightings from tokens that should no longer have them.
-  std::vector<LineHighlightings> Diffed =
-      diffHighlightings(Highlightings, Old, NumLines);
+  std::vector<LineHighlightings> Diffed = diffHighlightings(Highlightings, Old);
   publishSemanticHighlighting(
       {{URIForFile::canonicalize(File, /*TUPath=*/File)},
        toSemanticHighlightingInformation(Diffed)});

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 65aa447f4806cc1da385a730e5a53a151be4279a..4e7e042bb9ada7b64403c196c517fa79bb43138e 100644 (file)
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -55,9 +55,9 @@ private:
   // Implement DiagnosticsConsumer.
   void onDiagnosticsReady(PathRef File, std::vector<Diag> Diagnostics) override;
   void onFileUpdated(PathRef File, const TUStatus &Status) override;
-  void onHighlightingsReady(PathRef File,
-                            std::vector<HighlightingToken> Highlightings,
-                            int NumLines) override;
+  void
+  onHighlightingsReady(PathRef File,
+                       std::vector<HighlightingToken> Highlightings) override;
 
   // LSP methods. Notifications have signature void(const Params&).
   // Calls have signature void(const Params&, Callback<Response>).

diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index 5f70264a1868d5dd968a7e324f3ba14bc3700731..460883b84391fd99d6d7293f951f50cc5c58b828 100644 (file)
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -73,19 +73,10 @@ struct UpdateIndexCallbacks : public ParsingCallbacks {
     if (SemanticHighlighting)
       Highlightings = getSemanticHighlightings(AST);
 
-    // FIXME: We need a better way to send the maximum line number to the
-    // differ.
-    // The differ needs the information about the max number of lines
-    // to not send diffs that are outside the file.
-    const SourceManager &SM = AST.getSourceManager();
-    FileID MainFileID = SM.getMainFileID();
-    int NumLines = SM.getBufferData(MainFileID).count('\n') + 1;
-
     Publish([&]() {
       DiagConsumer.onDiagnosticsReady(Path, std::move(Diagnostics));
       if (SemanticHighlighting)
-        DiagConsumer.onHighlightingsReady(Path, std::move(Highlightings),
-                                          NumLines);
+        DiagConsumer.onHighlightingsReady(Path, std::move(Highlightings));
     });
   }
 

diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index 378cea5cf61c342e70bb631670d13e923331bc97..f6f07f5fdd7105d8b8c71fba5c9d9ffe769a017b 100644 (file)
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -56,8 +56,7 @@ public:
   /// where generated from.
   virtual void
   onHighlightingsReady(PathRef File,
-                       std::vector<HighlightingToken> Highlightings,
-                       int NumLines) {}
+                       std::vector<HighlightingToken> Highlightings) {}
 };
 
 /// When set, used by ClangdServer to get clang-tidy options for each particular

diff --git a/clang-tools-extra/clangd/SemanticHighlighting.cpp b/clang-tools-extra/clangd/SemanticHighlighting.cpp
index a9bf5aff382fa0125afb178a425399a14f425b2b..40a2296f45fc34f50915696860f8b2d201c9cc24 100644 (file)
--- a/clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ b/clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -339,9 +339,11 @@ takeLine(ArrayRef<HighlightingToken> AllTokens,
 
 std::vector<LineHighlightings>
 diffHighlightings(ArrayRef<HighlightingToken> New,
-                  ArrayRef<HighlightingToken> Old, int NewMaxLine) {
-  assert(std::is_sorted(New.begin(), New.end()) && "New must be a sorted vector");
-  assert(std::is_sorted(Old.begin(), Old.end()) && "Old must be a sorted vector");
+                  ArrayRef<HighlightingToken> Old) {
+  assert(std::is_sorted(New.begin(), New.end()) &&
+         "New must be a sorted vector");
+  assert(std::is_sorted(Old.begin(), Old.end()) &&
+         "Old must be a sorted vector");
 
   // FIXME: There's an edge case when tokens span multiple lines. If the first
   // token on the line started on a line above the current one and the rest of
@@ -371,9 +373,7 @@ diffHighlightings(ArrayRef<HighlightingToken> New,
     return std::min(NextNew, NextOld);
   };
 
-  // If the New file has fewer lines than the Old file we don't want to send
-  // highlightings beyond the end of the file.
-  for (int LineNumber = 0; LineNumber < NewMaxLine;
+  for (int LineNumber = 0; NewLine.end() < NewEnd || OldLine.end() < OldEnd;
        LineNumber = NextLineNumber()) {
     NewLine = takeLine(New, NewLine.end(), LineNumber);
     OldLine = takeLine(Old, OldLine.end(), LineNumber);

diff --git a/clang-tools-extra/clangd/SemanticHighlighting.h b/clang-tools-extra/clangd/SemanticHighlighting.h
index 9b2c203d0879f0bde4500a4815b9ecc30d3e7711..8d90b33edba8045cfc2980f27d3b8d0f31449aa6 100644 (file)
--- a/clang-tools-extra/clangd/SemanticHighlighting.h
+++ b/clang-tools-extra/clangd/SemanticHighlighting.h
@@ -71,15 +71,15 @@ toSemanticHighlightingInformation(llvm::ArrayRef<LineHighlightings> Tokens);
 /// Return a line-by-line diff between two highlightings.
 ///  - if the tokens on a line are the same in both hightlightings, this line is
 ///  omitted.
-///  - if a line exists in New but not in Old the tokens on this line are
+///  - if a line exists in New but not in Old, the tokens on this line are
 ///  emitted.
-///  - if a line does not exists in New but exists in Old an empty line is
+///  - if a line does not exist in New but exists in Old, an empty line is
 ///  emitted (to tell client to clear the previous highlightings on this line).
-/// \p NewMaxLine is the maximum line number from the new file.
+///
 /// REQUIRED: Old and New are sorted.
 std::vector<LineHighlightings>
 diffHighlightings(ArrayRef<HighlightingToken> New,
-                  ArrayRef<HighlightingToken> Old, int NewMaxLine);
+                  ArrayRef<HighlightingToken> Old);
 
 } // namespace clangd
 } // namespace clang

diff --git a/clang-tools-extra/clangd/test/semantic-highlighting.test b/clang-tools-extra/clangd/test/semantic-highlighting.test
index c500561b1f09d9599e1528fb649704c936e52501..0b587130f84242b51b52a2c1306cf882f2412c30 100644 (file)
--- a/clang-tools-extra/clangd/test/semantic-highlighting.test
+++ b/clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -92,7 +92,12 @@
 {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"test:///foo.cpp","version":2},"contentChanges": [{"range":{"start": {"line": 0,"character": 10},"end": {"line": 1,"character": 10}},"rangeLength": 11,"text": ""}]}}
 #      CHECK:  "method": "textDocument/semanticHighlighting",
 # CHECK-NEXT:  "params": {
-# CHECK-NEXT:    "lines": [],
+# CHECK-NEXT:    "lines": [
+# CHECK-NEXT:      {
+# CHECK-NEXT:        "line": 1,
+# CHECK-NEXT:        "tokens": ""
+# CHECK-NEXT:      }
+# CHECK-NEXT:   ],
 # CHECK-NEXT:    "textDocument": {
 # CHECK-NEXT:      "uri": "file:///clangd-test/foo.cpp"
 # CHECK-NEXT:    }

diff --git a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
index 903f719132af35fbe16e85400f72d2009f5576fa..8a7826442546a0db1d3a0d8d0bb51267188e29b7 100644 (file)
--- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -18,6 +18,9 @@ namespace clang {
 namespace clangd {
 namespace {
 
+MATCHER_P(LineNumber, L, "") { return arg.Line == L; }
+MATCHER(EmptyHighlightings, "") { return arg.Tokens.empty(); }
+
 std::vector<HighlightingToken>
 makeHighlightingTokens(llvm::ArrayRef<Range> Ranges, HighlightingKind Kind) {
   std::vector<HighlightingToken> Tokens(Ranges.size());
@@ -92,9 +95,10 @@ void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
         {LineTokens.first, LineTokens.second});
 
   std::vector<LineHighlightings> ActualDiffed =
-      diffHighlightings(NewTokens, OldTokens, NewCode.count('\n'));
+      diffHighlightings(NewTokens, OldTokens);
   EXPECT_THAT(ActualDiffed,
-              testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting));
+              testing::UnorderedElementsAreArray(ExpectedLinePairHighlighting))
+      << OldCode;
 }
 
 TEST(SemanticHighlighting, GetsCorrectTokens) {
@@ -463,9 +467,8 @@ TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
     std::atomic<int> Count = {0};
 
     void onDiagnosticsReady(PathRef, std::vector<Diag>) override {}
-    void onHighlightingsReady(PathRef File,
-                              std::vector<HighlightingToken> Highlightings,
-                              int NLines) override {
+    void onHighlightingsReady(
+        PathRef File, std::vector<HighlightingToken> Highlightings) override {
       ++Count;
     }
   };
@@ -569,17 +572,6 @@ TEST(SemanticHighlighting, HighlightingDiffer) {
         $Class[[A]]
        ^$Variable[[AA]]
         $Variable[[A]]
-      )"},
-                {
-                    R"(
-        $Class[[A]]
-        $Variable[[A]]
-        $Class[[A]]
-        $Variable[[A]]
-      )",
-                    R"(
-        $Class[[A]]
-        $Variable[[A]]
       )"},
                 {
                     R"(
@@ -608,6 +600,32 @@ TEST(SemanticHighlighting, HighlightingDiffer) {
     checkDiffedHighlights(Test.OldCode, Test.NewCode);
 }
 
+TEST(SemanticHighlighting, DiffBeyondTheEndOfFile) {
+  llvm::StringRef OldCode =
+      R"(
+        $Class[[A]]
+        $Variable[[A]]
+        $Class[[A]]
+        $Variable[[A]]
+      )";
+  llvm::StringRef NewCode =
+      R"(
+        $Class[[A]] // line 1
+        $Variable[[A]] // line 2
+      )";
+
+  Annotations OldTest(OldCode);
+  Annotations NewTest(NewCode);
+  std::vector<HighlightingToken> OldTokens = getExpectedTokens(OldTest);
+  std::vector<HighlightingToken> NewTokens = getExpectedTokens(NewTest);
+
+  auto ActualDiff = diffHighlightings(NewTokens, OldTokens);
+  EXPECT_THAT(ActualDiff,
+              testing::UnorderedElementsAre(
+                  testing::AllOf(LineNumber(3), EmptyHighlightings()),
+                  testing::AllOf(LineNumber(4), EmptyHighlightings())));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang