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

[hokein]

Summary

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

[jvikstrom]

It feels a bit strange to be sending highlighting (even if they are empty) beyond eof. But I guess the proposal does not specify this and it would make life for the vscode extension (much) simpler.
Theia also does not crash from this and if we apply decorations to vscode outside the file nothing happens as well so I guess it should be fine. (even when vscode finally implements this feature into lsp)

Just had the comment about comparing HighlightigLine objects instead of using matchers but up to you.

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

625 ↗

(On Diff #216417)

Maybe create the HighlightingLine objects for Line 3 and 4 and add them directly in the UnorderedElementsAre because then we don't have to create the matchers.

[hokein]

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

625 ↗

(On Diff #216417)

we prefer to use gtest matchers in the unittest as they are easier to understand, and provide better error messages (when the tests fail).

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

public:
   ///
   /// \return Whether we shut down cleanly with a 'shutdown' -> 'exit' sequence.
   bool run();
 
 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>).
   void onInitialize(const InitializeParams &, Callback<llvm::json::Value>);
   void onShutdown(const ShutdownParams &, Callback<std::nullptr_t>);
   void onSync(const NoParams &, Callback<std::nullptr_t>);
   void onDocumentDidOpen(const DidOpenTextDocumentParams &);
   void onDocumentDidChange(const DidChangeTextDocumentParams &);

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

if (Trigger == ">")
     return (*Code)[*Offset - 2] == '-'; // trigger only on '->'.
   if (Trigger == ":")
     return (*Code)[*Offset - 2] == ':'; // trigger only on '::'.
   assert(false && "unhandled trigger character");
   return true;
 }
 
 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;
   {
     std::lock_guard<std::mutex> Lock(HighlightingsMutex);
     Old = std::move(FileToHighlightings[File]);
     FileToHighlightings[File] = std::move(HighlightingsCopy);
   }
   // 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)});
 }
 
 void ClangdLSPServer::onDiagnosticsReady(PathRef File,
                                          std::vector<Diag> Diagnostics) {
   auto URI = URIForFile::canonicalize(File, /*TUPath=*/File);

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

public:
   /// Called whenever the file status is updated.
   virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
 
   /// Called by ClangdServer when some \p Highlightings for \p File are ready.
   /// \p NumLines are the number of lines in the file where the highlightings
   /// 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
 /// file. Must be thread-safe. We use this instead of ClangTidyOptionsProvider
 /// to allow reading tidy configs from the VFS used for parsing.
 using ClangTidyOptionsBuilder = std::function<tidy::ClangTidyOptions(
     llvm::vfs::FileSystem &, llvm::StringRef /*File*/)>;
 

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

void onMainAST(PathRef Path, ParsedAST &AST, PublishFn Publish) override {
     if (FIndex)
       FIndex->updateMain(Path, AST);
 
     std::vector<Diag> Diagnostics = AST.getDiagnostics();
     std::vector<HighlightingToken> Highlightings;
     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));
     });
   }
 
   void onFileUpdated(PathRef File, const TUStatus &Status) override {
     DiagConsumer.onFileUpdated(File, Status);
   }
 
 private:

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

 
 /// Convert to LSP's semantic highlighting information.
 std::vector<SemanticHighlightingInformation>
 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
 
 #endif

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

return ArrayRef<HighlightingToken>(StartLineIt, AllTokens.end())
       .take_while([Line](const HighlightingToken &Token) {
         return Token.R.start.line == Line;
       });
 }
 } // namespace
 
 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
   // the line is the equal to the previous one than we will remove all
   // highlights but the ones for the token spanning multiple lines. This means
   // that when we get into the LSP layer the only highlights that will be
   // visible are the ones for the token spanning multiple lines.
   // Example:
diffHighlightings(ArrayRef<HighlightingToken> New,
   auto NextLineNumber = [&]() {
     int NextNew = NewLine.end() != NewEnd ? NewLine.end()->R.start.line
                                              : std::numeric_limits<int>::max();
     int NextOld = OldLine.end() != OldEnd ? OldLine.end()->R.start.line
                                              : std::numeric_limits<int>::max();
     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);
     if (NewLine != OldLine)
       DiffedLines.push_back({LineNumber, NewLine});
   }
 
   return DiffedLines;

clang-tools-extra/trunk/clangd/test/semantic-highlighting.test

 # CHECK-NEXT:      "uri": "file:///clangd-test/foo.cpp"
 # CHECK-NEXT:    }
 # CHECK-NEXT:  }
 # CHECK-NEXT:}
 ---
 {"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:    }
 # CHECK-NEXT:  }
 # CHECK-NEXT:}
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

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

 #include "TestFS.h"
 #include "TestTU.h"
 #include "gmock/gmock.h"
 
 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());
   for (int I = 0, End = Ranges.size(); I < End; ++I) {
     Tokens[I].R = Ranges[I];
     Tokens[I].Kind = Kind;
   }
 
for (const HighlightingToken &Token : NewTokens) {
     if (It != ExpectedLines.end())
       It->second.push_back(Token);
   }
   for (auto &LineTokens : ExpectedLines)
     ExpectedLinePairHighlighting.push_back(
         {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) {
   const char *TestCases[] = {
     R"cpp(
       struct $Class[[AS]] {
         $Primitive[[double]] $Field[[SomeMember]];
       };

 }
 
 TEST(SemanticHighlighting, GeneratesHighlightsWhenFileChange) {
   class HighlightingsCounterDiagConsumer : public DiagnosticsConsumer {
   public:
     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;
     }
   };
 
   auto FooCpp = testPath("foo.cpp");
   MockFSProvider FS;
   FS.Files[FooCpp] = "";
 
llvm::StringRef NewCode;
         $Class[[A]]
        ^$Variable[[AA]]
         $Variable[[A]]
       )"},
                 {
                     R"(
         $Class[[A]]
         $Variable[[A]]
-        $Class[[A]]
-        $Variable[[A]]
-      )",
-                    R"(
-        $Class[[A]]
-        $Variable[[A]]
-      )"},
-                {
-                    R"(
-        $Class[[A]]
-        $Variable[[A]]
       )",
                     R"(
         $Class[[A]]
         $Variable[[A]]
        ^$Class[[A]]
        ^$Variable[[A]]
       )"},
                 {
                     R"(
         $Variable[[A]]
         $Variable[[A]]
         $Variable[[A]]
       )",
                     R"(
        ^$Class[[A]]
        ^$Class[[A]]
        ^$Class[[A]]
       )"}};
 
   for (const auto &Test : TestCases)
     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