From 2f2267553769bd7c63ab49c85e4c67a7339355c3 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 26 Dec 2020 14:11:42 +0300 Subject: Simplify assists resolution API Assist vs UnresolvedAssist split doesn't really pull its weight. This is especially bad if we want to include `Assist` as a field of diagnostics, where we'd have to make the thing generic. --- crates/assists/src/assist_context.rs | 51 +++++++++--------------------------- crates/assists/src/lib.rs | 41 ++++++++++------------------- crates/assists/src/tests.rs | 48 ++++++++++++++++----------------- crates/ide/src/lib.rs | 22 ++++++---------- crates/rust-analyzer/src/handlers.rs | 8 +++--- crates/rust-analyzer/src/to_proto.rs | 19 ++++++-------- 6 files changed, 69 insertions(+), 120 deletions(-) diff --git a/crates/assists/src/assist_context.rs b/crates/assists/src/assist_context.rs index 80cf9aba1..4f59d39a9 100644 --- a/crates/assists/src/assist_context.rs +++ b/crates/assists/src/assist_context.rs @@ -19,7 +19,7 @@ use text_edit::{TextEdit, TextEditBuilder}; use crate::{ assist_config::{AssistConfig, SnippetCap}, - Assist, AssistId, AssistKind, GroupLabel, ResolvedAssist, + Assist, AssistId, AssistKind, GroupLabel, }; /// `AssistContext` allows to apply an assist or check if it could be applied. @@ -105,46 +105,23 @@ impl<'a> AssistContext<'a> { pub(crate) struct Assists { resolve: bool, file: FileId, - buf: Vec<(Assist, Option)>, + buf: Vec, allowed: Option>, } impl Assists { - pub(crate) fn new_resolved(ctx: &AssistContext) -> Assists { + pub(crate) fn new(ctx: &AssistContext, resolve: bool) -> Assists { Assists { - resolve: true, + resolve, file: ctx.frange.file_id, buf: Vec::new(), allowed: ctx.config.allowed.clone(), } } - pub(crate) fn new_unresolved(ctx: &AssistContext) -> Assists { - Assists { - resolve: false, - file: ctx.frange.file_id, - buf: Vec::new(), - allowed: ctx.config.allowed.clone(), - } - } - - pub(crate) fn finish_unresolved(self) -> Vec { - assert!(!self.resolve); - self.finish() - .into_iter() - .map(|(label, edit)| { - assert!(edit.is_none()); - label - }) - .collect() - } - - pub(crate) fn finish_resolved(self) -> Vec { - assert!(self.resolve); - self.finish() - .into_iter() - .map(|(label, edit)| ResolvedAssist { assist: label, source_change: edit.unwrap() }) - .collect() + pub(crate) fn finish(mut self) -> Vec { + self.buf.sort_by_key(|assist| assist.target.len()); + self.buf } pub(crate) fn add( @@ -158,7 +135,7 @@ impl Assists { return None; } let label = Label::new(label.into()); - let assist = Assist { id, label, group: None, target }; + let assist = Assist { id, label, group: None, target, source_change: None }; self.add_impl(assist, f) } @@ -174,11 +151,11 @@ impl Assists { return None; } let label = Label::new(label.into()); - let assist = Assist { id, label, group: Some(group.clone()), target }; + let assist = Assist { id, label, group: Some(group.clone()), target, source_change: None }; self.add_impl(assist, f) } - fn add_impl(&mut self, assist: Assist, f: impl FnOnce(&mut AssistBuilder)) -> Option<()> { + fn add_impl(&mut self, mut assist: Assist, f: impl FnOnce(&mut AssistBuilder)) -> Option<()> { let source_change = if self.resolve { let mut builder = AssistBuilder::new(self.file); f(&mut builder); @@ -186,16 +163,12 @@ impl Assists { } else { None }; + assist.source_change = source_change.clone(); - self.buf.push((assist, source_change)); + self.buf.push(assist); Some(()) } - fn finish(mut self) -> Vec<(Assist, Option)> { - self.buf.sort_by_key(|(label, _edit)| label.target.len()); - self.buf - } - fn is_allowed(&self, id: &AssistId) -> bool { match &self.allowed { Some(allowed) => allowed.iter().any(|kind| kind.contains(id.1)), diff --git a/crates/assists/src/lib.rs b/crates/assists/src/lib.rs index 6b89b2d04..fdec886e9 100644 --- a/crates/assists/src/lib.rs +++ b/crates/assists/src/lib.rs @@ -73,45 +73,32 @@ pub struct Assist { /// Target ranges are used to sort assists: the smaller the target range, /// the more specific assist is, and so it should be sorted first. pub target: TextRange, -} - -#[derive(Debug, Clone)] -pub struct ResolvedAssist { - pub assist: Assist, - pub source_change: SourceChange, + /// Computing source change sometimes is much more costly then computing the + /// other fields. Additionally, the actual change is not required to show + /// the lightbulb UI, it only is needed when the user tries to apply an + /// assist. So, we compute it lazily: the API allow requesting assists with + /// or without source change. We could (and in fact, used to) distinguish + /// between resolved and unresolved assists at the type level, but this is + /// cumbersome, especially if you want to embed an assist into another data + /// structure, such as a diagnostic. + pub source_change: Option, } impl Assist { /// Return all the assists applicable at the given position. - /// - /// Assists are returned in the "unresolved" state, that is only labels are - /// returned, without actual edits. - pub fn unresolved(db: &RootDatabase, config: &AssistConfig, range: FileRange) -> Vec { - let sema = Semantics::new(db); - let ctx = AssistContext::new(sema, config, range); - let mut acc = Assists::new_unresolved(&ctx); - handlers::all().iter().for_each(|handler| { - handler(&mut acc, &ctx); - }); - acc.finish_unresolved() - } - - /// Return all the assists applicable at the given position. - /// - /// Assists are returned in the "resolved" state, that is with edit fully - /// computed. - pub fn resolved( + pub fn get( db: &RootDatabase, config: &AssistConfig, + resolve: bool, range: FileRange, - ) -> Vec { + ) -> Vec { let sema = Semantics::new(db); let ctx = AssistContext::new(sema, config, range); - let mut acc = Assists::new_resolved(&ctx); + let mut acc = Assists::new(&ctx, resolve); handlers::all().iter().for_each(|handler| { handler(&mut acc, &ctx); }); - acc.finish_resolved() + acc.finish() } } diff --git a/crates/assists/src/tests.rs b/crates/assists/src/tests.rs index b41f4874a..21e448fb8 100644 --- a/crates/assists/src/tests.rs +++ b/crates/assists/src/tests.rs @@ -48,24 +48,25 @@ fn check_doc_test(assist_id: &str, before: &str, after: &str) { let before = db.file_text(file_id).to_string(); let frange = FileRange { file_id, range: selection.into() }; - let assist = Assist::resolved(&db, &AssistConfig::default(), frange) + let assist = Assist::get(&db, &AssistConfig::default(), true, frange) .into_iter() - .find(|assist| assist.assist.id.0 == assist_id) + .find(|assist| assist.id.0 == assist_id) .unwrap_or_else(|| { panic!( "\n\nAssist is not applicable: {}\nAvailable assists: {}", assist_id, - Assist::resolved(&db, &AssistConfig::default(), frange) + Assist::get(&db, &AssistConfig::default(), false, frange) .into_iter() - .map(|assist| assist.assist.id.0) + .map(|assist| assist.id.0) .collect::>() .join(", ") ) }); let actual = { + let source_change = assist.source_change.unwrap(); let mut actual = before; - for source_file_edit in assist.source_change.source_file_edits { + for source_file_edit in source_change.source_file_edits { if source_file_edit.file_id == file_id { source_file_edit.edit.apply(&mut actual) } @@ -90,18 +91,18 @@ fn check(handler: Handler, before: &str, expected: ExpectedResult, assist_label: let sema = Semantics::new(&db); let config = AssistConfig::default(); let ctx = AssistContext::new(sema, &config, frange); - let mut acc = Assists::new_resolved(&ctx); + let mut acc = Assists::new(&ctx, true); handler(&mut acc, &ctx); - let mut res = acc.finish_resolved(); + let mut res = acc.finish(); let assist = match assist_label { - Some(label) => res.into_iter().find(|resolved| resolved.assist.label == label), + Some(label) => res.into_iter().find(|resolved| resolved.label == label), None => res.pop(), }; match (assist, expected) { (Some(assist), ExpectedResult::After(after)) => { - let mut source_change = assist.source_change; + let mut source_change = assist.source_change.unwrap(); assert!(!source_change.source_file_edits.is_empty()); let skip_header = source_change.source_file_edits.len() == 1 && source_change.file_system_edits.len() == 0; @@ -138,7 +139,7 @@ fn check(handler: Handler, before: &str, expected: ExpectedResult, assist_label: assert_eq_text!(after, &buf); } (Some(assist), ExpectedResult::Target(target)) => { - let range = assist.assist.target; + let range = assist.target; assert_eq_text!(&text_without_caret[range], target); } (Some(_), ExpectedResult::NotApplicable) => panic!("assist should not be applicable!"), @@ -155,14 +156,11 @@ fn assist_order_field_struct() { let (before_cursor_pos, before) = extract_offset(before); let (db, file_id) = with_single_file(&before); let frange = FileRange { file_id, range: TextRange::empty(before_cursor_pos) }; - let assists = Assist::resolved(&db, &AssistConfig::default(), frange); + let assists = Assist::get(&db, &AssistConfig::default(), false, frange); let mut assists = assists.iter(); - assert_eq!( - assists.next().expect("expected assist").assist.label, - "Change visibility to pub(crate)" - ); - assert_eq!(assists.next().expect("expected assist").assist.label, "Add `#[derive]`"); + assert_eq!(assists.next().expect("expected assist").label, "Change visibility to pub(crate)"); + assert_eq!(assists.next().expect("expected assist").label, "Add `#[derive]`"); } #[test] @@ -178,11 +176,11 @@ fn assist_order_if_expr() { let (range, before) = extract_range(before); let (db, file_id) = with_single_file(&before); let frange = FileRange { file_id, range }; - let assists = Assist::resolved(&db, &AssistConfig::default(), frange); + let assists = Assist::get(&db, &AssistConfig::default(), false, frange); let mut assists = assists.iter(); - assert_eq!(assists.next().expect("expected assist").assist.label, "Extract into variable"); - assert_eq!(assists.next().expect("expected assist").assist.label, "Replace with match"); + assert_eq!(assists.next().expect("expected assist").label, "Extract into variable"); + assert_eq!(assists.next().expect("expected assist").label, "Replace with match"); } #[test] @@ -203,27 +201,27 @@ fn assist_filter_works() { let mut cfg = AssistConfig::default(); cfg.allowed = Some(vec![AssistKind::Refactor]); - let assists = Assist::resolved(&db, &cfg, frange); + let assists = Assist::get(&db, &cfg, false, frange); let mut assists = assists.iter(); - assert_eq!(assists.next().expect("expected assist").assist.label, "Extract into variable"); - assert_eq!(assists.next().expect("expected assist").assist.label, "Replace with match"); + assert_eq!(assists.next().expect("expected assist").label, "Extract into variable"); + assert_eq!(assists.next().expect("expected assist").label, "Replace with match"); } { let mut cfg = AssistConfig::default(); cfg.allowed = Some(vec![AssistKind::RefactorExtract]); - let assists = Assist::resolved(&db, &cfg, frange); + let assists = Assist::get(&db, &cfg, false, frange); assert_eq!(assists.len(), 1); let mut assists = assists.iter(); - assert_eq!(assists.next().expect("expected assist").assist.label, "Extract into variable"); + assert_eq!(assists.next().expect("expected assist").label, "Extract into variable"); } { let mut cfg = AssistConfig::default(); cfg.allowed = Some(vec![AssistKind::QuickFix]); - let assists = Assist::resolved(&db, &cfg, frange); + let assists = Assist::get(&db, &cfg, false, frange); assert!(assists.is_empty(), "All asserts but quickfixes should be filtered out"); } } diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs index a75cc85b6..41eb139d1 100644 --- a/crates/ide/src/lib.rs +++ b/crates/ide/src/lib.rs @@ -79,7 +79,7 @@ pub use crate::{ HighlightedRange, }, }; -pub use assists::{Assist, AssistConfig, AssistId, AssistKind, ResolvedAssist}; +pub use assists::{Assist, AssistConfig, AssistId, AssistKind}; pub use completion::{ CompletionConfig, CompletionItem, CompletionItemKind, CompletionResolveCapability, CompletionScore, ImportEdit, InsertTextFormat, @@ -491,22 +491,16 @@ impl Analysis { } /// Computes assists (aka code actions aka intentions) for the given - /// position. Computes enough info to show the lightbulb list in the editor, - /// but doesn't compute actual edits, to improve performance. - /// - /// When the user clicks on the assist, call `resolve_assists` to get the - /// edit. - pub fn assists(&self, config: &AssistConfig, frange: FileRange) -> Cancelable> { - self.with_db(|db| Assist::unresolved(db, config, frange)) - } - - /// Computes resolved assists with source changes for the given position. - pub fn resolve_assists( + /// position. If `resolve == false`, computes enough info to show the + /// lightbulb list in the editor, but doesn't compute actual edits, to + /// improve performance. + pub fn assists( &self, config: &AssistConfig, + resolve: bool, frange: FileRange, - ) -> Cancelable> { - self.with_db(|db| assists::Assist::resolved(db, config, frange)) + ) -> Cancelable> { + self.with_db(|db| Assist::get(db, config, resolve, frange)) } /// Computes the set of diagnostics for the given file. diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index 1207b31c4..374fb5302 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -946,12 +946,12 @@ pub(crate) fn handle_code_action( if snap.config.client_caps.code_action_resolve { for (index, assist) in - snap.analysis.assists(&assists_config, frange)?.into_iter().enumerate() + snap.analysis.assists(&assists_config, false, frange)?.into_iter().enumerate() { res.push(to_proto::unresolved_code_action(&snap, params.clone(), assist, index)?); } } else { - for assist in snap.analysis.resolve_assists(&assists_config, frange)?.into_iter() { + for assist in snap.analysis.assists(&assists_config, true, frange)?.into_iter() { res.push(to_proto::resolved_code_action(&snap, assist)?); } } @@ -1014,11 +1014,11 @@ pub(crate) fn handle_code_action_resolve( .only .map(|it| it.into_iter().filter_map(from_proto::assist_kind).collect()); - let assists = snap.analysis.resolve_assists(&snap.config.assist, frange)?; + let assists = snap.analysis.assists(&snap.config.assist, true, frange)?; let (id, index) = split_once(¶ms.id, ':').unwrap(); let index = index.parse::().unwrap(); let assist = &assists[index]; - assert!(assist.assist.id.0 == id); + assert!(assist.id.0 == id); let edit = to_proto::resolved_code_action(&snap, assist.clone())?.edit; code_action.edit = edit; Ok(code_action) diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs index 753aad628..1a38e79f0 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -8,8 +8,8 @@ use ide::{ Assist, AssistKind, CallInfo, CompletionItem, CompletionItemKind, Documentation, FileId, FileRange, FileSystemEdit, Fold, FoldKind, Highlight, HighlightModifier, HighlightTag, HighlightedRange, Indel, InlayHint, InlayKind, InsertTextFormat, LineIndex, Markup, - NavigationTarget, ReferenceAccess, ResolvedAssist, Runnable, Severity, SourceChange, - SourceFileEdit, SymbolKind, TextEdit, TextRange, TextSize, + NavigationTarget, ReferenceAccess, Runnable, Severity, SourceChange, SourceFileEdit, + SymbolKind, TextEdit, TextRange, TextSize, }; use itertools::Itertools; @@ -780,6 +780,7 @@ pub(crate) fn unresolved_code_action( assist: Assist, index: usize, ) -> Result { + assert!(assist.source_change.is_none()); let res = lsp_ext::CodeAction { title: assist.label.to_string(), group: assist.group.filter(|_| snap.config.client_caps.code_action_group).map(|gr| gr.0), @@ -796,18 +797,14 @@ pub(crate) fn unresolved_code_action( pub(crate) fn resolved_code_action( snap: &GlobalStateSnapshot, - assist: ResolvedAssist, + assist: Assist, ) -> Result { - let change = assist.source_change; + let change = assist.source_change.unwrap(); 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)), + title: assist.label.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)), is_preferred: None, data: None, }; -- cgit v1.2.3