From e5cdcb8b124f5b7d59950429787e760e46388f72 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 3 May 2021 17:08:09 +0300 Subject: [PATCH 1/9] Add a way to resolve certain assists --- crates/ide/src/diagnostics.rs | 43 +++++++++++----- crates/ide/src/diagnostics/fixes.rs | 51 +++++++++++++++---- crates/ide/src/diagnostics/unlinked_file.rs | 7 ++- crates/ide/src/lib.rs | 35 +++++++------ crates/ide/src/ssr.rs | 55 +++++++++------------ crates/ide_assists/src/assist_context.rs | 10 ++-- crates/ide_assists/src/lib.rs | 24 +++++++-- crates/ide_assists/src/tests.rs | 21 ++++---- crates/rust-analyzer/src/cli/diagnostics.rs | 7 +-- crates/rust-analyzer/src/handlers.rs | 17 +++++-- 10 files changed, 178 insertions(+), 92 deletions(-) diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index 1c911a8b2c8..455f20c93f2 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs @@ -15,6 +15,7 @@ diagnostics::{Diagnostic as _, DiagnosticCode, DiagnosticSinkBuilder}, InFile, Semantics, }; +use ide_assists::AssistResolveStrategy; use ide_db::{base_db::SourceDatabase, RootDatabase}; use itertools::Itertools; use rustc_hash::FxHashSet; @@ -84,7 +85,7 @@ pub struct DiagnosticsConfig { pub(crate) fn diagnostics( db: &RootDatabase, config: &DiagnosticsConfig, - resolve: bool, + resolve: AssistResolveStrategy, file_id: FileId, ) -> Vec { let _p = profile::span("diagnostics"); @@ -212,7 +213,7 @@ pub(crate) fn diagnostics( fn diagnostic_with_fix( d: &D, sema: &Semantics, - resolve: bool, + resolve: AssistResolveStrategy, ) -> Diagnostic { Diagnostic::error(sema.diagnostics_display_range(d.display_source()).range, d.message()) .with_fix(d.fix(&sema, resolve)) @@ -222,7 +223,7 @@ fn diagnostic_with_fix( fn warning_with_fix( d: &D, sema: &Semantics, - resolve: bool, + resolve: AssistResolveStrategy, ) -> Diagnostic { Diagnostic::hint(sema.diagnostics_display_range(d.display_source()).range, d.message()) .with_fix(d.fix(&sema, resolve)) @@ -299,6 +300,7 @@ fn unresolved_fix(id: &'static str, label: &str, target: TextRange) -> Assist { #[cfg(test)] mod tests { use expect_test::{expect, Expect}; + use ide_assists::AssistResolveStrategy; use stdx::trim_indent; use test_utils::assert_eq_text; @@ -314,7 +316,11 @@ pub(crate) fn check_fix(ra_fixture_before: &str, ra_fixture_after: &str) { let (analysis, file_position) = fixture::position(ra_fixture_before); let diagnostic = analysis - .diagnostics(&DiagnosticsConfig::default(), true, file_position.file_id) + .diagnostics( + &DiagnosticsConfig::default(), + AssistResolveStrategy::All, + file_position.file_id, + ) .unwrap() .pop() .unwrap(); @@ -343,7 +349,11 @@ pub(crate) fn check_fix(ra_fixture_before: &str, ra_fixture_after: &str) { fn check_no_fix(ra_fixture: &str) { let (analysis, file_position) = fixture::position(ra_fixture); let diagnostic = analysis - .diagnostics(&DiagnosticsConfig::default(), true, file_position.file_id) + .diagnostics( + &DiagnosticsConfig::default(), + AssistResolveStrategy::All, + file_position.file_id, + ) .unwrap() .pop() .unwrap(); @@ -357,7 +367,9 @@ pub(crate) fn check_no_diagnostics(ra_fixture: &str) { let diagnostics = files .into_iter() .flat_map(|file_id| { - analysis.diagnostics(&DiagnosticsConfig::default(), true, file_id).unwrap() + analysis + .diagnostics(&DiagnosticsConfig::default(), AssistResolveStrategy::All, file_id) + .unwrap() }) .collect::>(); assert_eq!(diagnostics.len(), 0, "unexpected diagnostics:\n{:#?}", diagnostics); @@ -365,8 +377,9 @@ pub(crate) fn check_no_diagnostics(ra_fixture: &str) { fn check_expect(ra_fixture: &str, expect: Expect) { let (analysis, file_id) = fixture::file(ra_fixture); - let diagnostics = - analysis.diagnostics(&DiagnosticsConfig::default(), true, file_id).unwrap(); + let diagnostics = analysis + .diagnostics(&DiagnosticsConfig::default(), AssistResolveStrategy::All, file_id) + .unwrap(); expect.assert_debug_eq(&diagnostics) } @@ -911,11 +924,13 @@ fn test_disabled_diagnostics() { let (analysis, file_id) = fixture::file(r#"mod foo;"#); - let diagnostics = analysis.diagnostics(&config, true, file_id).unwrap(); + let diagnostics = + analysis.diagnostics(&config, AssistResolveStrategy::All, file_id).unwrap(); assert!(diagnostics.is_empty()); - let diagnostics = - analysis.diagnostics(&DiagnosticsConfig::default(), true, file_id).unwrap(); + let diagnostics = analysis + .diagnostics(&DiagnosticsConfig::default(), AssistResolveStrategy::All, file_id) + .unwrap(); assert!(!diagnostics.is_empty()); } @@ -1022,7 +1037,11 @@ fn test_single_incorrect_case_diagnostic_in_function_name_issue_6970() { let (analysis, file_position) = fixture::position(input); let diagnostics = analysis - .diagnostics(&DiagnosticsConfig::default(), true, file_position.file_id) + .diagnostics( + &DiagnosticsConfig::default(), + AssistResolveStrategy::All, + file_position.file_id, + ) .unwrap(); assert_eq!(diagnostics.len(), 1); diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs index 7be8b345930..f23064eac10 100644 --- a/crates/ide/src/diagnostics/fixes.rs +++ b/crates/ide/src/diagnostics/fixes.rs @@ -8,6 +8,7 @@ }, HasSource, HirDisplay, InFile, Semantics, VariantDef, }; +use ide_assists::AssistResolveStrategy; use ide_db::{ base_db::{AnchoredPathBuf, FileId}, source_change::{FileSystemEdit, SourceChange}, @@ -35,11 +36,19 @@ pub(crate) trait DiagnosticWithFix: Diagnostic { /// /// If `resolve` is false, the edit will be computed later, on demand, and /// can be omitted. - fn fix(&self, sema: &Semantics, _resolve: bool) -> Option; + fn fix( + &self, + sema: &Semantics, + _resolve: AssistResolveStrategy, + ) -> Option; } impl DiagnosticWithFix for UnresolvedModule { - fn fix(&self, sema: &Semantics, _resolve: bool) -> Option { + fn fix( + &self, + sema: &Semantics, + _resolve: AssistResolveStrategy, + ) -> Option { let root = sema.db.parse_or_expand(self.file)?; let unresolved_module = self.decl.to_node(&root); Some(fix( @@ -59,7 +68,11 @@ fn fix(&self, sema: &Semantics, _resolve: bool) -> Option } impl DiagnosticWithFix for NoSuchField { - fn fix(&self, sema: &Semantics, _resolve: bool) -> Option { + fn fix( + &self, + sema: &Semantics, + _resolve: AssistResolveStrategy, + ) -> Option { let root = sema.db.parse_or_expand(self.file)?; missing_record_expr_field_fix( &sema, @@ -70,7 +83,11 @@ fn fix(&self, sema: &Semantics, _resolve: bool) -> Option } impl DiagnosticWithFix for MissingFields { - fn fix(&self, sema: &Semantics, _resolve: bool) -> Option { + fn fix( + &self, + sema: &Semantics, + _resolve: AssistResolveStrategy, + ) -> Option { // Note that although we could add a diagnostics to // fill the missing tuple field, e.g : // `struct A(usize);` @@ -106,7 +123,11 @@ fn fix(&self, sema: &Semantics, _resolve: bool) -> Option } impl DiagnosticWithFix for MissingOkOrSomeInTailExpr { - fn fix(&self, sema: &Semantics, _resolve: bool) -> Option { + fn fix( + &self, + sema: &Semantics, + _resolve: AssistResolveStrategy, + ) -> Option { let root = sema.db.parse_or_expand(self.file)?; let tail_expr = self.expr.to_node(&root); let tail_expr_range = tail_expr.syntax().text_range(); @@ -119,7 +140,11 @@ fn fix(&self, sema: &Semantics, _resolve: bool) -> Option } impl DiagnosticWithFix for RemoveThisSemicolon { - fn fix(&self, sema: &Semantics, _resolve: bool) -> Option { + fn fix( + &self, + sema: &Semantics, + _resolve: AssistResolveStrategy, + ) -> Option { let root = sema.db.parse_or_expand(self.file)?; let semicolon = self @@ -139,7 +164,11 @@ fn fix(&self, sema: &Semantics, _resolve: bool) -> Option } impl DiagnosticWithFix for IncorrectCase { - fn fix(&self, sema: &Semantics, resolve: bool) -> Option { + fn fix( + &self, + sema: &Semantics, + resolve: AssistResolveStrategy, + ) -> Option { let root = sema.db.parse_or_expand(self.file)?; let name_node = self.ident.to_node(&root); @@ -149,7 +178,7 @@ fn fix(&self, sema: &Semantics, resolve: bool) -> Option { let label = format!("Rename to {}", self.suggested_text); let mut res = unresolved_fix("change_case", &label, frange.range); - if resolve { + if resolve.should_resolve(&res.id) { let source_change = rename_with_semantics(sema, file_position, &self.suggested_text); res.source_change = Some(source_change.ok().unwrap_or_default()); } @@ -159,7 +188,11 @@ fn fix(&self, sema: &Semantics, resolve: bool) -> Option { } impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap { - fn fix(&self, sema: &Semantics, _resolve: bool) -> Option { + fn fix( + &self, + sema: &Semantics, + _resolve: AssistResolveStrategy, + ) -> Option { let root = sema.db.parse_or_expand(self.file)?; let next_expr = self.next_expr.to_node(&root); let next_call = ast::MethodCallExpr::cast(next_expr.syntax().clone())?; diff --git a/crates/ide/src/diagnostics/unlinked_file.rs b/crates/ide/src/diagnostics/unlinked_file.rs index 7d39f4fbe40..e48528bed25 100644 --- a/crates/ide/src/diagnostics/unlinked_file.rs +++ b/crates/ide/src/diagnostics/unlinked_file.rs @@ -5,6 +5,7 @@ diagnostics::{Diagnostic, DiagnosticCode}, InFile, }; +use ide_assists::AssistResolveStrategy; use ide_db::{ base_db::{FileId, FileLoader, SourceDatabase, SourceDatabaseExt}, source_change::SourceChange, @@ -50,7 +51,11 @@ fn as_any(&self) -> &(dyn std::any::Any + Send + 'static) { } impl DiagnosticWithFix for UnlinkedFile { - fn fix(&self, sema: &hir::Semantics, _resolve: bool) -> Option { + fn fix( + &self, + sema: &hir::Semantics, + _resolve: AssistResolveStrategy, + ) -> Option { // If there's an existing module that could add a `mod` item to include the unlinked file, // suggest that as a fix. diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs index 99e45633e4a..6847b7e836a 100644 --- a/crates/ide/src/lib.rs +++ b/crates/ide/src/lib.rs @@ -87,7 +87,7 @@ macro_rules! eprintln { }, }; pub use hir::{Documentation, Semantics}; -pub use ide_assists::{Assist, AssistConfig, AssistId, AssistKind}; +pub use ide_assists::{Assist, AssistConfig, AssistId, AssistKind, AssistResolveStrategy}; pub use ide_completion::{ CompletionConfig, CompletionItem, CompletionItemKind, CompletionRelevance, ImportEdit, InsertTextFormat, @@ -518,12 +518,13 @@ pub fn resolve_completion_edits( pub fn assists( &self, config: &AssistConfig, - resolve: bool, + resolve: AssistResolveStrategy, frange: FileRange, ) -> Cancelable> { self.with_db(|db| { + let ssr_assists = ssr::ssr_assists(db, resolve, frange); let mut acc = Assist::get(db, config, resolve, frange); - ssr::add_ssr_assist(db, &mut acc, resolve, frange); + acc.extend(ssr_assists.into_iter()); acc }) } @@ -532,7 +533,7 @@ pub fn assists( pub fn diagnostics( &self, config: &DiagnosticsConfig, - resolve: bool, + resolve: AssistResolveStrategy, file_id: FileId, ) -> Cancelable> { self.with_db(|db| diagnostics::diagnostics(db, config, resolve, file_id)) @@ -543,7 +544,7 @@ pub fn assists_with_fixes( &self, assist_config: &AssistConfig, diagnostics_config: &DiagnosticsConfig, - resolve: bool, + resolve: AssistResolveStrategy, frange: FileRange, ) -> Cancelable> { let include_fixes = match &assist_config.allowed { @@ -552,17 +553,21 @@ pub fn assists_with_fixes( }; self.with_db(|db| { - let mut res = Assist::get(db, assist_config, resolve, frange); - ssr::add_ssr_assist(db, &mut res, resolve, frange); + let ssr_assists = ssr::ssr_assists(db, resolve, frange); + let diagnostic_assists = if include_fixes { + diagnostics::diagnostics(db, diagnostics_config, resolve, frange.file_id) + .into_iter() + .filter_map(|it| it.fix) + .filter(|it| it.target.intersect(frange.range).is_some()) + .collect() + } else { + Vec::new() + }; + + let mut res = Assist::get(db, assist_config, resolve, frange); + res.extend(ssr_assists.into_iter()); + res.extend(diagnostic_assists.into_iter()); - if include_fixes { - res.extend( - diagnostics::diagnostics(db, diagnostics_config, resolve, frange.file_id) - .into_iter() - .filter_map(|it| it.fix) - .filter(|it| it.target.intersect(frange.range).is_some()), - ); - } res }) } diff --git a/crates/ide/src/ssr.rs b/crates/ide/src/ssr.rs index f3638d928c0..785ce301099 100644 --- a/crates/ide/src/ssr.rs +++ b/crates/ide/src/ssr.rs @@ -2,18 +2,23 @@ //! assist in ide_assists because that would require the ide_assists crate //! depend on the ide_ssr crate. -use ide_assists::{Assist, AssistId, AssistKind, GroupLabel}; +use ide_assists::{Assist, AssistId, AssistKind, AssistResolveStrategy, GroupLabel}; use ide_db::{base_db::FileRange, label::Label, source_change::SourceChange, RootDatabase}; -pub(crate) fn add_ssr_assist( +pub(crate) fn ssr_assists( db: &RootDatabase, - base: &mut Vec, - resolve: bool, + resolve: AssistResolveStrategy, frange: FileRange, -) -> Option<()> { - let (match_finder, comment_range) = ide_ssr::ssr_from_comment(db, frange)?; +) -> Vec { + let mut ssr_assists = Vec::with_capacity(2); - let (source_change_for_file, source_change_for_workspace) = if resolve { + let (match_finder, comment_range) = match ide_ssr::ssr_from_comment(db, frange) { + Some((match_finder, comment_range)) => (match_finder, comment_range), + None => return ssr_assists, + }; + let id = AssistId("ssr", AssistKind::RefactorRewrite); + + let (source_change_for_file, source_change_for_workspace) = if resolve.should_resolve(&id) { let edits = match_finder.edits(); let source_change_for_file = { @@ -35,16 +40,17 @@ pub(crate) fn add_ssr_assist( for (label, source_change) in assists.into_iter() { let assist = Assist { - id: AssistId("ssr", AssistKind::RefactorRewrite), + id, label: Label::new(label), group: Some(GroupLabel("Apply SSR".into())), target: comment_range, source_change, }; - base.push(assist); + ssr_assists.push(assist); } - Some(()) + + ssr_assists } #[cfg(test)] @@ -52,7 +58,7 @@ mod tests { use std::sync::Arc; use expect_test::expect; - use ide_assists::Assist; + use ide_assists::{Assist, AssistResolveStrategy}; use ide_db::{ base_db::{fixture::WithFixture, salsa::Durability, FileRange}, symbol_index::SymbolsDatabase, @@ -60,24 +66,14 @@ mod tests { }; use rustc_hash::FxHashSet; - use super::add_ssr_assist; + use super::ssr_assists; - fn get_assists(ra_fixture: &str, resolve: bool) -> Vec { + fn get_assists(ra_fixture: &str, resolve: AssistResolveStrategy) -> Vec { let (mut db, file_id, range_or_offset) = RootDatabase::with_range_or_offset(ra_fixture); let mut local_roots = FxHashSet::default(); local_roots.insert(ide_db::base_db::fixture::WORKSPACE); db.set_local_roots_with_durability(Arc::new(local_roots), Durability::HIGH); - - let mut assists = vec![]; - - add_ssr_assist( - &db, - &mut assists, - resolve, - FileRange { file_id, range: range_or_offset.into() }, - ); - - assists + ssr_assists(&db, resolve, FileRange { file_id, range: range_or_offset.into() }) } #[test] @@ -88,16 +84,14 @@ fn not_applicable_comment_not_ssr() { // This is foo $0 fn foo() {} "#; - let resolve = true; - - let assists = get_assists(ra_fixture, resolve); + let assists = get_assists(ra_fixture, AssistResolveStrategy::All); assert_eq!(0, assists.len()); } + // TODO kb add partial resolve test #[test] fn resolve_edits_true() { - let resolve = true; let assists = get_assists( r#" //- /lib.rs @@ -109,7 +103,7 @@ fn foo() { 2 } //- /bar.rs fn bar() { 2 } "#, - resolve, + AssistResolveStrategy::All, ); assert_eq!(2, assists.len()); @@ -200,7 +194,6 @@ fn bar() { 2 } #[test] fn resolve_edits_false() { - let resolve = false; let assists = get_assists( r#" //- /lib.rs @@ -212,7 +205,7 @@ fn foo() { 2 } //- /bar.rs fn bar() { 2 } "#, - resolve, + AssistResolveStrategy::None, ); assert_eq!(2, assists.len()); diff --git a/crates/ide_assists/src/assist_context.rs b/crates/ide_assists/src/assist_context.rs index 8714e4978c5..19e9f179ed5 100644 --- a/crates/ide_assists/src/assist_context.rs +++ b/crates/ide_assists/src/assist_context.rs @@ -19,7 +19,9 @@ }; use text_edit::{TextEdit, TextEditBuilder}; -use crate::{assist_config::AssistConfig, Assist, AssistId, AssistKind, GroupLabel}; +use crate::{ + assist_config::AssistConfig, Assist, AssistId, AssistKind, AssistResolveStrategy, GroupLabel, +}; /// `AssistContext` allows to apply an assist or check if it could be applied. /// @@ -105,14 +107,14 @@ pub(crate) fn covering_node_for_range(&self, range: TextRange) -> SyntaxElement } pub(crate) struct Assists { - resolve: bool, file: FileId, + resolve: AssistResolveStrategy, buf: Vec, allowed: Option>, } impl Assists { - pub(crate) fn new(ctx: &AssistContext, resolve: bool) -> Assists { + pub(crate) fn new(ctx: &AssistContext, resolve: AssistResolveStrategy) -> Assists { Assists { resolve, file: ctx.frange.file_id, @@ -158,7 +160,7 @@ pub(crate) fn add_group( } fn add_impl(&mut self, mut assist: Assist, f: impl FnOnce(&mut AssistBuilder)) -> Option<()> { - let source_change = if self.resolve { + let source_change = if self.resolve.should_resolve(&assist.id) { let mut builder = AssistBuilder::new(self.file); f(&mut builder); Some(builder.finish()) diff --git a/crates/ide_assists/src/lib.rs b/crates/ide_assists/src/lib.rs index 88ae5c9a935..397d2a3d0ee 100644 --- a/crates/ide_assists/src/lib.rs +++ b/crates/ide_assists/src/lib.rs @@ -26,7 +26,7 @@ macro_rules! eprintln { pub use assist_config::AssistConfig; -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub enum AssistKind { // FIXME: does the None variant make sense? Probably not. None, @@ -60,9 +60,27 @@ pub fn contains(self, other: AssistKind) -> bool { /// Unique identifier of the assist, should not be shown to the user /// directly. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct AssistId(pub &'static str, pub AssistKind); +// TODO kb docs +#[derive(Debug, Clone, Copy)] +pub enum AssistResolveStrategy { + None, + All, + Single(AssistId), +} + +impl AssistResolveStrategy { + pub fn should_resolve(&self, id: &AssistId) -> bool { + match self { + AssistResolveStrategy::None => false, + AssistResolveStrategy::All => true, + AssistResolveStrategy::Single(id_to_resolve) => id_to_resolve == id, + } + } +} + #[derive(Clone, Debug)] pub struct GroupLabel(pub String); @@ -91,7 +109,7 @@ impl Assist { pub fn get( db: &RootDatabase, config: &AssistConfig, - resolve: bool, + resolve: AssistResolveStrategy, range: FileRange, ) -> Vec { let sema = Semantics::new(db); diff --git a/crates/ide_assists/src/tests.rs b/crates/ide_assists/src/tests.rs index 6f4f97361d7..227687766dd 100644 --- a/crates/ide_assists/src/tests.rs +++ b/crates/ide_assists/src/tests.rs @@ -12,7 +12,10 @@ use syntax::TextRange; use test_utils::{assert_eq_text, extract_offset}; -use crate::{handlers::Handler, Assist, AssistConfig, AssistContext, AssistKind, Assists}; +use crate::{ + handlers::Handler, Assist, AssistConfig, AssistContext, AssistKind, AssistResolveStrategy, + Assists, +}; pub(crate) const TEST_CONFIG: AssistConfig = AssistConfig { snippet_cap: SnippetCap::new(true), @@ -65,14 +68,14 @@ 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::get(&db, &TEST_CONFIG, true, frange) + let assist = Assist::get(&db, &TEST_CONFIG, AssistResolveStrategy::All, frange) .into_iter() .find(|assist| assist.id.0 == assist_id) .unwrap_or_else(|| { panic!( "\n\nAssist is not applicable: {}\nAvailable assists: {}", assist_id, - Assist::get(&db, &TEST_CONFIG, false, frange) + Assist::get(&db, &TEST_CONFIG, AssistResolveStrategy::None, frange) .into_iter() .map(|assist| assist.id.0) .collect::>() @@ -108,7 +111,7 @@ fn check(handler: Handler, before: &str, expected: ExpectedResult, assist_label: let sema = Semantics::new(&db); let config = TEST_CONFIG; let ctx = AssistContext::new(sema, &config, frange); - let mut acc = Assists::new(&ctx, true); + let mut acc = Assists::new(&ctx, AssistResolveStrategy::All); handler(&mut acc, &ctx); let mut res = acc.finish(); @@ -186,7 +189,7 @@ 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::get(&db, &TEST_CONFIG, false, frange); + let assists = Assist::get(&db, &TEST_CONFIG, AssistResolveStrategy::None, frange); let mut assists = assists.iter(); assert_eq!(assists.next().expect("expected assist").label, "Change visibility to pub(crate)"); @@ -211,7 +214,7 @@ pub fn test_some_range(a: int) -> bool { "#, ); - let assists = Assist::get(&db, &TEST_CONFIG, false, frange); + let assists = Assist::get(&db, &TEST_CONFIG, AssistResolveStrategy::None, frange); let expected = labels(&assists); expect![[r#" @@ -240,7 +243,7 @@ pub fn test_some_range(a: int) -> bool { let mut cfg = TEST_CONFIG; cfg.allowed = Some(vec![AssistKind::Refactor]); - let assists = Assist::get(&db, &cfg, false, frange); + let assists = Assist::get(&db, &cfg, AssistResolveStrategy::None, frange); let expected = labels(&assists); expect![[r#" @@ -255,7 +258,7 @@ pub fn test_some_range(a: int) -> bool { { let mut cfg = TEST_CONFIG; cfg.allowed = Some(vec![AssistKind::RefactorExtract]); - let assists = Assist::get(&db, &cfg, false, frange); + let assists = Assist::get(&db, &cfg, AssistResolveStrategy::None, frange); let expected = labels(&assists); expect![[r#" @@ -268,7 +271,7 @@ pub fn test_some_range(a: int) -> bool { { let mut cfg = TEST_CONFIG; cfg.allowed = Some(vec![AssistKind::QuickFix]); - let assists = Assist::get(&db, &cfg, false, frange); + let assists = Assist::get(&db, &cfg, AssistResolveStrategy::None, frange); let expected = labels(&assists); expect![[r#""#]].assert_eq(&expected); diff --git a/crates/rust-analyzer/src/cli/diagnostics.rs b/crates/rust-analyzer/src/cli/diagnostics.rs index 74f784338fc..c33c8179cb3 100644 --- a/crates/rust-analyzer/src/cli/diagnostics.rs +++ b/crates/rust-analyzer/src/cli/diagnostics.rs @@ -7,7 +7,7 @@ use rustc_hash::FxHashSet; use hir::{db::HirDatabase, Crate, Module}; -use ide::{DiagnosticsConfig, Severity}; +use ide::{AssistResolveStrategy, DiagnosticsConfig, Severity}; use ide_db::base_db::SourceDatabaseExt; use crate::cli::{ @@ -57,8 +57,9 @@ pub fn diagnostics( let crate_name = module.krate().display_name(db).as_deref().unwrap_or("unknown").to_string(); println!("processing crate: {}, module: {}", crate_name, _vfs.file_path(file_id)); - for diagnostic in - analysis.diagnostics(&DiagnosticsConfig::default(), false, file_id).unwrap() + for diagnostic in analysis + .diagnostics(&DiagnosticsConfig::default(), AssistResolveStrategy::None, file_id) + .unwrap() { if matches!(diagnostic.severity, Severity::Error) { found_error = true; diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index 1f59402e5ce..dc350c01fb3 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -8,8 +8,9 @@ }; use ide::{ - AnnotationConfig, FileId, FilePosition, FileRange, HoverAction, HoverGotoTypeData, Query, - RangeInfo, Runnable, RunnableKind, SearchScope, SourceChange, TextEdit, + AnnotationConfig, AssistResolveStrategy, FileId, FilePosition, FileRange, HoverAction, + HoverGotoTypeData, Query, RangeInfo, Runnable, RunnableKind, SearchScope, SourceChange, + TextEdit, }; use ide_db::SymbolKind; use itertools::Itertools; @@ -1004,10 +1005,15 @@ pub(crate) fn handle_code_action( let mut res: Vec = Vec::new(); let code_action_resolve_cap = snap.config.code_action_resolve(); + let resolve = if code_action_resolve_cap { + AssistResolveStrategy::None + } else { + AssistResolveStrategy::All + }; let assists = snap.analysis.assists_with_fixes( &assists_config, &snap.config.diagnostics(), - !code_action_resolve_cap, + resolve, frange, )?; for (index, assist) in assists.into_iter().enumerate() { @@ -1055,7 +1061,8 @@ pub(crate) fn handle_code_action_resolve( let assists = snap.analysis.assists_with_fixes( &assists_config, &snap.config.diagnostics(), - true, + // TODO kb pass a certain id + AssistResolveStrategy::All, frange, )?; @@ -1182,7 +1189,7 @@ pub(crate) fn publish_diagnostics( let diagnostics: Vec = snap .analysis - .diagnostics(&snap.config.diagnostics(), false, file_id)? + .diagnostics(&snap.config.diagnostics(), AssistResolveStrategy::None, file_id)? .into_iter() .map(|d| Diagnostic { range: to_proto::range(&line_index, d.range), From 1679a376f30c5ad8971c0f855074a3f489fee5fa Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 3 May 2021 18:03:28 +0300 Subject: [PATCH 2/9] Resolve single assist only --- crates/ide/src/diagnostics.rs | 6 ++-- crates/ide/src/diagnostics/fixes.rs | 16 ++++----- crates/ide/src/diagnostics/unlinked_file.rs | 2 +- crates/ide/src/lib.rs | 8 ++--- crates/ide/src/ssr.rs | 4 +-- crates/ide_assists/src/lib.rs | 39 +++++++++++++++++++-- crates/rust-analyzer/src/handlers.rs | 29 +++++++++------ crates/rust-analyzer/src/lsp_ext.rs | 2 ++ crates/rust-analyzer/src/to_proto.rs | 4 ++- 9 files changed, 78 insertions(+), 32 deletions(-) diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index 455f20c93f2..b14f908b75e 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs @@ -85,7 +85,7 @@ pub struct DiagnosticsConfig { pub(crate) fn diagnostics( db: &RootDatabase, config: &DiagnosticsConfig, - resolve: AssistResolveStrategy, + resolve: &AssistResolveStrategy, file_id: FileId, ) -> Vec { let _p = profile::span("diagnostics"); @@ -213,7 +213,7 @@ pub(crate) fn diagnostics( fn diagnostic_with_fix( d: &D, sema: &Semantics, - resolve: AssistResolveStrategy, + resolve: &AssistResolveStrategy, ) -> Diagnostic { Diagnostic::error(sema.diagnostics_display_range(d.display_source()).range, d.message()) .with_fix(d.fix(&sema, resolve)) @@ -223,7 +223,7 @@ fn diagnostic_with_fix( fn warning_with_fix( d: &D, sema: &Semantics, - resolve: AssistResolveStrategy, + resolve: &AssistResolveStrategy, ) -> Diagnostic { Diagnostic::hint(sema.diagnostics_display_range(d.display_source()).range, d.message()) .with_fix(d.fix(&sema, resolve)) diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs index f23064eac10..15821500f1a 100644 --- a/crates/ide/src/diagnostics/fixes.rs +++ b/crates/ide/src/diagnostics/fixes.rs @@ -39,7 +39,7 @@ pub(crate) trait DiagnosticWithFix: Diagnostic { fn fix( &self, sema: &Semantics, - _resolve: AssistResolveStrategy, + _resolve: &AssistResolveStrategy, ) -> Option; } @@ -47,7 +47,7 @@ impl DiagnosticWithFix for UnresolvedModule { fn fix( &self, sema: &Semantics, - _resolve: AssistResolveStrategy, + _resolve: &AssistResolveStrategy, ) -> Option { let root = sema.db.parse_or_expand(self.file)?; let unresolved_module = self.decl.to_node(&root); @@ -71,7 +71,7 @@ impl DiagnosticWithFix for NoSuchField { fn fix( &self, sema: &Semantics, - _resolve: AssistResolveStrategy, + _resolve: &AssistResolveStrategy, ) -> Option { let root = sema.db.parse_or_expand(self.file)?; missing_record_expr_field_fix( @@ -86,7 +86,7 @@ impl DiagnosticWithFix for MissingFields { fn fix( &self, sema: &Semantics, - _resolve: AssistResolveStrategy, + _resolve: &AssistResolveStrategy, ) -> Option { // Note that although we could add a diagnostics to // fill the missing tuple field, e.g : @@ -126,7 +126,7 @@ impl DiagnosticWithFix for MissingOkOrSomeInTailExpr { fn fix( &self, sema: &Semantics, - _resolve: AssistResolveStrategy, + _resolve: &AssistResolveStrategy, ) -> Option { let root = sema.db.parse_or_expand(self.file)?; let tail_expr = self.expr.to_node(&root); @@ -143,7 +143,7 @@ impl DiagnosticWithFix for RemoveThisSemicolon { fn fix( &self, sema: &Semantics, - _resolve: AssistResolveStrategy, + _resolve: &AssistResolveStrategy, ) -> Option { let root = sema.db.parse_or_expand(self.file)?; @@ -167,7 +167,7 @@ impl DiagnosticWithFix for IncorrectCase { fn fix( &self, sema: &Semantics, - resolve: AssistResolveStrategy, + resolve: &AssistResolveStrategy, ) -> Option { let root = sema.db.parse_or_expand(self.file)?; let name_node = self.ident.to_node(&root); @@ -191,7 +191,7 @@ impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap { fn fix( &self, sema: &Semantics, - _resolve: AssistResolveStrategy, + _resolve: &AssistResolveStrategy, ) -> Option { let root = sema.db.parse_or_expand(self.file)?; let next_expr = self.next_expr.to_node(&root); diff --git a/crates/ide/src/diagnostics/unlinked_file.rs b/crates/ide/src/diagnostics/unlinked_file.rs index e48528bed25..93fd25dea47 100644 --- a/crates/ide/src/diagnostics/unlinked_file.rs +++ b/crates/ide/src/diagnostics/unlinked_file.rs @@ -54,7 +54,7 @@ impl DiagnosticWithFix for UnlinkedFile { fn fix( &self, sema: &hir::Semantics, - _resolve: AssistResolveStrategy, + _resolve: &AssistResolveStrategy, ) -> Option { // If there's an existing module that could add a `mod` item to include the unlinked file, // suggest that as a fix. diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs index 6847b7e836a..6a88236e3f4 100644 --- a/crates/ide/src/lib.rs +++ b/crates/ide/src/lib.rs @@ -522,7 +522,7 @@ pub fn assists( frange: FileRange, ) -> Cancelable> { self.with_db(|db| { - let ssr_assists = ssr::ssr_assists(db, resolve, frange); + let ssr_assists = ssr::ssr_assists(db, &resolve, frange); let mut acc = Assist::get(db, config, resolve, frange); acc.extend(ssr_assists.into_iter()); acc @@ -536,7 +536,7 @@ pub fn diagnostics( resolve: AssistResolveStrategy, file_id: FileId, ) -> Cancelable> { - self.with_db(|db| diagnostics::diagnostics(db, config, resolve, file_id)) + self.with_db(|db| diagnostics::diagnostics(db, config, &resolve, file_id)) } /// Convenience function to return assists + quick fixes for diagnostics @@ -553,9 +553,9 @@ pub fn assists_with_fixes( }; self.with_db(|db| { - let ssr_assists = ssr::ssr_assists(db, resolve, frange); + let ssr_assists = ssr::ssr_assists(db, &resolve, frange); let diagnostic_assists = if include_fixes { - diagnostics::diagnostics(db, diagnostics_config, resolve, frange.file_id) + diagnostics::diagnostics(db, diagnostics_config, &resolve, frange.file_id) .into_iter() .filter_map(|it| it.fix) .filter(|it| it.target.intersect(frange.range).is_some()) diff --git a/crates/ide/src/ssr.rs b/crates/ide/src/ssr.rs index 785ce301099..1695e52ece8 100644 --- a/crates/ide/src/ssr.rs +++ b/crates/ide/src/ssr.rs @@ -7,7 +7,7 @@ pub(crate) fn ssr_assists( db: &RootDatabase, - resolve: AssistResolveStrategy, + resolve: &AssistResolveStrategy, frange: FileRange, ) -> Vec { let mut ssr_assists = Vec::with_capacity(2); @@ -73,7 +73,7 @@ fn get_assists(ra_fixture: &str, resolve: AssistResolveStrategy) -> Vec let mut local_roots = FxHashSet::default(); local_roots.insert(ide_db::base_db::fixture::WORKSPACE); db.set_local_roots_with_durability(Arc::new(local_roots), Durability::HIGH); - ssr_assists(&db, resolve, FileRange { file_id, range: range_or_offset.into() }) + ssr_assists(&db, &resolve, FileRange { file_id, range: range_or_offset.into() }) } #[test] diff --git a/crates/ide_assists/src/lib.rs b/crates/ide_assists/src/lib.rs index 397d2a3d0ee..01addffe949 100644 --- a/crates/ide_assists/src/lib.rs +++ b/crates/ide_assists/src/lib.rs @@ -17,6 +17,8 @@ macro_rules! eprintln { pub mod utils; pub mod ast_transform; +use std::str::FromStr; + use hir::Semantics; use ide_db::base_db::FileRange; use ide_db::{label::Label, source_change::SourceChange, RootDatabase}; @@ -56,6 +58,35 @@ pub fn contains(self, other: AssistKind) -> bool { _ => return false, } } + + pub fn name(&self) -> &str { + match self { + AssistKind::None => "None", + AssistKind::QuickFix => "QuickFix", + AssistKind::Generate => "Generate", + AssistKind::Refactor => "Refactor", + AssistKind::RefactorExtract => "RefactorExtract", + AssistKind::RefactorInline => "RefactorInline", + AssistKind::RefactorRewrite => "RefactorRewrite", + } + } +} + +impl FromStr for AssistKind { + type Err = String; + + fn from_str(s: &str) -> Result { + match s { + "None" => Ok(AssistKind::None), + "QuickFix" => Ok(AssistKind::QuickFix), + "Generate" => Ok(AssistKind::Generate), + "Refactor" => Ok(AssistKind::Refactor), + "RefactorExtract" => Ok(AssistKind::RefactorExtract), + "RefactorInline" => Ok(AssistKind::RefactorInline), + "RefactorRewrite" => Ok(AssistKind::RefactorRewrite), + unknown => Err(format!("Unknown AssistKind: '{}'", unknown)), + } + } } /// Unique identifier of the assist, should not be shown to the user @@ -64,11 +95,11 @@ pub fn contains(self, other: AssistKind) -> bool { pub struct AssistId(pub &'static str, pub AssistKind); // TODO kb docs -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone)] pub enum AssistResolveStrategy { None, All, - Single(AssistId), + Single(String, AssistKind), } impl AssistResolveStrategy { @@ -76,7 +107,9 @@ pub fn should_resolve(&self, id: &AssistId) -> bool { match self { AssistResolveStrategy::None => false, AssistResolveStrategy::All => true, - AssistResolveStrategy::Single(id_to_resolve) => id_to_resolve == id, + AssistResolveStrategy::Single(id_to_resolve, kind) => { + id_to_resolve == id.0 && kind == &id.1 + } } } } diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index dc350c01fb3..cd6bbf30333 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -8,9 +8,9 @@ }; use ide::{ - AnnotationConfig, AssistResolveStrategy, FileId, FilePosition, FileRange, HoverAction, - HoverGotoTypeData, Query, RangeInfo, Runnable, RunnableKind, SearchScope, SourceChange, - TextEdit, + AnnotationConfig, AssistKind, AssistResolveStrategy, FileId, FilePosition, FileRange, + HoverAction, HoverGotoTypeData, Query, RangeInfo, Runnable, RunnableKind, SearchScope, + SourceChange, TextEdit, }; use ide_db::SymbolKind; use itertools::Itertools; @@ -28,7 +28,7 @@ use project_model::TargetKind; use serde::{Deserialize, Serialize}; use serde_json::to_value; -use stdx::{format_to, split_once}; +use stdx::format_to; use syntax::{algo, ast, AstNode, TextRange, TextSize}; use crate::{ @@ -1058,18 +1058,27 @@ pub(crate) fn handle_code_action_resolve( .only .map(|it| it.into_iter().filter_map(from_proto::assist_kind).collect()); + let assist_kind: AssistKind = match params.kind.parse() { + Ok(kind) => kind, + Err(e) => { + return Err(LspError::new( + ErrorCode::InvalidParams as i32, + format!("For the assist to resolve, failed to parse the kind: {}", e), + ) + .into()) + } + }; + let assists = snap.analysis.assists_with_fixes( &assists_config, &snap.config.diagnostics(), - // TODO kb pass a certain id - AssistResolveStrategy::All, + AssistResolveStrategy::Single(params.id.clone(), assist_kind), frange, )?; - let (id, index) = split_once(¶ms.id, ':').unwrap(); - let index = index.parse::().unwrap(); - let assist = &assists[index]; - assert!(assist.id.0 == id); + let assist = &assists[params.index]; + assert!(assist.id.0 == params.id); + assert!(assist.id.1 == assist_kind); let edit = to_proto::code_action(&snap, assist.clone(), None)?.edit; code_action.edit = edit; Ok(code_action) diff --git a/crates/rust-analyzer/src/lsp_ext.rs b/crates/rust-analyzer/src/lsp_ext.rs index b8835a5349b..292aedc9c2c 100644 --- a/crates/rust-analyzer/src/lsp_ext.rs +++ b/crates/rust-analyzer/src/lsp_ext.rs @@ -303,6 +303,8 @@ pub struct CodeAction { pub struct CodeActionData { pub code_action_params: lsp_types::CodeActionParams, pub id: String, + pub kind: String, + pub index: usize, } #[derive(Debug, Eq, PartialEq, Clone, Default, Deserialize, Serialize)] diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs index c2361b32e73..62e16680b4a 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -897,8 +897,10 @@ pub(crate) fn code_action( (Some(it), _) => res.edit = Some(snippet_workspace_edit(snap, it)?), (None, Some((index, code_action_params))) => { res.data = Some(lsp_ext::CodeActionData { - id: format!("{}:{}", assist.id.0, index.to_string()), + id: assist.id.0.to_string(), code_action_params, + kind: assist.id.1.name().to_string(), + index, }); } (None, None) => { From 28293d370ffc4270bb6244579166f0df18962951 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 3 May 2021 18:16:35 +0300 Subject: [PATCH 3/9] Add docs and use better naming --- crates/ide/src/lib.rs | 4 +++- crates/ide_assists/src/lib.rs | 25 ++++++++++++++++++++----- crates/rust-analyzer/src/handlers.rs | 4 ++-- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/crates/ide/src/lib.rs b/crates/ide/src/lib.rs index 6a88236e3f4..8e5b7204496 100644 --- a/crates/ide/src/lib.rs +++ b/crates/ide/src/lib.rs @@ -87,7 +87,9 @@ macro_rules! eprintln { }, }; pub use hir::{Documentation, Semantics}; -pub use ide_assists::{Assist, AssistConfig, AssistId, AssistKind, AssistResolveStrategy}; +pub use ide_assists::{ + Assist, AssistConfig, AssistId, AssistKind, AssistResolveStrategy, SingleResolve, +}; pub use ide_completion::{ CompletionConfig, CompletionItem, CompletionItemKind, CompletionRelevance, ImportEdit, InsertTextFormat, diff --git a/crates/ide_assists/src/lib.rs b/crates/ide_assists/src/lib.rs index 01addffe949..5a0047f0373 100644 --- a/crates/ide_assists/src/lib.rs +++ b/crates/ide_assists/src/lib.rs @@ -94,12 +94,27 @@ fn from_str(s: &str) -> Result { #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct AssistId(pub &'static str, pub AssistKind); -// TODO kb docs -#[derive(Debug, Clone)] +/// A way to control how many asssist to resolve during the assist resolution. +/// When an assist is resolved, its edits are calculated that might be costly to always do by default. +#[derive(Debug)] pub enum AssistResolveStrategy { + /// No assists should be resolved. None, + /// All assists should be resolved. All, - Single(String, AssistKind), + /// Only a certain assists should be resolved. + Single(SingleResolve), +} + +/// Hold the [`AssistId`] data of a certain assist to resolve. +/// The original id object cannot be used due to a `'static` lifetime +/// and the requirement to construct this struct dynamically during the resolve handling. +#[derive(Debug)] +pub struct SingleResolve { + /// The id of the assist. + pub assist_id: String, + // The kind of the assist. + pub assist_kind: AssistKind, } impl AssistResolveStrategy { @@ -107,8 +122,8 @@ pub fn should_resolve(&self, id: &AssistId) -> bool { match self { AssistResolveStrategy::None => false, AssistResolveStrategy::All => true, - AssistResolveStrategy::Single(id_to_resolve, kind) => { - id_to_resolve == id.0 && kind == &id.1 + AssistResolveStrategy::Single(single_resolve) => { + single_resolve.assist_id == id.0 && single_resolve.assist_kind == id.1 } } } diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index cd6bbf30333..304951b7d23 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -10,7 +10,7 @@ use ide::{ AnnotationConfig, AssistKind, AssistResolveStrategy, FileId, FilePosition, FileRange, HoverAction, HoverGotoTypeData, Query, RangeInfo, Runnable, RunnableKind, SearchScope, - SourceChange, TextEdit, + SingleResolve, SourceChange, TextEdit, }; use ide_db::SymbolKind; use itertools::Itertools; @@ -1072,7 +1072,7 @@ pub(crate) fn handle_code_action_resolve( let assists = snap.analysis.assists_with_fixes( &assists_config, &snap.config.diagnostics(), - AssistResolveStrategy::Single(params.id.clone(), assist_kind), + AssistResolveStrategy::Single(SingleResolve { assist_id: params.id.clone(), assist_kind }), frange, )?; From b1d600a1ecbf2032ef09d56559111da4382135df Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 3 May 2021 18:18:45 +0300 Subject: [PATCH 4/9] Less panics in the assist resolution --- crates/rust-analyzer/src/handlers.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index 304951b7d23..77faf757935 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -1077,8 +1077,16 @@ pub(crate) fn handle_code_action_resolve( )?; let assist = &assists[params.index]; - assert!(assist.id.0 == params.id); - assert!(assist.id.1 == assist_kind); + if assist.id.0 != params.id || assist.id.1 != assist_kind { + return Err(LspError::new( + ErrorCode::InvalidParams as i32, + format!( + "Failed to find exactly the same assist at index {} for the resolve parameters given. Expected id and kind: {}, {:?}, actual id: {:?}.", + params.index, params.id, assist_kind, assist.id + ), + ) + .into()); + } let edit = to_proto::code_action(&snap, assist.clone(), None)?.edit; code_action.edit = edit; Ok(code_action) From 8089a227f4b40872cf8491cfb9e065a8b05705a2 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 3 May 2021 18:40:04 +0300 Subject: [PATCH 5/9] Tests added --- crates/ide/src/ssr.rs | 1 - crates/ide_assists/src/tests.rs | 243 +++++++++++++++++++++++++++++++- 2 files changed, 242 insertions(+), 2 deletions(-) diff --git a/crates/ide/src/ssr.rs b/crates/ide/src/ssr.rs index 1695e52ece8..97ad66d3589 100644 --- a/crates/ide/src/ssr.rs +++ b/crates/ide/src/ssr.rs @@ -89,7 +89,6 @@ fn foo() {} assert_eq!(0, assists.len()); } - // TODO kb add partial resolve test #[test] fn resolve_edits_true() { let assists = get_assists( diff --git a/crates/ide_assists/src/tests.rs b/crates/ide_assists/src/tests.rs index 227687766dd..9c284799847 100644 --- a/crates/ide_assists/src/tests.rs +++ b/crates/ide_assists/src/tests.rs @@ -14,7 +14,7 @@ use crate::{ handlers::Handler, Assist, AssistConfig, AssistContext, AssistKind, AssistResolveStrategy, - Assists, + Assists, SingleResolve, }; pub(crate) const TEST_CONFIG: AssistConfig = AssistConfig { @@ -277,3 +277,244 @@ pub fn test_some_range(a: int) -> bool { expect![[r#""#]].assert_eq(&expected); } } + +#[test] +fn various_resolve_strategies() { + let (db, frange) = RootDatabase::with_range( + r#" +pub fn test_some_range(a: int) -> bool { + if let 2..6 = $05$0 { + true + } else { + false + } +} +"#, + ); + + let mut cfg = TEST_CONFIG; + cfg.allowed = Some(vec![AssistKind::RefactorExtract]); + + { + let assists = Assist::get(&db, &cfg, AssistResolveStrategy::None, frange); + assert_eq!(2, assists.len()); + let mut assists = assists.into_iter(); + + let extract_into_variable_assist = assists.next().unwrap(); + expect![[r#" + Assist { + id: AssistId( + "extract_variable", + RefactorExtract, + ), + label: "Extract into variable", + group: None, + target: 59..60, + source_change: None, + } + "#]] + .assert_debug_eq(&extract_into_variable_assist); + + let extract_into_function_assist = assists.next().unwrap(); + expect![[r#" + Assist { + id: AssistId( + "extract_function", + RefactorExtract, + ), + label: "Extract into function", + group: None, + target: 59..60, + source_change: None, + } + "#]] + .assert_debug_eq(&extract_into_function_assist); + } + + { + let assists = Assist::get( + &db, + &cfg, + AssistResolveStrategy::Single(SingleResolve { + assist_id: "SOMETHING_MISMATCHING".to_string(), + assist_kind: AssistKind::RefactorExtract, + }), + frange, + ); + assert_eq!(2, assists.len()); + let mut assists = assists.into_iter(); + + let extract_into_variable_assist = assists.next().unwrap(); + expect![[r#" + Assist { + id: AssistId( + "extract_variable", + RefactorExtract, + ), + label: "Extract into variable", + group: None, + target: 59..60, + source_change: None, + } + "#]] + .assert_debug_eq(&extract_into_variable_assist); + + let extract_into_function_assist = assists.next().unwrap(); + expect![[r#" + Assist { + id: AssistId( + "extract_function", + RefactorExtract, + ), + label: "Extract into function", + group: None, + target: 59..60, + source_change: None, + } + "#]] + .assert_debug_eq(&extract_into_function_assist); + } + + { + let assists = Assist::get( + &db, + &cfg, + AssistResolveStrategy::Single(SingleResolve { + assist_id: "extract_variable".to_string(), + assist_kind: AssistKind::RefactorExtract, + }), + frange, + ); + assert_eq!(2, assists.len()); + let mut assists = assists.into_iter(); + + let extract_into_variable_assist = assists.next().unwrap(); + expect![[r#" + Assist { + id: AssistId( + "extract_variable", + RefactorExtract, + ), + label: "Extract into variable", + group: None, + target: 59..60, + source_change: Some( + SourceChange { + source_file_edits: { + FileId( + 0, + ): TextEdit { + indels: [ + Indel { + insert: "let $0var_name = 5;\n ", + delete: 45..45, + }, + Indel { + insert: "var_name", + delete: 59..60, + }, + ], + }, + }, + file_system_edits: [], + is_snippet: true, + }, + ), + } + "#]] + .assert_debug_eq(&extract_into_variable_assist); + + let extract_into_function_assist = assists.next().unwrap(); + expect![[r#" + Assist { + id: AssistId( + "extract_function", + RefactorExtract, + ), + label: "Extract into function", + group: None, + target: 59..60, + source_change: None, + } + "#]] + .assert_debug_eq(&extract_into_function_assist); + } + + { + let assists = Assist::get(&db, &cfg, AssistResolveStrategy::All, frange); + assert_eq!(2, assists.len()); + let mut assists = assists.into_iter(); + + let extract_into_variable_assist = assists.next().unwrap(); + expect![[r#" + Assist { + id: AssistId( + "extract_variable", + RefactorExtract, + ), + label: "Extract into variable", + group: None, + target: 59..60, + source_change: Some( + SourceChange { + source_file_edits: { + FileId( + 0, + ): TextEdit { + indels: [ + Indel { + insert: "let $0var_name = 5;\n ", + delete: 45..45, + }, + Indel { + insert: "var_name", + delete: 59..60, + }, + ], + }, + }, + file_system_edits: [], + is_snippet: true, + }, + ), + } + "#]] + .assert_debug_eq(&extract_into_variable_assist); + + let extract_into_function_assist = assists.next().unwrap(); + expect![[r#" + Assist { + id: AssistId( + "extract_function", + RefactorExtract, + ), + label: "Extract into function", + group: None, + target: 59..60, + source_change: Some( + SourceChange { + source_file_edits: { + FileId( + 0, + ): TextEdit { + indels: [ + Indel { + insert: "fun_name()", + delete: 59..60, + }, + Indel { + insert: "\n\nfn $0fun_name() -> i32 {\n 5\n}", + delete: 110..110, + }, + ], + }, + }, + file_system_edits: [], + is_snippet: true, + }, + ), + } + "#]] + .assert_debug_eq(&extract_into_function_assist); + } +} From 53a73de3d10e20a13153c94e050a8ad9230169eb Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 3 May 2021 18:44:58 +0300 Subject: [PATCH 6/9] Small fixes --- crates/ide/src/ssr.rs | 2 +- crates/ide_assists/src/lib.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ide/src/ssr.rs b/crates/ide/src/ssr.rs index 97ad66d3589..57ec8026140 100644 --- a/crates/ide/src/ssr.rs +++ b/crates/ide/src/ssr.rs @@ -13,7 +13,7 @@ pub(crate) fn ssr_assists( let mut ssr_assists = Vec::with_capacity(2); let (match_finder, comment_range) = match ide_ssr::ssr_from_comment(db, frange) { - Some((match_finder, comment_range)) => (match_finder, comment_range), + Some(ssr_data) => ssr_data, None => return ssr_assists, }; let id = AssistId("ssr", AssistKind::RefactorRewrite); diff --git a/crates/ide_assists/src/lib.rs b/crates/ide_assists/src/lib.rs index 5a0047f0373..723531078f1 100644 --- a/crates/ide_assists/src/lib.rs +++ b/crates/ide_assists/src/lib.rs @@ -28,7 +28,7 @@ macro_rules! eprintln { pub use assist_config::AssistConfig; -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum AssistKind { // FIXME: does the None variant make sense? Probably not. None, @@ -91,7 +91,7 @@ fn from_str(s: &str) -> Result { /// Unique identifier of the assist, should not be shown to the user /// directly. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct AssistId(pub &'static str, pub AssistKind); /// A way to control how many asssist to resolve during the assist resolution. From 3eab6ce2e3eef6c8dca1127b4c1375899bf13b8c Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 3 May 2021 19:00:24 +0300 Subject: [PATCH 7/9] Touch lsp-extensions.md --- docs/dev/lsp-extensions.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/dev/lsp-extensions.md b/docs/dev/lsp-extensions.md index f0f981802ef..e2ea695f260 100644 --- a/docs/dev/lsp-extensions.md +++ b/docs/dev/lsp-extensions.md @@ -81,6 +81,7 @@ If this capability is set, `CodeAction` returned from the server contain an addi interface CodeAction { title: string; group?: string; + data?: string; ... } ``` @@ -101,6 +102,8 @@ The set of actions `[ { title: "foo" }, { group: "frobnicate", title: "bar" }, { Alternatively, selecting `frobnicate` could present a user with an additional menu to choose between `bar` and `baz`. +`data` field contains optional json data for deferred resolve of the action data that's slow to compute in the original request. + ### Example ```rust From 90fc32937785b3f17899f14d8cb2f7b3738a9850 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 3 May 2021 19:35:44 +0300 Subject: [PATCH 8/9] Index retrieval fix --- crates/ide_assists/src/lib.rs | 2 +- crates/rust-analyzer/src/handlers.rs | 12 +++++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/ide_assists/src/lib.rs b/crates/ide_assists/src/lib.rs index 723531078f1..2e0c58504c0 100644 --- a/crates/ide_assists/src/lib.rs +++ b/crates/ide_assists/src/lib.rs @@ -102,7 +102,7 @@ pub enum AssistResolveStrategy { None, /// All assists should be resolved. All, - /// Only a certain assists should be resolved. + /// Only a certain assist should be resolved. Single(SingleResolve), } diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index 77faf757935..0fd03bbaa72 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -1076,7 +1076,17 @@ pub(crate) fn handle_code_action_resolve( frange, )?; - let assist = &assists[params.index]; + let assist = match assists.get(params.index) { + Some(assist) => assist, + None => return Err(LspError::new( + ErrorCode::InvalidParams as i32, + format!( + "Failed to find the assist for index {} provided by the resolve request. Expected assist id: {:?}", + params.index, params.id, + ), + ) + .into()) + }; if assist.id.0 != params.id || assist.id.1 != assist_kind { return Err(LspError::new( ErrorCode::InvalidParams as i32, From 734b95a1ac9a65cec45d8f9024d53638e6a3cd2e Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 3 May 2021 22:58:53 +0300 Subject: [PATCH 9/9] Code review fixes --- crates/rust-analyzer/src/handlers.rs | 38 ++++++++++++++++++++-------- crates/rust-analyzer/src/lsp_ext.rs | 2 -- crates/rust-analyzer/src/to_proto.rs | 4 +-- docs/dev/lsp-extensions.md | 3 --- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index 0fd03bbaa72..f6e40f87222 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -1058,41 +1058,44 @@ pub(crate) fn handle_code_action_resolve( .only .map(|it| it.into_iter().filter_map(from_proto::assist_kind).collect()); - let assist_kind: AssistKind = match params.kind.parse() { - Ok(kind) => kind, + let (assist_index, assist_resolve) = match parse_action_id(¶ms.id) { + Ok(parsed_data) => parsed_data, Err(e) => { return Err(LspError::new( ErrorCode::InvalidParams as i32, - format!("For the assist to resolve, failed to parse the kind: {}", e), + format!("Failed to parse action id string '{}': {}", params.id, e), ) .into()) } }; + let expected_assist_id = assist_resolve.assist_id.clone(); + let expected_kind = assist_resolve.assist_kind; + let assists = snap.analysis.assists_with_fixes( &assists_config, &snap.config.diagnostics(), - AssistResolveStrategy::Single(SingleResolve { assist_id: params.id.clone(), assist_kind }), + AssistResolveStrategy::Single(assist_resolve), frange, )?; - let assist = match assists.get(params.index) { + let assist = match assists.get(assist_index) { Some(assist) => assist, None => return Err(LspError::new( ErrorCode::InvalidParams as i32, format!( - "Failed to find the assist for index {} provided by the resolve request. Expected assist id: {:?}", - params.index, params.id, + "Failed to find the assist for index {} provided by the resolve request. Resolve request assist id: {}", + assist_index, params.id, ), ) .into()) }; - if assist.id.0 != params.id || assist.id.1 != assist_kind { + if assist.id.0 != expected_assist_id || assist.id.1 != expected_kind { return Err(LspError::new( ErrorCode::InvalidParams as i32, format!( - "Failed to find exactly the same assist at index {} for the resolve parameters given. Expected id and kind: {}, {:?}, actual id: {:?}.", - params.index, params.id, assist_kind, assist.id + "Mismatching assist at index {} for the resolve parameters given. Resolve request assist id: {}, actual id: {:?}.", + assist_index, params.id, assist.id ), ) .into()); @@ -1102,6 +1105,21 @@ pub(crate) fn handle_code_action_resolve( Ok(code_action) } +fn parse_action_id(action_id: &str) -> Result<(usize, SingleResolve), String> { + let id_parts = action_id.split(':').collect_vec(); + match id_parts.as_slice() { + &[assist_id_string, assist_kind_string, index_string] => { + let assist_kind: AssistKind = assist_kind_string.parse()?; + let index: usize = match index_string.parse() { + Ok(index) => index, + Err(e) => return Err(format!("Incorrect index string: {}", e)), + }; + Ok((index, SingleResolve { assist_id: assist_id_string.to_string(), assist_kind })) + } + _ => Err("Action id contains incorrect number of segments".to_string()), + } +} + pub(crate) fn handle_code_lens( snap: GlobalStateSnapshot, params: lsp_types::CodeLensParams, diff --git a/crates/rust-analyzer/src/lsp_ext.rs b/crates/rust-analyzer/src/lsp_ext.rs index 292aedc9c2c..b8835a5349b 100644 --- a/crates/rust-analyzer/src/lsp_ext.rs +++ b/crates/rust-analyzer/src/lsp_ext.rs @@ -303,8 +303,6 @@ pub struct CodeAction { pub struct CodeActionData { pub code_action_params: lsp_types::CodeActionParams, pub id: String, - pub kind: String, - pub index: usize, } #[derive(Debug, Eq, PartialEq, Clone, Default, Deserialize, Serialize)] diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs index 62e16680b4a..1d27aa7b373 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -897,10 +897,8 @@ pub(crate) fn code_action( (Some(it), _) => res.edit = Some(snippet_workspace_edit(snap, it)?), (None, Some((index, code_action_params))) => { res.data = Some(lsp_ext::CodeActionData { - id: assist.id.0.to_string(), + id: format!("{}:{}:{}", assist.id.0, assist.id.1.name(), index), code_action_params, - kind: assist.id.1.name().to_string(), - index, }); } (None, None) => { diff --git a/docs/dev/lsp-extensions.md b/docs/dev/lsp-extensions.md index e2ea695f260..f0f981802ef 100644 --- a/docs/dev/lsp-extensions.md +++ b/docs/dev/lsp-extensions.md @@ -81,7 +81,6 @@ If this capability is set, `CodeAction` returned from the server contain an addi interface CodeAction { title: string; group?: string; - data?: string; ... } ``` @@ -102,8 +101,6 @@ The set of actions `[ { title: "foo" }, { group: "frobnicate", title: "bar" }, { Alternatively, selecting `frobnicate` could present a user with an additional menu to choose between `bar` and `baz`. -`data` field contains optional json data for deferred resolve of the action data that's slow to compute in the original request. - ### Example ```rust