diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -55,9 +55,9 @@ // Implement DiagnosticsConsumer. void onDiagnosticsReady(PathRef File, std::vector Diagnostics) override; void onFileUpdated(PathRef File, const TUStatus &Status) override; - void onHighlightingsReady(PathRef File, - std::vector Highlightings, - int NumLines) override; + void + onHighlightingsReady(PathRef File, + std::vector Highlightings) override; // LSP methods. Notifications have signature void(const Params&). // Calls have signature void(const Params&, Callback). diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -1185,7 +1185,7 @@ } void ClangdLSPServer::onHighlightingsReady( - PathRef File, std::vector Highlightings, int NumLines) { + PathRef File, std::vector Highlightings) { std::vector Old; std::vector HighlightingsCopy = Highlightings; { @@ -1195,8 +1195,7 @@ } // 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 Diffed = - diffHighlightings(Highlightings, Old, NumLines); + std::vector Diffed = diffHighlightings(Highlightings, Old); publishSemanticHighlighting( {{URIForFile::canonicalize(File, /*TUPath=*/File)}, toSemanticHighlightingInformation(Diffed)}); diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -56,8 +56,7 @@ /// where generated from. virtual void onHighlightingsReady(PathRef File, - std::vector Highlightings, - int NumLines) {} + std::vector Highlightings) {} }; /// When set, used by ClangdServer to get clang-tidy options for each particular diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -73,19 +73,10 @@ 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/SemanticHighlighting.h b/clang-tools-extra/clangd/SemanticHighlighting.h --- a/clang-tools-extra/clangd/SemanticHighlighting.h +++ b/clang-tools-extra/clangd/SemanticHighlighting.h @@ -71,15 +71,15 @@ /// 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 diffHighlightings(ArrayRef New, - ArrayRef Old, int NewMaxLine); + ArrayRef Old); } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/SemanticHighlighting.cpp b/clang-tools-extra/clangd/SemanticHighlighting.cpp --- a/clang-tools-extra/clangd/SemanticHighlighting.cpp +++ b/clang-tools-extra/clangd/SemanticHighlighting.cpp @@ -339,9 +339,11 @@ std::vector diffHighlightings(ArrayRef New, - ArrayRef 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 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 @@ 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/test/semantic-highlighting.test b/clang-tools-extra/clangd/test/semantic-highlighting.test --- 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 --- a/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp +++ b/clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp @@ -18,6 +18,9 @@ namespace clangd { namespace { +MATCHER_P(LineNumber, L, "") { return arg.Line == L; } +MATCHER(EmptyHighlightings, "") { return arg.Tokens.empty(); } + std::vector makeHighlightingTokens(llvm::ArrayRef Ranges, HighlightingKind Kind) { std::vector Tokens(Ranges.size()); @@ -92,9 +95,10 @@ {LineTokens.first, LineTokens.second}); std::vector 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 @@ std::atomic Count = {0}; void onDiagnosticsReady(PathRef, std::vector) override {} - void onHighlightingsReady(PathRef File, - std::vector Highlightings, - int NLines) override { + void onHighlightingsReady( + PathRef File, std::vector Highlightings) override { ++Count; } }; @@ -569,17 +572,6 @@ $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 @@ 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 OldTokens = getExpectedTokens(OldTest); + std::vector 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