Add ExecuteCommand/ApplyEdit #114

rockbruno · 2019-06-28T13:57:14Z

Part 2 to #70: This adds the skeleton for command execution and the resulting edits that are sent back to client.

The thing to note here is that ExecuteCommand isn't a TextDocumentRequest in the LSP, but we still need to determine which server we send the request to. My solution for this involved creating a special type of SwiftCommand that can be detected by the use of a swift.lsp prefix in the command identifier, but I would love to know if there are cleaner ways to handle this.

EDIT: It now simply intercepts the CodeAction result and appends a metadata object containing the URL.

rockbruno · 2019-06-28T13:59:05Z

Sources/SourceKit/SourceKitServer.swift

@@ -269,6 +270,9 @@ extension SourceKitServer {
        clientCapabilities: req.params.capabilities.textDocument?.codeAction,
        codeActionOptions: CodeActionOptions(codeActionKinds: nil),
        supportsCodeActions: false // TODO: Turn it on after a provider is implemented.
+      ),
+      executeCommandProvider: ExecuteCommandOptions(
+        commands: builtinSwiftCommands // FIXME: Clangd commands?


VSCode doesn't call the request if a command isn't defined by the server - should we worry about clangd here? I noticed that it has an initialize request of its own, but I was unsure if what we define here affects it.

You can make any steps to get closer to opening it still learning

Hmm, that's unfortunate. We load clangd dynamically through the toolchain, so there's no good way for us to get the response from clang this early. Maybe someday this would be a good place for the specification to evolve to add dynamic capabilities for the server (there are already dynamic capabilities for the client). As a short term hack, we could maintain a list of every command clangd supports, but that doesn't need to be done in this patch.

For now, this is fine.

rockbruno · 2019-06-28T14:00:18Z

Sources/SourceKit/sourcekitd/SwiftCommand.swift

+  }
+
+  /// Converts this `SwiftCommand` to a generic LSP `Command` object.
+  public func asCommand() throws -> Command {


You can see how this gets used here:
https://github.com/apple/sourcekit-lsp/pull/70/files#diff-6175f3a32db6757aaeb5b49a98bab346R949

rockbruno · 2019-06-28T14:01:01Z

Sources/SourceKit/sourcekitd/SwiftCommand.swift

+  ///
+  /// - Parameters:
+  ///   - type: The `SwiftCommand` metatype to convert to.
+  public func swiftCommand<T: SwiftCommand>(ofType type: T.Type) -> T? {


You can see how this gets used here:
https://github.com/apple/sourcekit-lsp/pull/70/files#diff-6175f3a32db6757aaeb5b49a98bab346R969

rockbruno · 2019-06-28T14:02:03Z

Sources/SourceKit/sourcekitd/SwiftLanguageServer.swift

@@ -888,6 +891,34 @@ extension SwiftLanguageServer {
      }
    }
  }
