[clangd] Add semantic selection to ClangdLSPServer.

[usaxena95]

Summary

This adds semantic selection to the LSP Server.
Adds support for serialization of input request and the output reply.
Also adds regression tests for the feature.

Currently we do not support multi cursor.The LSP Server only accepts single position in the request as opposed to many position in the spec.

Spec:
https://github.com/microsoft/language-server-protocol/blob/dbaeumer/3.15/specification.md#textDocument_selectionRange

[ilya-biryukov]

A drive-by comment...

clang-tools-extra/clangd/ClangdLSPServer.cpp

1135 ↗

(On Diff #220699)

This should be return Reply(...), right?
Especially bad if positions is empty as we attempt to read the first element right away.

[usaxena95]

Fixed error message.

[usaxena95]

Addressed comments.

[usaxena95]

clang-tools-extra/clangd/ClangdLSPServer.cpp

1135 ↗

(On Diff #220699)

Aah. Yeah. Thanks.

[hokein]

I think we're missing the client/server capability implementation in this patch, we should include them.

clang-tools-extra/clangd/ClangdLSPServer.cpp

1131 ↗

(On Diff #220710)

maybe add an assert(!Params.positions.empty()). I think we should not run into this case.

clang-tools-extra/clangd/Protocol.h

1234 ↗

(On Diff #220710)

The data structures defined in Protocol are mirrored to LSP's. I think we should follow it for semantic selection as well (even though LSP use a wired structure, linked list).

so here, the structure should be like

struct SelectionRange {
    /**
     * The [range](#Range) of this selection range.
     */
    Range range;
    /**
     * The parent selection range containing this range. Therefore `parent.range` must contain `this.range`.
     */
    llvm::Optional<SelectionRange> parent;
}

And we'd need to define a render function in ClangLSPServer to covert the vector<Range> to the LSP's SelectionRange.

1241 ↗

(On Diff #220710)

does this operator get used in this patch?

clang-tools-extra/clangd/test/selection-range.test

4 ↗

(On Diff #220710)

could we create a smaller test? just void func() {} is enough, I'd keep the lit test as small as possible (since this feature has been test in the unittest)

[ilya-biryukov]

clang-tools-extra/clangd/ClangdLSPServer.cpp

1131 ↗

(On Diff #220710)

But Params comes to clangd over LSP, right?
That means assert can fire in case of bad inputs over LSP to clangd.
Bad inputs over LSP should never crash clangd.

[usaxena95]

Addressed comments:
Added client/server capabilities.
Made lit test smaller.

[usaxena95]

I am not sure adding client capability is useful here. I have not used the client capability for selectionRange anywhere and I think we can remove it.
WDYT ?

clang-tools-extra/clangd/ClangdLSPServer.cpp

1131 ↗

(On Diff #220710)

Yes this comes from the client and can be a bad input. We should just return error and not crash in such case.

clang-tools-extra/clangd/Protocol.h

1241 ↗

(On Diff #220710)

No. Removed it.

[hokein]

In D67720#1680460, @usaxena95 wrote:

I am not sure adding client capability is useful here. I have not used the client capability for selectionRange anywhere and I think we can remove it.
WDYT ?

The SelectionRangeClientCapabilities determines what should the LSP server send the client, if it is true, clangd should send SelectionRangeRegistrationOptions. But looking at the current specification, it doesn't seem to add too much value. I think we can just simplify return a bool for now (as you did in this patch).

clang-tools-extra/clangd/ClangdLSPServer.cpp

141 ↗

(On Diff #221499)

nit: use for range.

1131 ↗

(On Diff #220710)

but the code still doesn't handle the empty case?

clang-tools-extra/clangd/Protocol.h

1248 ↗

(On Diff #221499)

I think we can simplify the code further, using llvm::Optional<SelectionRange> should be enough, the parent is null for the outer-most range.

[usaxena95]

Addressed comments.

[usaxena95]

The SelectionRangeClientCapabilities determines what should the LSP server send the client, if it is true, clangd should send SelectionRangeRegistrationOptions.
But looking at the current specification, it doesn't seem to add too much value. I think we can just simplify return a bool for now (as you did in this patch).

Yeah. So should I remove the client capability since we do not use it and just return bool as now?

clang-tools-extra/clangd/ClangdLSPServer.cpp

1131 ↗

(On Diff #220710)

We do right ?
If the size != 1 then we just return an error.

clang-tools-extra/clangd/Protocol.h

1248 ↗

(On Diff #221499)

I don't think it is possible to do that since the type (SelectionRange) would be incomplete at that point. For example size of this class cannot be computed.

[hokein]

mostly good, a few nits.

clang-tools-extra/clangd/ClangdLSPServer.cpp

136 ↗

(On Diff #221510)

nit: remove the {}, in LLVM we prefer no {} for a single-statement body.

1169 ↗

(On Diff #221510)

does Reply({render(std::move(*Ranges))}); work?

1131 ↗

(On Diff #220710)

yes, it is correct now. maybe I read the wrong snapshot before.

clang-tools-extra/clangd/Protocol.h

422 ↗

(On Diff #221510)

I think we could drop this field now, it is not used, as we always return selectionRangeProvider:true.

1241 ↗

(On Diff #221510)

nit: please remove the markdown elements around the range in the comment.

1251 ↗

(On Diff #221510)

Are these constructors needed?

1248 ↗

(On Diff #221499)

ah, good point, I see. We could just use std::unique_ptr<SelectionRange> parent;.

[usaxena95]

Addressed comments.

clang-tools-extra/clangd/ClangdLSPServer.cpp

1169 ↗

(On Diff #221510)

List initialization of vector of move only types is painful 😃
Even this does not work: Reply(std::vector<SelectionRange>{render(std::move(*Ranges))}); because I think it vector tries to copy construct it because of initializer_lists

clang-tools-extra/clangd/Protocol.h

1251 ↗

(On Diff #221510)

We don't need them if we use just unique_ptr. They were needed before in Optional<unique_ptr>. Somehow it was not able to deduce this is a trivially-constructible + move only class.

[hokein]

thanks, looks good.

clang-tools-extra/clangd/Protocol.cpp

18 ↗

(On Diff #221521)

the header seems not used?

[usaxena95]

Removed ununsed header.

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

void onHover(const TextDocumentPositionParams &,
                Callback<llvm::Optional<Hover>>);
   void onTypeHierarchy(const TypeHierarchyParams &,
                        Callback<llvm::Optional<TypeHierarchyItem>>);
   void onResolveTypeHierarchy(const ResolveTypeHierarchyItemParams &,
                               Callback<llvm::Optional<TypeHierarchyItem>>);
   void onChangeConfiguration(const DidChangeConfigurationParams &);
   void onSymbolInfo(const TextDocumentPositionParams &,
                     Callback<std::vector<SymbolDetails>>);
+  void onSelectionRange(const SelectionRangeParams &,
+                        Callback<std::vector<SelectionRange>>);
 
   std::vector<Fix> getFixes(StringRef File, const clangd::Diagnostic &D);
 
   /// Checks if completion request should be ignored. We need this due to the
   /// limitation of the LSP. Per LSP, a client sends requests for all "trigger
   /// character" we specify, but for '>' and ':' we need to check they actually
   /// produce '->' and '::', respectively.
   bool shouldRunCompletion(const CompletionParams &Params) const;

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

 #include "Trace.h"
 #include "URI.h"
 #include "refactor/Tweak.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SHA1.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include <cstddef>
+#include <memory>
 #include <string>
+#include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 /// Transforms a tweak into a code action that would apply it if executed.
 /// EXPECTS: T.prepare() was called and returned true.
 CodeAction toCodeAction(const ClangdServer::TweakRef &T, const URIForFile &File,
                         Range Selection) {
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
llvm::json::Object Result{
              llvm::json::Object{
                  {"triggerCharacters", {"(", ","}},
              }},
             {"declarationProvider", true},
             {"definitionProvider", true},
             {"documentHighlightProvider", true},
             {"hoverProvider", true},
             {"renameProvider", std::move(RenameProvider)},
+            {"selectionRangeProvider", true},
             {"documentSymbolProvider", true},
             {"workspaceSymbolProvider", true},
             {"referencesProvider", true},
             {"executeCommandProvider",
              llvm::json::Object{
                  {"commands",
                   {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND,
                    ExecuteCommandParams::CLANGD_APPLY_TWEAK}},

 }
 
 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],
+      [Reply = std::move(Reply)](
+          llvm::Expected<std::vector<Range>> Ranges) mutable {
+        if (!Ranges) {
+          return Reply(Ranges.takeError());
+        }
+        std::vector<SelectionRange> Result;
+        Result.emplace_back(render(std::move(*Ranges)));
+        return Reply(std::move(Result));
+      });
+}
+
 ClangdLSPServer::ClangdLSPServer(
     class Transport &Transp, const FileSystemProvider &FSProvider,
     const clangd::CodeCompleteOptions &CCOpts,
     llvm::Optional<Path> CompileCommandsDir, bool UseDirBasedCDB,
     llvm::Optional<OffsetEncoding> ForcedOffsetEncoding,
     const ClangdServer::Options &Opts)
     : Transp(Transp), MsgHandler(new MessageHandler(*this)),
       FSProvider(FSProvider), CCOpts(CCOpts),
ClangdLSPServer::ClangdLSPServer(
   MsgHandler->bind("textDocument/didOpen", &ClangdLSPServer::onDocumentDidOpen);
   MsgHandler->bind("textDocument/didClose", &ClangdLSPServer::onDocumentDidClose);
   MsgHandler->bind("textDocument/didChange", &ClangdLSPServer::onDocumentDidChange);
   MsgHandler->bind("workspace/didChangeWatchedFiles", &ClangdLSPServer::onFileEvent);
   MsgHandler->bind("workspace/didChangeConfiguration", &ClangdLSPServer::onChangeConfiguration);
   MsgHandler->bind("textDocument/symbolInfo", &ClangdLSPServer::onSymbolInfo);
   MsgHandler->bind("textDocument/typeHierarchy", &ClangdLSPServer::onTypeHierarchy);
   MsgHandler->bind("typeHierarchy/resolve", &ClangdLSPServer::onResolveTypeHierarchy);
+  MsgHandler->bind("textDocument/selectionRange", &ClangdLSPServer::onSelectionRange);
   // clang-format on
 }
 
 ClangdLSPServer::~ClangdLSPServer() { IsBeingDestroyed = true; }
 
 bool ClangdLSPServer::run() {
   // Run the Language Server loop.
   bool CleanExit = true;

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

 
 #include "URI.h"
 #include "index/SymbolID.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 #include <bitset>
+#include <memory>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 enum class ErrorCode {
   // Defined by JSON RPC.

 struct SemanticHighlightingParams {
   /// The textdocument these highlightings belong to.
   TextDocumentIdentifier TextDocument;
   /// The lines of highlightings that should be sent.
   std::vector<SemanticHighlightingInformation> Lines;
 };
 llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting);
 
+struct SelectionRangeParams {
+  /// The text document.
+  TextDocumentIdentifier textDocument;
+
+  /// The positions inside the text document.
+  std::vector<Position> positions;
+};
+bool fromJSON(const llvm::json::Value &, SelectionRangeParams &);
+
+struct SelectionRange {
+  /**
+   * The range of this selection range.
+   */
+  Range range;
+  /**
+   * The parent selection range containing this range. Therefore `parent.range`
+   * must contain `this.range`.
+   */
+  std::unique_ptr<SelectionRange> parent;
+};
+llvm::json::Value toJSON(const SelectionRange &);
+
 } // namespace clangd
 } // namespace clang
 
 namespace llvm {
 template <> struct format_provider<clang::clangd::Position> {
   static void format(const clang::clangd::Position &Pos, raw_ostream &OS,
                      StringRef Style) {
     assert(Style.empty() && "style modifiers for this type are not supported");
     OS << Pos;
   }
 };
 } // namespace llvm
 
 #endif

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

 
 llvm::json::Value toJSON(const SemanticHighlightingParams &Highlighting) {
   return llvm::json::Object{
       {"textDocument", Highlighting.TextDocument},
       {"lines", std::move(Highlighting.Lines)},
   };
 }
 
+bool fromJSON(const llvm::json::Value &Params, SelectionRangeParams &P) {
+  llvm::json::ObjectMapper O(Params);
+  return O && O.map("textDocument", P.textDocument) &&
+         O.map("positions", P.positions);
+}
+
+llvm::json::Value toJSON(const SelectionRange &Out) {
+  if (Out.parent) {
+    return llvm::json::Object{{"range", Out.range},
+                              {"parent", toJSON(*Out.parent)}};
+  }
+  return llvm::json::Object{{"range", Out.range}};
+}
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/clangd/test/initialize-params.test

 # CHECK-NEXT:        "commands": [
 # CHECK-NEXT:          "clangd.applyFix",
 # CHECK-NEXT:          "clangd.applyTweak"
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "hoverProvider": true,
 # CHECK-NEXT:      "referencesProvider": true,
 # CHECK-NEXT:      "renameProvider": true,
+# CHECK-NEXT:      "selectionRangeProvider": true,
 # CHECK-NEXT:      "signatureHelpProvider": {
 # CHECK-NEXT:        "triggerCharacters": [
 # CHECK-NEXT:          "(",
 # CHECK-NEXT:          ","
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "textDocumentSync": 2,
 # CHECK-NEXT:      "typeHierarchyProvider": true

clang-tools-extra/trunk/clangd/test/selection-range.test

+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void func() {\n}"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/selectionRange","params":{"textDocument":{"uri":"test:///main.cpp"},"positions":[{"line":1,"character":0}]}}
+#      CHECK:  "id": 1
+# CHECK-NEXT:  "jsonrpc": "2.0",
+# CHECK-NEXT:  "result": [
+# CHECK-NEXT:    {
+# CHECK-NEXT:      "parent": {
+# CHECK-NEXT:        "range": {
+# CHECK-NEXT:          "end": {
+# CHECK-NEXT:            "character": 1,
+# CHECK-NEXT:            "line": 1
+# CHECK-NEXT:          },
+# CHECK-NEXT:          "start": {
+# CHECK-NEXT:            "character": 0,
+# CHECK-NEXT:            "line": 0
+# CHECK-NEXT:          }
+# CHECK-NEXT:        }
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "range": {
+# CHECK-NEXT:        "end": {
+# CHECK-NEXT:          "character": 1,
+# CHECK-NEXT:          "line": 1
+# CHECK-NEXT:        },
+# CHECK-NEXT:        "start": {
+# CHECK-NEXT:          "character": 12,
+# CHECK-NEXT:          "line": 0
+# CHECK-NEXT:        }
+# CHECK-NEXT:      }
+# CHECK-NEXT:    }
+# CHECK-NEXT:  ]
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}