Switch to upstream protocol for resolving code action

matklad · matklad · commit 7d2eb000b078 · 2020-11-10T18:48:46.000+01:00
Note that we have to maintain custom implementation on the client
side: I don't see how to marry bulitin resolve support with groups and
snippets.
diff --git a/crates/rust-analyzer/src/caps.rs b/crates/rust-analyzer/src/caps.rs
@@ -16,8 +16,6 @@ use serde_json::json;
 use crate::semantic_tokens;
 
 pub fn server_capabilities(client_caps: &ClientCapabilities) -> ServerCapabilities {
-    let code_action_provider = code_action_capabilities(client_caps);
-
     ServerCapabilities {
         text_document_sync: Some(TextDocumentSyncCapability::Options(TextDocumentSyncOptions {
             open_close: Some(true),
@@ -49,7 +47,7 @@ pub fn server_capabilities(client_caps: &ClientCapabilities) -> ServerCapabiliti
         document_highlight_provider: Some(OneOf::Left(true)),
         document_symbol_provider: Some(OneOf::Left(true)),
         workspace_symbol_provider: Some(OneOf::Left(true)),
-        code_action_provider: Some(code_action_provider),
+        code_action_provider: Some(code_action_capabilities(client_caps)),
         code_lens_provider: Some(CodeLensOptions { resolve_provider: Some(true) }),
         document_formatting_provider: Some(OneOf::Left(true)),
         document_range_formatting_provider: None,
@@ -113,7 +111,7 @@ fn code_action_capabilities(client_caps: &ClientCapabilities) -> CodeActionProvi
                     CodeActionKind::REFACTOR_INLINE,
                     CodeActionKind::REFACTOR_REWRITE,
                 ]),
-                resolve_provider: None,
+                resolve_provider: Some(true),
                 work_done_progress_options: Default::default(),
             })
         })
diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs
@@ -144,7 +144,7 @@ pub struct ClientCapsConfig {
     pub code_action_literals: bool,
     pub work_done_progress: bool,
     pub code_action_group: bool,
-    pub resolve_code_action: bool,
+    pub code_action_resolve: bool,
     pub hover_actions: bool,
     pub status_notification: bool,
     pub signature_help_label_offsets: bool,
@@ -383,6 +383,14 @@ impl Config {
                     }
                 }
             }
