[clangd] Support multiple cursors in selectionRange.

[sammccall]

Summary

One change: because there's no way to signal failure individually for
each cursor, we now "succeed" with an empty range with no parent if a
cursor doesn't point at anything.

[usaxena95]

Mostly looks good. Few nits. Thanks.

clang-tools-extra/clangd/ClangdLSPServer.cpp

1193–1194

You can remove this check now.

clang-tools-extra/clangd/ClangdServer.cpp

671

nit: return statement can be omitted.

clang-tools-extra/clangd/SemanticSelection.cpp

65

nit: s/Dummy/Empty ?

74

nit:Range can be made a const ref.

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

175

I think it would be better to name the two points in the test and explicitly specify their order in the request (instead of relying on SourceAnnotations.points()).
Annotations::points doesn't guarantee order in its contract. Even if the current implementation does, it would be better to be explicit and not rely on it.

[sammccall]

Address comments

clang-tools-extra/clangd/ClangdLSPServer.cpp

1193–1194

Oops!

clang-tools-extra/clangd/SemanticSelection.cpp

74

Oops, fixed to actually move.

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

175

Good point. I updated the documentation on Annotations instead to make this guarantee, this was always the intent.

[usaxena95]

Thanks. LG.

clang-tools-extra/clangd/ClangdLSPServer.cpp

return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "File must be saved first: " +
                                        LastInvalidFile);
   return llvm::createStringError(
       llvm::inconvertibleErrorCode(),
       "Files must be saved first: " + LastInvalidFile + " (and " +
           llvm::to_string(InvalidFileCount - 1) + " others)");
 }
 
-// Converts a list of Ranges to a LinkedList of SelectionRange.
-SelectionRange render(const std::vector<Range> &Ranges) {
-  if (Ranges.empty())
-    return {};
-  SelectionRange Result;
-  Result.range = Ranges[0];
-  auto *Next = &Result.parent;
-  for (const auto &R : llvm::make_range(Ranges.begin() + 1, Ranges.end())) {
-    *Next = std::make_unique<SelectionRange>();
-    Next->get()->range = R;
-    Next = &Next->get()->parent;
-  }
-  return Result;
-}
-
 } // namespace
 
 // MessageHandler dispatches incoming LSP messages.
 // It handles cross-cutting concerns:
 //  - serializes/deserializes protocol objects to JSON
 //  - logging of inbound messages
 //  - cancellation handling
 //  - basic call tracing

 void ClangdLSPServer::onSymbolInfo(const TextDocumentPositionParams &Params,
                                    Callback<std::vector<SymbolDetails>> Reply) {
   Server->symbolInfo(Params.textDocument.uri.file(), Params.position,
                      std::move(Reply));
 }
 
 void ClangdLSPServer::onSelectionRange(
     const SelectionRangeParams &Params,
     Callback<std::vector<SelectionRange>> Reply) {
-  if (Params.positions.size() != 1) {
-    elog("{0} positions provided to SelectionRange. Supports exactly one "
-         "position.",
-         Params.positions.size());
-    return Reply(llvm::make_error<LSPError>(
-        "SelectionRange supports exactly one position",
-        ErrorCode::InvalidRequest));
-  }
   Server->semanticRanges(
-      Params.textDocument.uri.file(), Params.positions[0],
+      Params.textDocument.uri.file(), Params.positions,
       [Reply = std::move(Reply)](
-          llvm::Expected<std::vector<Range>> Ranges) mutable {
-        if (!Ranges) {
+          llvm::Expected<std::vector<SelectionRange>> Ranges) mutable {
+        if (!Ranges)
           return Reply(Ranges.takeError());
-        }
-        std::vector<SelectionRange> Result;
-        Result.emplace_back(render(std::move(*Ranges)));
-        return Reply(std::move(Result));
+        return Reply(std::move(*Ranges));
       });
 }
 
 void ClangdLSPServer::onDocumentLink(
     const DocumentLinkParams &Params,
     Callback<std::vector<DocumentLink>> Reply) {
 
   // TODO(forster): This currently resolves all targets eagerly. This is slow,

clang-tools-extra/clangd/ClangdServer.h

public:
   void onFileEvent(const DidChangeWatchedFilesParams &Params);
 
   /// Get symbol info for given position.
   /// Clangd extension - not part of official LSP.
   void symbolInfo(PathRef File, Position Pos,
                   Callback<std::vector<SymbolDetails>> CB);
 
   /// Get semantic ranges around a specified position in a file.
-  void semanticRanges(PathRef File, Position Pos,
-                      Callback<std::vector<Range>> CB);
+  void semanticRanges(PathRef File, const std::vector<Position> &Pos,
+                      Callback<std::vector<SelectionRange>> CB);
 
   /// Get all document links in a file.
   void documentLinks(PathRef File, Callback<std::vector<DocumentLink>> CB);
  
   /// Returns estimated memory usage for each of the currently open files.
   /// The order of results is unspecified.
   /// Overall memory usage of clangd may be significantly more than reported
   /// here, as this metric does not account (at least) for:

clang-tools-extra/clangd/ClangdServer.cpp

auto Action =
         if (!InpAST)
           return CB(InpAST.takeError());
         CB(clangd::getSymbolInfo(InpAST->AST, Pos));
       };
 
   WorkScheduler.runWithAST("SymbolInfo", File, std::move(Action));
 }
 
