From e1aca75974d917755c04e5040100dfb6ae92f2f9 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 23 Dec 2020 12:58:49 +0300 Subject: [PATCH 1/3] Rename for clarity --- crates/rust-analyzer/src/handlers.rs | 92 ++++++++++++++-------------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index 66f8bee9915..a51a9293fd1 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -865,10 +865,52 @@ pub(crate) fn handle_formatting( } } -fn handle_fixes( +pub(crate) fn handle_code_action( + mut snap: GlobalStateSnapshot, + params: lsp_types::CodeActionParams, +) -> Result>> { + let _p = profile::span("handle_code_action"); + // We intentionally don't support command-based actions, as those either + // requires custom client-code anyway, or requires server-initiated edits. + // Server initiated edits break causality, so we avoid those as well. + if !snap.config.client_caps.code_action_literals { + return Ok(None); + } + + let file_id = from_proto::file_id(&snap, ¶ms.text_document.uri)?; + let line_index = snap.analysis.file_line_index(file_id)?; + let range = from_proto::text_range(&line_index, params.range); + let frange = FileRange { file_id, range }; + + snap.config.assist.allowed = params + .clone() + .context + .only + .map(|it| it.into_iter().filter_map(from_proto::assist_kind).collect()); + + let mut res: Vec = Vec::new(); + + add_quick_fixes(&snap, ¶ms, &mut res)?; + + 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, params.clone(), assist, index)?); + } + } else { + for assist in snap.analysis.resolved_assists(&snap.config.assist, frange)?.into_iter() { + res.push(to_proto::resolved_code_action(&snap, assist)?); + } + } + + Ok(Some(res)) +} + +fn add_quick_fixes( snap: &GlobalStateSnapshot, params: &lsp_types::CodeActionParams, - res: &mut Vec, + acc: &mut Vec, ) -> Result<()> { let file_id = from_proto::file_id(&snap, ¶ms.text_document.uri)?; let line_index = snap.analysis.file_line_index(file_id)?; @@ -902,7 +944,7 @@ fn handle_fixes( is_preferred: Some(false), data: None, }; - res.push(action); + acc.push(action); } for fix in snap.check_fixes.get(&file_id).into_iter().flatten() { @@ -910,53 +952,11 @@ fn handle_fixes( if fix_range.intersect(range).is_none() { continue; } - res.push(fix.action.clone()); + acc.push(fix.action.clone()); } Ok(()) } -pub(crate) fn handle_code_action( - mut snap: GlobalStateSnapshot, - params: lsp_types::CodeActionParams, -) -> Result>> { - let _p = profile::span("handle_code_action"); - // We intentionally don't support command-based actions, as those either - // requires custom client-code anyway, or requires server-initiated edits. - // Server initiated edits break causality, so we avoid those as well. - if !snap.config.client_caps.code_action_literals { - return Ok(None); - } - - let file_id = from_proto::file_id(&snap, ¶ms.text_document.uri)?; - let line_index = snap.analysis.file_line_index(file_id)?; - let range = from_proto::text_range(&line_index, params.range); - let frange = FileRange { file_id, range }; - - snap.config.assist.allowed = params - .clone() - .context - .only - .map(|it| it.into_iter().filter_map(from_proto::assist_kind).collect()); - - let mut res: Vec = Vec::new(); - - handle_fixes(&snap, ¶ms, &mut res)?; - - 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, params.clone(), assist, index)?); - } - } else { - for assist in snap.analysis.resolved_assists(&snap.config.assist, frange)?.into_iter() { - res.push(to_proto::resolved_code_action(&snap, assist)?); - } - } - - Ok(Some(res)) -} - pub(crate) fn handle_code_action_resolve( mut snap: GlobalStateSnapshot, mut code_action: lsp_ext::CodeAction, From 3ced546033973a63cb2ef1644ca099740fdfb1c2 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 23 Dec 2020 13:16:24 +0300 Subject: [PATCH 2/3] Make code more understandable Avoid mutation of snapshot's config -- that's spooky action at a distance. Instead, copy it over to a local variable. This points out a minor architecture problem, which we won't fix right away. Various `ide`-level config structs, like `AssistConfig`, are geared towards one-shot use when calling a specific methods. On the other hand, the large `Config` struct in `rust-analyzer` is a long-term config store. The fact that `Config` stores `AssistConfig` is accidental -- a better design would probably be to just store `ConfigData` inside `Config` and create various `Config`s on the fly out of it. --- crates/rust-analyzer/src/handlers.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index a51a9293fd1..83b3a343cd2 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -8,8 +8,9 @@ use std::{ }; use ide::{ - CompletionResolveCapability, FileId, FilePosition, FileRange, HoverAction, HoverGotoTypeData, - NavigationTarget, Query, RangeInfo, Runnable, RunnableKind, SearchScope, SymbolKind, TextEdit, + AssistConfig, CompletionResolveCapability, FileId, FilePosition, FileRange, HoverAction, + HoverGotoTypeData, NavigationTarget, Query, RangeInfo, Runnable, RunnableKind, SearchScope, + SymbolKind, TextEdit, }; use itertools::Itertools; use lsp_server::ErrorCode; @@ -882,11 +883,14 @@ pub(crate) fn handle_code_action( let range = from_proto::text_range(&line_index, params.range); let frange = FileRange { file_id, range }; - snap.config.assist.allowed = params - .clone() - .context - .only - .map(|it| it.into_iter().filter_map(from_proto::assist_kind).collect()); + let assists_config = AssistConfig { + allowed: params + .clone() + .context + .only + .map(|it| it.into_iter().filter_map(from_proto::assist_kind).collect()), + ..snap.config.assist + }; let mut res: Vec = Vec::new(); @@ -894,12 +898,12 @@ pub(crate) fn handle_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() + snap.analysis.unresolved_assists(&assists_config, frange)?.into_iter().enumerate() { 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() { + for assist in snap.analysis.resolved_assists(&assists_config, frange)?.into_iter() { res.push(to_proto::resolved_code_action(&snap, assist)?); } } From 2ec92b3dc3e3c51641e288fcba7ba13e7372cdd6 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 23 Dec 2020 13:36:57 +0300 Subject: [PATCH 3/3] Make code more direct * Push control flow outwards, as per https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/style.md#preconditions * Don't re-do the work, pass-in the arguments --- crates/rust-analyzer/src/handlers.rs | 47 ++++++++++++---------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index 83b3a343cd2..55bc2bceca1 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -5,12 +5,13 @@ use std::{ io::Write as _, process::{self, Stdio}, + sync::Arc, }; use ide::{ AssistConfig, CompletionResolveCapability, FileId, FilePosition, FileRange, HoverAction, - HoverGotoTypeData, NavigationTarget, Query, RangeInfo, Runnable, RunnableKind, SearchScope, - SymbolKind, TextEdit, + HoverGotoTypeData, LineIndex, NavigationTarget, Query, RangeInfo, Runnable, RunnableKind, + SearchScope, SymbolKind, TextEdit, }; use itertools::Itertools; use lsp_server::ErrorCode; @@ -867,7 +868,7 @@ pub(crate) fn handle_formatting( } pub(crate) fn handle_code_action( - mut snap: GlobalStateSnapshot, + snap: GlobalStateSnapshot, params: lsp_types::CodeActionParams, ) -> Result>> { let _p = profile::span("handle_code_action"); @@ -894,7 +895,15 @@ pub(crate) fn handle_code_action( let mut res: Vec = Vec::new(); - add_quick_fixes(&snap, ¶ms, &mut res)?; + let include_quick_fixes = match ¶ms.context.only { + Some(v) => v.iter().any(|it| { + it == &lsp_types::CodeActionKind::EMPTY || it == &lsp_types::CodeActionKind::QUICKFIX + }), + None => true, + }; + if include_quick_fixes { + add_quick_fixes(&snap, frange, &line_index, &mut res)?; + } if snap.config.client_caps.code_action_resolve { for (index, assist) in @@ -913,31 +922,16 @@ pub(crate) fn handle_code_action( fn add_quick_fixes( snap: &GlobalStateSnapshot, - params: &lsp_types::CodeActionParams, + frange: FileRange, + line_index: &Arc, acc: &mut Vec, ) -> Result<()> { - let file_id = from_proto::file_id(&snap, ¶ms.text_document.uri)?; - let line_index = snap.analysis.file_line_index(file_id)?; - let range = from_proto::text_range(&line_index, params.range); - - match ¶ms.context.only { - Some(v) => { - if !v.iter().any(|it| { - it == &lsp_types::CodeActionKind::EMPTY - || it == &lsp_types::CodeActionKind::QUICKFIX - }) { - return Ok(()); - } - } - None => {} - }; - - let diagnostics = snap.analysis.diagnostics(&snap.config.diagnostics, file_id)?; + let diagnostics = snap.analysis.diagnostics(&snap.config.diagnostics, frange.file_id)?; for fix in diagnostics .into_iter() .filter_map(|d| d.fix) - .filter(|fix| fix.fix_trigger_range.intersect(range).is_some()) + .filter(|fix| fix.fix_trigger_range.intersect(frange.range).is_some()) { let edit = to_proto::snippet_workspace_edit(&snap, fix.source_change)?; let action = lsp_ext::CodeAction { @@ -951,12 +945,11 @@ fn add_quick_fixes( acc.push(action); } - for fix in snap.check_fixes.get(&file_id).into_iter().flatten() { + for fix in snap.check_fixes.get(&frange.file_id).into_iter().flatten() { let fix_range = from_proto::text_range(&line_index, fix.range); - if fix_range.intersect(range).is_none() { - continue; + if fix_range.intersect(frange.range).is_some() { + acc.push(fix.action.clone()); } - acc.push(fix.action.clone()); } Ok(()) }