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 ++++++++++++++++----------------- 3 files changed, 49 insertions(+), 91 deletions(-) (limited to 'crates/assists') 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"); } } -- cgit v1.2.3