-void ClangdServer::semanticRanges(PathRef File, Position Pos,
-                                  Callback<std::vector<Range>> CB) {
-  auto Action =
-      [Pos, CB = std::move(CB)](llvm::Expected<InputsAndAST> InpAST) mutable {
-        if (!InpAST)
-          return CB(InpAST.takeError());
-        CB(clangd::getSemanticRanges(InpAST->AST, Pos));
-      };
+void ClangdServer::semanticRanges(PathRef File,
+                                  const std::vector<Position> &Positions,
+                                  Callback<std::vector<SelectionRange>> CB) {
+  auto Action = [Positions, CB = std::move(CB)](
+                    llvm::Expected<InputsAndAST> InpAST) mutable {
+    std::vector<SelectionRange> Result;
+    for (const auto &Pos : Positions) {
+      if (auto Range = clangd::getSemanticRanges(InpAST->AST, Pos))
+        Result.push_back(std::move(*Range));
+      else
+        return CB(Range.takeError());
+    }
+    CB(std::move(Result));
   WorkScheduler.runWithAST("SemanticRanges", File, std::move(Action));
 }
 
 void ClangdServer::documentLinks(PathRef File,
                                  Callback<std::vector<DocumentLink>> CB) {
   auto Action =
       [CB = std::move(CB)](llvm::Expected<InputsAndAST> InpAST) mutable {
         if (!InpAST)

clang-tools-extra/clangd/SemanticSelection.h

 #include "llvm/Support/Error.h"
 #include <vector>
 namespace clang {
 namespace clangd {
 
 /// Returns the list of all interesting ranges around the Position \p Pos.
 /// The interesting ranges corresponds to the AST nodes in the SelectionTree
 /// containing \p Pos.
-/// Any range in the result strictly contains all the previous ranges in the
-/// result. front() is the inner most range. back() is the outermost range.
-llvm::Expected<std::vector<Range>> getSemanticRanges(ParsedAST &AST,
-                                                     Position Pos);
+/// If pos is not in any interesting range, return [Pos, Pos).
+llvm::Expected<SelectionRange> getSemanticRanges(ParsedAST &AST, Position Pos);
+
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICSELECTION_H

clang-tools-extra/clangd/SemanticSelection.cpp

 //===--- SemanticSelection.cpp -----------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "SemanticSelection.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 // Adds Range \p R to the Result if it is distinct from the last added Range.
 // Assumes that only consecutive ranges can coincide.
 void addIfDistinct(const Range &R, std::vector<Range> &Result) {
   if (Result.empty() || Result.back() != R) {
     Result.push_back(R);
   }
 }
 } // namespace
 
-llvm::Expected<std::vector<Range>> getSemanticRanges(ParsedAST &AST,
-                                                     Position Pos) {
-  std::vector<Range> Result;
+llvm::Expected<SelectionRange> getSemanticRanges(ParsedAST &AST, Position Pos) {
+  std::vector<Range> Ranges;
   const auto &SM = AST.getSourceManager();
   const auto &LangOpts = AST.getLangOpts();
 
   auto FID = SM.getMainFileID();
   auto Offset = positionToOffset(SM.getBufferData(FID), Pos);
   if (!Offset) {
     return Offset.takeError();
   }
for (const auto *Node = ST.commonAncestor(); Node != nullptr;
 
     auto SR = toHalfOpenFileRange(SM, LangOpts, Node->ASTNode.getSourceRange());
     if (!SR.hasValue() || SM.getFileID(SR->getBegin()) != SM.getMainFileID()) {
       continue;
     }
     Range R;
     R.start = sourceLocToPosition(SM, SR->getBegin());
     R.end = sourceLocToPosition(SM, SR->getEnd());
-    addIfDistinct(R, Result);
+    addIfDistinct(R, Ranges);
   }
-  return Result;
+
+  if (Ranges.empty()) {
+    // LSP provides no way to signal "the point is not within a semantic range".
+    // Return an empty range at the point.
+    SelectionRange Empty;
+    Empty.range.start = Empty.range.end = Pos;
+    return Empty;
+  }
+
+  // Convert to the LSP linked-list representation.
+  SelectionRange Head;
+  Head.range = std::move(Ranges.front());
+  SelectionRange *Tail = &Head;
+  for (auto &Range :
+       llvm::makeMutableArrayRef(Ranges.data(), Ranges.size()).drop_front()) {
+    Tail->parent = std::make_unique<SelectionRange>();
+    Tail = Tail->parent.get();
+    Tail->range = std::move(Range);
+  }
+
+  return Head;
 }
 
 } // namespace clangd
 } // namespace clang

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

 #include "clang/Basic/SourceManager.h"
 #include "llvm/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <vector>
 namespace clang {
 namespace clangd {
 namespace {
+using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
 
+// front() is SR.range, back() is outermost range.
+std::vector<Range> gatherRanges(const SelectionRange &SR) {
+  std::vector<Range> Ranges;
+  for (const SelectionRange *S = &SR; S; S = S->parent.get())
+    Ranges.push_back(S->range);
+  return Ranges;
+}
+
 TEST(SemanticSelection, All) {
   const char *Tests[] = {
       R"cpp( // Single statement in a function body.
         [[void func() [[{
           [[[[int v = [[1^00]]]];]]
         }]]]]
       )cpp",
       R"cpp( // Expression
const char *Tests[] = {
        )cpp",
       R"cpp( // Before comment.
         [[void func() [[{
           int var1 = 1;
           [[[[int var2 = [[[[var1]]^ /*some comment*/ + 41]]]];]]
         }]]]]
        )cpp",
       // Empty file.
-      "^",
+      "[[^]]",
       // FIXME: We should get the whole DeclStmt as a range.
       R"cpp( // Single statement in TU.
         [[int v = [[1^00]]]];
       )cpp",
       R"cpp( // Cursor at end of VarDecl.
         [[int v = [[100]]^]];
       )cpp",
       // FIXME: No node found associated to the position.
       R"cpp( // Cursor in between spaces.
         void func() {
-          int v = 100 + ^  100;
+          int v = 100 + [[^]]  100;
         }
       )cpp",
       // Structs.
       R"cpp(
         struct AAA { struct BBB { static int ccc(); };};
         [[void func() [[{
           // int x = AAA::BBB::ccc();
           [[[[int x = [[[[AAA::BBB::c^cc]]()]]]];]]
const char *Tests[] = {
         }]]
       )cpp",
 
   };
 
   for (const char *Test : Tests) {
     auto T = Annotations(Test);
     auto AST = TestTU::withCode(T.code()).build();
-    EXPECT_THAT(llvm::cantFail(getSemanticRanges(AST, T.point())),
+    EXPECT_THAT(gatherRanges(llvm::cantFail(getSemanticRanges(AST, T.point()))),
                 ElementsAreArray(T.ranges()))
         << Test;
   }
 }
 
-TEST(SemanticSelection, RunViaClangDServer) {
+TEST(SemanticSelection, RunViaClangdServer) {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
 
   auto FooH = testPath("foo.h");
   FS.Files[FooH] = R"cpp(
     int foo(int x);
     #define HASH(x) ((x) % 10)
   )cpp";
 
   auto FooCpp = testPath("Foo.cpp");
   const char *SourceContents = R"cpp(
   #include "foo.h"
   [[void bar(int& inp) [[{
     // inp = HASH(foo(inp));
     [[inp = [[HASH([[foo([[in^p]])]])]]]];
   }]]]]
+  $empty[[^]]
   )cpp";
   Annotations SourceAnnotations(SourceContents);
   FS.Files[FooCpp] = std::string(SourceAnnotations.code());
   Server.addDocument(FooCpp, SourceAnnotations.code());
 
-  auto Ranges = runSemanticRanges(Server, FooCpp, SourceAnnotations.point());
+  auto Ranges = runSemanticRanges(Server, FooCpp, SourceAnnotations.points());
   ASSERT_TRUE(bool(Ranges))
       << "getSemanticRange returned an error: " << Ranges.takeError();
-  EXPECT_THAT(*Ranges, ElementsAreArray(SourceAnnotations.ranges()));
+  ASSERT_EQ(Ranges->size(), SourceAnnotations.points().size());
+  EXPECT_THAT(gatherRanges(Ranges->front()),
+              ElementsAreArray(SourceAnnotations.ranges()));
+  EXPECT_THAT(gatherRanges(Ranges->back()),
+              ElementsAre(SourceAnnotations.range("empty")));
 }
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/SyncAPI.h

 
 Expected<std::vector<DocumentSymbol>> runDocumentSymbols(ClangdServer &Server,
                                                          PathRef File);
 
 SymbolSlab runFuzzyFind(const SymbolIndex &Index, StringRef Query);
 SymbolSlab runFuzzyFind(const SymbolIndex &Index, const FuzzyFindRequest &Req);
 RefSlab getRefs(const SymbolIndex &Index, SymbolID ID);
 
-llvm::Expected<std::vector<Range>>
-runSemanticRanges(ClangdServer &Server, PathRef File, Position Pos);
+llvm::Expected<std::vector<SelectionRange>>
+runSemanticRanges(ClangdServer &Server, PathRef File,
+                  const std::vector<Position> &Pos);
 
 llvm::Expected<llvm::Optional<clangd::Path>>
 runSwitchHeaderSource(ClangdServer &Server, PathRef File);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SYNCAPI_H

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

 RefSlab getRefs(const SymbolIndex &Index, SymbolID ID) {
   RefsRequest Req;
   Req.IDs = {ID};
   RefSlab::Builder Slab;
   Index.refs(Req, [&](const Ref &S) { Slab.insert(ID, S); });
   return std::move(Slab).build();
 }
 
-llvm::Expected<std::vector<Range>>
-runSemanticRanges(ClangdServer &Server, PathRef File, Position Pos) {
-  llvm::Optional<llvm::Expected<std::vector<Range>>> Result;
+llvm::Expected<std::vector<SelectionRange>>
+runSemanticRanges(ClangdServer &Server, PathRef File,
+                  const std::vector<Position> &Pos) {
+  llvm::Optional<llvm::Expected<std::vector<SelectionRange>>> Result;
   Server.semanticRanges(File, Pos, capture(Result));
   return std::move(*Result);
 }
 
 llvm::Expected<llvm::Optional<clangd::Path>>
 runSwitchHeaderSource(ClangdServer &Server, PathRef File) {
   llvm::Optional<llvm::Expected<llvm::Optional<clangd::Path>>> Result;
   Server.switchSourceHeader(File, capture(Result));
   return std::move(*Result);
 }
 
 } // namespace clangd
 } // namespace clang

llvm/include/llvm/Testing/Support/Annotations.h

public:
   /// The input text with all annotations stripped.
   /// All points and ranges are relative to this stripped text.
   llvm::StringRef code() const { return Code; }
 
   /// Returns the position of the point marked by ^ (or $name^) in the text.
   /// Crashes if there isn't exactly one.
   size_t point(llvm::StringRef Name = "") const;
   /// Returns the position of all points marked by ^ (or $name^) in the text.
+  /// Order matches the order within the text.
   std::vector<size_t> points(llvm::StringRef Name = "") const;
 
   /// Returns the location of the range marked by [[ ]] (or $name[[ ]]).
   /// Crashes if there isn't exactly one.
   Range range(llvm::StringRef Name = "") const;
   /// Returns the location of all ranges marked by [[ ]] (or $name[[ ]]).
+  /// They are ordered by start position within the text.
   std::vector<Range> ranges(llvm::StringRef Name = "") const;
 
 private:
   std::string Code;
   llvm::StringMap<llvm::SmallVector<size_t, 1>> Points;
   llvm::StringMap<llvm::SmallVector<Range, 1>> Ranges;
 };
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &O,
                               const llvm::Annotations::Range &R);
 
 } // namespace llvm
 
 #endif