+
+            if let Some(code_action) = &doc_caps.code_action {
+                if let Some(resolve_support) = &code_action.resolve_support {
+                    if resolve_support.properties.iter().any(|it| it == "edit") {
+                        self.client_caps.code_action_resolve = true;
+                    }
+                }
+            }
         }
 
         if let Some(window_caps) = caps.window.as_ref() {
@@ -400,7 +408,6 @@ impl Config {
             self.assist.allow_snippets(snippet_text_edit);
 
             self.client_caps.code_action_group = get_bool("codeActionGroup");
-            self.client_caps.resolve_code_action = get_bool("resolveCodeAction");
             self.client_caps.hover_actions = get_bool("hoverActions");
             self.client_caps.status_notification = get_bool("statusNotification");
         }
diff --git a/crates/rust-analyzer/src/diagnostics/test_data/rustc_unused_variable.txt b/crates/rust-analyzer/src/diagnostics/test_data/rustc_unused_variable.txt
@@ -36,7 +36,6 @@
         fixes: [
             CodeAction {
                 title: "consider prefixing with an underscore",
-                id: None,
                 group: None,
                 kind: Some(
                     CodeActionKind(
@@ -70,6 +69,7 @@
                 is_preferred: Some(
                     true,
                 ),
+                data: None,
             },
         ],
     },
diff --git a/crates/rust-analyzer/src/diagnostics/test_data/rustc_unused_variable_as_hint.txt b/crates/rust-analyzer/src/diagnostics/test_data/rustc_unused_variable_as_hint.txt
@@ -36,7 +36,6 @@
         fixes: [
             CodeAction {
                 title: "consider prefixing with an underscore",
-                id: None,
                 group: None,
                 kind: Some(
                     CodeActionKind(
@@ -70,6 +69,7 @@
                 is_preferred: Some(
                     true,
                 ),
+                data: None,
             },
         ],
     },
diff --git a/crates/rust-analyzer/src/diagnostics/test_data/rustc_unused_variable_as_info.txt b/crates/rust-analyzer/src/diagnostics/test_data/rustc_unused_variable_as_info.txt
@@ -36,7 +36,6 @@
         fixes: [
             CodeAction {
                 title: "consider prefixing with an underscore",
-                id: None,
                 group: None,
                 kind: Some(
                     CodeActionKind(
@@ -70,6 +69,7 @@
                 is_preferred: Some(
                     true,
                 ),
+                data: None,
             },
         ],
     },
diff --git a/crates/rust-analyzer/src/diagnostics/test_data/snap_multi_line_fix.txt b/crates/rust-analyzer/src/diagnostics/test_data/snap_multi_line_fix.txt
@@ -51,7 +51,6 @@
         fixes: [
             CodeAction {
                 title: "return the expression directly",
-                id: None,
                 group: None,
                 kind: Some(
                     CodeActionKind(
@@ -98,6 +97,7 @@
                 is_preferred: Some(
                     true,
                 ),
+                data: None,
             },
         ],
     },
diff --git a/crates/rust-analyzer/src/diagnostics/to_proto.rs b/crates/rust-analyzer/src/diagnostics/to_proto.rs
@@ -110,7 +110,6 @@ fn map_rust_child_diagnostic(
     } else {
         MappedRustChildDiagnostic::SuggestedFix(lsp_ext::CodeAction {
             title: rd.message.clone(),
-            id: None,
             group: None,
             kind: Some(lsp_types::CodeActionKind::QUICKFIX),
             edit: Some(lsp_ext::SnippetWorkspaceEdit {
@@ -119,6 +118,7 @@ fn map_rust_child_diagnostic(
                 document_changes: None,
             }),
             is_preferred: Some(true),
+            data: None,
         })
     }
 }
diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs
@@ -806,11 +806,11 @@ fn handle_fixes(
         let edit = to_proto::snippet_workspace_edit(&snap, fix.source_change)?;
         let action = lsp_ext::CodeAction {
             title: fix.label.to_string(),
-            id: None,
             group: None,
             kind: Some(CodeActionKind::QUICKFIX),
             edit: Some(edit),
             is_preferred: Some(false),
+            data: None,
         };
         res.push(action);
     }
@@ -852,11 +852,11 @@ pub(crate) fn handle_code_action(
 
     handle_fixes(&snap, &params, &mut res)?;
 
-    if snap.config.client_caps.resolve_code_action {
+    if snap.config.client_caps.code_action_resolve {
         for (index, assist) in
             snap.analysis.unresolved_assists(&snap.config.assist, frange)?.into_iter().enumerate()
         {
-            res.push(to_proto::unresolved_code_action(&snap, assist, index)?);
+            res.push(to_proto::unresolved_code_action(&snap, params.clone(), assist, index)?);
         }
     } else {
         for assist in snap.analysis.resolved_assists(&snap.config.assist, frange)?.into_iter() {
@@ -867,11 +867,16 @@ pub(crate) fn handle_code_action(
     Ok(Some(res))
 }
 
-pub(crate) fn handle_resolve_code_action(
+pub(crate) fn handle_code_action_resolve(
     mut snap: GlobalStateSnapshot,
-    params: lsp_ext::ResolveCodeActionParams,
-) -> Result<Option<lsp_ext::SnippetWorkspaceEdit>> {
-    let _p = profile::span("handle_resolve_code_action");
+    mut code_action: lsp_ext::CodeAction,
+) -> Result<lsp_ext::CodeAction> {
+    let _p = profile::span("handle_code_action_resolve");
+    let params = match code_action.data.take() {
+        Some(it) => it,
+        None => Err("can't resolve code action without data")?,
+    };
+
     let file_id = from_proto::file_id(&snap, &params.code_action_params.text_document.uri)?;
     let line_index = snap.analysis.file_line_index(file_id)?;
     let range = from_proto::text_range(&line_index, params.code_action_params.range);
@@ -888,7 +893,9 @@ pub(crate) fn handle_resolve_code_action(
     let index = index.parse::<usize>().unwrap();
     let assist = &assists[index];
     assert!(assist.assist.id.0 == id);
-    Ok(to_proto::resolved_code_action(&snap, assist.clone())?.edit)
+    let edit = to_proto::resolved_code_action(&snap, assist.clone())?.edit;
+    code_action.edit = edit;
+    Ok(code_action)
 }
 
 pub(crate) fn handle_code_lens(
diff --git a/crates/rust-analyzer/src/lsp_ext.rs b/crates/rust-analyzer/src/lsp_ext.rs
@@ -113,22 +113,6 @@ pub struct JoinLinesParams {
     pub ranges: Vec<Range>,
 }
 
-pub enum ResolveCodeActionRequest {}
-
-impl Request for ResolveCodeActionRequest {
-    type Params = ResolveCodeActionParams;
-    type Result = Option<SnippetWorkspaceEdit>;
-    const METHOD: &'static str = "experimental/resolveCodeAction";
-}
-
-/// Params for the ResolveCodeActionRequest
-#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
-#[serde(rename_all = "camelCase")]
-pub struct ResolveCodeActionParams {
-    pub code_action_params: lsp_types::CodeActionParams,
-    pub id: String,
-}
-
 pub enum OnEnter {}
 
 impl Request for OnEnter {
@@ -265,13 +249,18 @@ impl Request for CodeActionRequest {
     const METHOD: &'static str = "textDocument/codeAction";
 }
 
+pub enum CodeActionResolveRequest {}
+impl Request for CodeActionResolveRequest {
+    type Params = CodeAction;
+    type Result = CodeAction;
+    const METHOD: &'static str = "codeAction/resolve";
+}
+
 #[derive(Debug, PartialEq, Clone, Default, Deserialize, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct CodeAction {
     pub title: String,
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub id: Option<String>,
-    #[serde(skip_serializing_if = "Option::is_none")]
     pub group: Option<String>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub kind: Option<CodeActionKind>,
@@ -282,6 +271,16 @@ pub struct CodeAction {
     pub edit: Option<SnippetWorkspaceEdit>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub is_preferred: Option<bool>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub data: Option<CodeActionData>,
+}
+
+#[derive(Debug, Eq, PartialEq, Clone, Deserialize, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub struct CodeActionData {
+    pub code_action_params: lsp_types::CodeActionParams,
+    pub id: String,
 }
 
 #[derive(Debug, Eq, PartialEq, Clone, Default, Deserialize, Serialize)]
diff --git a/crates/rust-analyzer/src/main_loop.rs b/crates/rust-analyzer/src/main_loop.rs
@@ -435,7 +435,7 @@ impl GlobalState {
             .on::<lsp_ext::Runnables>(handlers::handle_runnables)
             .on::<lsp_ext::InlayHints>(handlers::handle_inlay_hints)
             .on::<lsp_ext::CodeActionRequest>(handlers::handle_code_action)
-            .on::<lsp_ext::ResolveCodeActionRequest>(handlers::handle_resolve_code_action)
+            .on::<lsp_ext::CodeActionResolveRequest>(handlers::handle_code_action_resolve)
             .on::<lsp_ext::HoverRequest>(handlers::handle_hover)
             .on::<lsp_ext::ExternalDocs>(handlers::handle_open_docs)
             .on::<lsp_types::request::OnTypeFormatting>(handlers::handle_on_type_formatting)
diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs
@@ -735,16 +735,20 @@ pub(crate) fn code_action_kind(kind: AssistKind) -> lsp_types::CodeActionKind {
 
 pub(crate) fn unresolved_code_action(
     snap: &GlobalStateSnapshot,
+    code_action_params: lsp_types::CodeActionParams,
     assist: Assist,
     index: usize,
 ) -> Result<lsp_ext::CodeAction> {
     let res = lsp_ext::CodeAction {
         title: assist.label.to_string(),
-        id: Some(format!("{}:{}", assist.id.0, index.to_string())),
         group: assist.group.filter(|_| snap.config.client_caps.code_action_group).map(|gr| gr.0),
         kind: Some(code_action_kind(assist.id.1)),
         edit: None,
         is_preferred: None,
+        data: Some(lsp_ext::CodeActionData {
+            id: format!("{}:{}", assist.id.0, index.to_string()),
+            code_action_params,
+        }),
     };
     Ok(res)
 }
@@ -754,13 +758,19 @@ pub(crate) fn resolved_code_action(
     assist: ResolvedAssist,
 ) -> Result<lsp_ext::CodeAction> {
     let change = assist.source_change;
-    unresolved_code_action(snap, assist.assist, 0).and_then(|it| {
-        Ok(lsp_ext::CodeAction {
-            id: None,
-            edit: Some(snippet_workspace_edit(snap, change)?),
-            ..it
-        })
-    })
+    let res = lsp_ext::CodeAction {
+        edit: Some(snippet_workspace_edit(snap, change)?),
+        title: assist.assist.label.to_string(),
+        group: assist
+            .assist
+            .group
+            .filter(|_| snap.config.client_caps.code_action_group)
+            .map(|gr| gr.0),
+        kind: Some(code_action_kind(assist.assist.id.1)),
+        is_preferred: None,
+        data: None,
+    };
+    Ok(res)
 }
 
 pub(crate) fn runnable(
diff --git a/docs/dev/lsp-extensions.md b/docs/dev/lsp-extensions.md
@@ -1,5 +1,5 @@
 <!---
-lsp_ext.rs hash: 286f8bbac885531a
+lsp_ext.rs hash: 4f86fb54e4b2870e
 
 If you need to change the above hash to make the test pass, please check if you
 need to adjust this doc as well and ping this issue:
@@ -109,30 +109,6 @@ Invoking code action at this position will yield two code actions for importing
 * Is a fixed two-level structure enough?
 * Should we devise a general way to encode custom interaction protocols for GUI refactorings?
 
-## Lazy assists with `ResolveCodeAction`
-
-**Issue:** https://github.com/microsoft/language-server-protocol/issues/787
-
-**Client Capability** `{ "resolveCodeAction": boolean }`
-
-If this capability is set, the assists will be computed lazily. Thus `CodeAction` returned from the server will only contain `id` but not `edit` or `command` fields. The only exclusion from the rule is the diagnostic edits.
-
-After the client got the id, it should then call `experimental/resolveCodeAction` command on the server and provide the following payload:
-
-```typescript
-interface ResolveCodeActionParams {
-    id: string;
-    codeActionParams: lc.CodeActionParams;
-}
-```
-
-As a result of the command call the client will get the respective workspace edit (`lc.WorkspaceEdit`).
-
-### Unresolved Questions
-
-* Apply smarter filtering for ids?
-* Upon `resolveCodeAction` command only call the assits which should be resolved and not all of them?
-
 ## Parent Module
 
 **Issue:** https://github.com/microsoft/language-server-protocol/issues/1002
diff --git a/editors/code/src/client.ts b/editors/code/src/client.ts
diff --git a/editors/code/src/commands.ts b/editors/code/src/commands.ts
diff --git a/editors/code/src/lsp_ext.ts b/editors/code/src/lsp_ext.ts

Original file line number	Diff line number	Diff line change
`@@ -144,7 +144,7 @@ pub struct ClientCapsConfig {`
`144`	`144`	`pub code_action_literals: bool,`
`145`	`145`	`pub work_done_progress: bool,`
`146`	`146`	`pub code_action_group: bool,`
`147`		`- pub resolve_code_action: bool,`
	`147`	`+ pub code_action_resolve: bool,`
`148`	`148`	`pub hover_actions: bool,`
`149`	`149`	`pub status_notification: bool,`
`150`	`150`	`pub signature_help_label_offsets: bool,`
`@@ -383,6 +383,14 @@ impl Config {`
`383`	`383`	`}`
`384`	`384`	`}`
`385`	`385`	`}`
	`386`	`+`
	`387`	`+ if let Some(code_action) = &doc_caps.code_action {`
	`388`	`+ if let Some(resolve_support) = &code_action.resolve_support {`
	`389`	`+ if resolve_support.properties.iter().any(\|it\| it == "edit") {`
	`390`	`+ self.client_caps.code_action_resolve = true;`
	`391`	`+ }`
	`392`	`+ }`
	`393`	`+ }`
`386`	`394`	`}`
`387`	`395`
`388`	`396`	`if let Some(window_caps) = caps.window.as_ref() {`
`@@ -400,7 +408,6 @@ impl Config {`
`400`	`408`	`self.assist.allow_snippets(snippet_text_edit);`
`401`	`409`
`402`	`410`	`self.client_caps.code_action_group = get_bool("codeActionGroup");`
`403`		`- self.client_caps.resolve_code_action = get_bool("resolveCodeAction");`
`404`	`411`	`self.client_caps.hover_actions = get_bool("hoverActions");`
`405`	`412`	`self.client_caps.status_notification = get_bool("statusNotification");`
`406`	`413`	`}`