+
+  func executeCommand(_ req: Request<ExecuteCommandRequest>) {


It's outdated, but the actual request is here:
https://github.com/apple/sourcekit-lsp/pull/70/files#diff-6175f3a32db6757aaeb5b49a98bab346R966

rockbruno · 2019-06-28T14:06:07Z

Tests/SourceKitTests/ExecuteCommandTests.swift

+    let haveClangd = ToolchainRegistry.shared.toolchains.contains { $0.clangd != nil }
+    if !haveClangd {
+      XCTFail("Cannot find clangd in toolchain")
+    }


I actually wasn't able to get this or anything in LocalClangTests to pass, I haven't got the right toolchains yet.

hanjoes · 2019-07-01T15:59:39Z

Sources/SourceKit/SourceKitServer.swift

+
+  func serviceFor(executeCommandRequest req: ExecuteCommandRequest, workspace: Workspace) -> Connection? {
+    // FIXME: ExecuteCommand requests have no URL associated to it, but we need to determine the server
+    // that it gets sent to. There should be a better way to do this.


can we pass url back in the argument field of Commands returned by CodeActionResponse?

We can for Swift, but AFAIK we have no control over what clangd's commands will look like

please correct me if i'm missing something, I assume parameters passed in the CodeActionResponse will be sent back to server when user picks the command. If that's the case, even for Clangd, server should still be the best place to generate those arguments, no?

True, I believe we can intercept the response from clang and inject the url that called the code action itself into the arguments. It should be enough to avoid this logic, so we just need to confirm that doing so won't be a problem for clangd.

rockbruno · 2019-07-08T11:26:03Z

Done, I added a result handler to the CodeActions that intercepts the original result and adds a metadata object to the commands. Worked great!

benlangmuir · 2019-07-12T15:34:57Z

Sources/LanguageServerProtocol/CodeAction.swift

+  public func injectMetadata(atResponse response: CodeActionRequestResponse?) -> CodeActionRequestResponse? {
+    let metadata = SourceKitLSPCommandMetadata(textDocument: textDocument)
+    guard let data = try? JSONEncoder().encode(metadata),
+          let metadataArgument = try? JSONDecoder().decode(CommandArgumentType.self, from: data) else


We should just construct the dictionary instead of round-tripping through json. The json (de) serialization code has a relatively high up front cost even for small payloads.

Ah, I was trying to make the compiler synthetize the usage of CommandArguments as much as possible to make it easy to add and edit commands... I changed these guys to encode and decode the dictionaries manually, but let me know if there's something that can be done to reduce the boilerplate.

I'm not aware of anything good here. In principle you could write a Decoder implementation that works directly from the dictionary - basically perform the object mapping part of JSONDecoder without doing any of the parsing. But that seems unnecessarily general to implement in sourcekit-lsp, so unless someone adds such a thing to the stdlib or Foundation I don't see us using it.

benlangmuir · 2019-07-12T15:35:55Z

Sources/LanguageServerProtocol/CodeAction.swift

@@ -47,6 +48,30 @@ public struct CodeActionRequest: TextDocumentRequest, Hashable {
    self.context = context
    self.textDocument = textDocument
  }
+
+  public func injectMetadata(atResponse response: CodeActionRequestResponse?) -> CodeActionRequestResponse? {


This implementation is not really a general feature of CodeAction, so it should probably move into an extension the SourceKit module.

benlangmuir · 2019-07-12T15:37:37Z

Sources/LanguageServerProtocol/ExecuteCommand.swift

+  }
+
+  /// Optional metadata containing SourceKit-LSP infomration about this command.
+  public var metadata: SourceKitLSPCommandMetadata? {


Same here, I'd prefer this moved to extensions in the SourceKit module.

benlangmuir · 2019-07-12T15:38:46Z

Sources/LanguageServerProtocol/ExecuteCommand.swift

+    guard let data = try? JSONEncoder().encode(dictionary) else {
+      return nil
+    }
+    return try? JSONDecoder().decode(SourceKitLSPCommandMetadata.self, from: data)


Similar to above, we already have a dictionary, so we should map it to our metadata directly without going back through json.

benlangmuir · 2019-07-12T15:45:23Z

Sources/SourceKit/SourceKitServer.swift

@@ -269,6 +270,9 @@ extension SourceKitServer {
        clientCapabilities: req.params.capabilities.textDocument?.codeAction,
        codeActionOptions: CodeActionOptions(codeActionKinds: nil),
        supportsCodeActions: false // TODO: Turn it on after a provider is implemented.
+      ),
+      executeCommandProvider: ExecuteCommandOptions(
+        commands: builtinSwiftCommands // FIXME: Clangd commands?


Hmm, that's unfortunate. We load clangd dynamically through the toolchain, so there's no good way for us to get the response from clang this early. Maybe someday this would be a good place for the specification to evolve to add dynamic capabilities for the server (there are already dynamic capabilities for the client). As a short term hack, we could maintain a list of every command clangd supports, but that doesn't need to be done in this patch.

For now, this is fine.

benlangmuir · 2019-07-12T15:49:55Z

Sources/LanguageServerProtocol/ExecuteCommand.swift

+/// Represents metadata that SourceKit-LSP injects at every command returned by code actions.
+/// The ExecuteCommand is not a TextDocumentRequest, so metadata is injected to allow SourceKit-LSP
+/// to determine where a command should be executed.
+public struct SourceKitLSPCommandMetadata: Codable, Hashable {


If we're going to inject this into clang responses, I think we need to mangle all the dictionary keys so it can't collide with anything (e.g. sourcekitlsp_textDocument).

benlangmuir · 2019-07-12T15:51:34Z

Sources/SourceKit/SourceKitServer.swift

+  }
+
+  func executeCommand(_ req: Request<ExecuteCommandRequest>, workspace: Workspace) {
+    guard let url = req.params.textDocument?.url else {


I wonder if we should strip the metadata off before sending it to clang. What do you think?

Yeah, since at this point the correct server can be determined we can strip it for Swift as well. Should make things more predictable 👍

benlangmuir · 2019-07-12T15:52:34Z

Sources/SourceKit/sourcekitd/SwiftCommand.swift

+  /// Converts this `SwiftCommand` to a generic LSP `Command` object.
+  public func asCommand() throws -> Command {
+    let data = try JSONEncoder().encode(self)
+    let argument = try JSONDecoder().decode(CommandArgumentType.self, from: data)


More round-tripping through JSON that we should avoid.

rockbruno · 2019-09-12T13:04:02Z

Rebased from master

benlangmuir

Hey @rockbruno! I saw that you updated this recently, but I didn't realize it that even before that it had been waiting on me. I must have missed the notification when you last changed. Sorry for the long delay! Please feel free to ping me in the future if I am not responding in a timely fashion.

I have a few minor requests, but otherwise I think this is ready to go. It's not ideal that we can't test the changes to SourceKitServer yet, but not the end of the world. On the other hand, I think we should defer the specific SwiftCommand bits until we can use them.

Thanks again for working on this!

benlangmuir · 2019-09-24T18:13:05Z

Sources/LanguageServerProtocol/WorkspaceEdit.swift

+public struct WorkspaceEdit: Codable, Hashable, ResponseType {
+
+  /// The edits to be applied to existing resources.
+  public var changes: [String: [TextEdit]]?


Why not use URL directly here? Everywhere else that uses DocumentUri we are using URL I think.

benlangmuir · 2019-09-24T18:16:26Z

Sources/SourceKit/SourceKitLSPCommandMetadata.swift

+/// to determine where a command should be executed.
+public struct SourceKitLSPCommandMetadata: Codable, Hashable {
+
+  public static func decode(fromDictionary dictionary: [String: LSPAny]) -> SourceKitLSPCommandMetadata? {


Let's rearrange this so that the instance fields and init are at the top, and the decoding and encoding are together.

Also, how about changing the signature to init?(fromLSPDictionary:)? Seems like it belongs in an init.

benlangmuir · 2019-09-24T18:17:15Z

Sources/SourceKit/SourceKitLSPCommandMetadata.swift

+}
+
+extension CodeActionRequest {
+  public func injectMetadata(atResponse response: CodeActionRequestResponse?) -> CodeActionRequestResponse? {


Nitpick: toResponse seems more grammatical to me

benlangmuir · 2019-09-24T18:18:13Z

Sources/SourceKit/SourceKitLSPCommandMetadata.swift

+  }
+
+  /// Returns this Command's arguments without SourceKit-LSP's injected metadata, if it exists.
+  public var argumentsWithoutLSPMetadata: [LSPAny]? {


WithoutSourceKitMetadata seems more correct, since it's not general to LSP, but specific to SK.

benlangmuir · 2019-09-24T18:20:33Z

Sources/SourceKit/SourceKitServer.swift

@@ -628,16 +650,28 @@ extension SourceKitServer {
  func toolchainTextDocumentRequest<PositionRequest>(
    _ req: Request<PositionRequest>,
    workspace: Workspace,
+    resultHandler: ((LSPResult<PositionRequest.Response>) -> LSPResult<PositionRequest.Response>)? = nil,


How about resultTransformer? To me "handler" implies something else.

benlangmuir · 2019-09-24T18:25:25Z

Sources/SourceKit/sourcekitd/SwiftCommand.swift

+  public var actionString: String
+
+  /// The starting line of the range to refactor.
+  public var line: Int


Why not use a PositionRange?

benlangmuir · 2019-09-24T18:35:26Z

Sources/SourceKit/sourcekitd/SwiftCommand.swift

+public let builtinSwiftCommands: [String] = []
+
+/// A `Command` that should be executed by Swift's language server.
+public protocol SwiftCommand: Codable, Hashable {


It looks like none of this is used yet. The basic design looks okay, but I'd like to leave this out until we have code that uses it (basically defer it to the next PR). WDYT?

benlangmuir · 2019-09-24T18:38:54Z

Sources/SourceKit/SourceKitLSPCommandMetadata.swift

+    self.sourcekitlsp_textDocument = textDocument
+  }
+
+  func asCommandArgument() -> LSPAny {


How about encode(toLSPAny:) to be closer to Codable?

rockbruno · 2019-09-24T21:37:18Z

No worries! I applied the changes here - I just wasn't sure about the encode(toLSPAny:) bit, I named it encodeToLSPAny() directly because we can't really append/construct the LSPAny type from an external reference in that context, but if you have an idea feel free to share it.

benlangmuir · 2019-09-24T21:47:35Z

@swift-ci please test

benlangmuir · 2019-09-24T21:47:49Z

@swift-ci please test

benlangmuir · 2019-09-24T21:48:10Z

@swift-ci please test macOS

benlangmuir · 2019-09-24T22:11:32Z

@swift-ci please test macOS

benlangmuir · 2019-09-24T22:48:00Z

@testable import SourceKit

I disabled testable support so that we can test in a configuration that matches what we want to distribute. This will need to be changed to a normal import. If there are specific internals you want to use, you can make them public, add a leading underscore to the name and put *Public for testing* in their documentation comment.

rockbruno · 2019-09-24T22:57:33Z

Ah that actually wasn't needed for the tests, I think I had that added out of habit for iOS.

benlangmuir · 2019-09-24T23:00:50Z

@swift-ci please test

benlangmuir · 2019-09-24T23:01:10Z

@swift-ci please test

benlangmuir · 2019-09-24T23:01:29Z

@swift-ci please test macOS

benlangmuir · 2019-09-25T15:59:20Z

@swift-ci please test macOS

rockbruno requested a review from benlangmuir as a code owner June 28, 2019 13:57

rockbruno commented Jun 28, 2019

View reviewed changes

rockbruno force-pushed the execute-command branch from 927e46c to 75e4df6 Compare June 28, 2019 14:28

hanjoes reviewed Jul 1, 2019

View reviewed changes

rockbruno force-pushed the execute-command branch from 71026b0 to 393b327 Compare July 8, 2019 11:23

benlangmuir suggested changes Jul 12, 2019

View reviewed changes

rockbruno force-pushed the execute-command branch 2 times, most recently from 447cb87 to ce7b695 Compare July 16, 2019 19:31

rockbruno force-pushed the execute-command branch from ce7b695 to 61f429c Compare September 12, 2019 12:58

benlangmuir suggested changes Sep 24, 2019

View reviewed changes

rockbruno added 5 commits September 24, 2019 17:21

Add ExecuteCommand/ApplyEdit

cef9167

Inject metadata at commands

e71965f

Avoid using decoders for retrieving dictionary data

7180d66

Convert CommandArgumentType -> LSPAny references

c20fe44

Cleaning Code

20c026c

rockbruno force-pushed the execute-command branch from 61f429c to 20c026c Compare September 24, 2019 21:31

Remove @testable from test class

3536217

benlangmuir merged commit 942aaa4 into swiftlang:master Sep 25, 2019

Add ExecuteCommand/ApplyEdit #114

Add ExecuteCommand/ApplyEdit #114

Conversation

rockbruno commented Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockbruno commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockbruno commented Sep 12, 2019

benlangmuir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockbruno commented Sep 24, 2019

benlangmuir commented Sep 24, 2019

benlangmuir commented Sep 24, 2019

benlangmuir commented Sep 24, 2019

benlangmuir commented Sep 24, 2019

benlangmuir commented Sep 24, 2019

rockbruno commented Sep 24, 2019

benlangmuir commented Sep 24, 2019

benlangmuir commented Sep 24, 2019

benlangmuir commented Sep 24, 2019

benlangmuir commented Sep 25, 2019

rockbruno commented Jun 28, 2019 •

edited

Loading