From cf55806257776baf7db6b02d260bdaa9e851c7d4 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 29 Jul 2020 11:44:01 +1000 Subject: SSR: Restrict to current selection if any The selection is also used to avoid unnecessary work, but only to the file level. Further restricting unnecessary work is left for later. --- crates/ra_ide/src/lib.rs | 3 +- crates/ra_ide/src/ssr.rs | 8 ++- crates/ra_ssr/src/lib.rs | 11 ++++- crates/ra_ssr/src/matching.rs | 4 +- crates/ra_ssr/src/search.rs | 87 +++++++++++++++++++++++--------- crates/ra_ssr/src/tests.rs | 96 +++++++++++++++++++++++++++++------- crates/rust-analyzer/src/handlers.rs | 13 ++++- crates/rust-analyzer/src/lsp_ext.rs | 3 ++ 8 files changed, 177 insertions(+), 48 deletions(-) (limited to 'crates') diff --git a/crates/ra_ide/src/lib.rs b/crates/ra_ide/src/lib.rs index 4c4d9f6fa..0fede0d87 100644 --- a/crates/ra_ide/src/lib.rs +++ b/crates/ra_ide/src/lib.rs @@ -510,9 +510,10 @@ impl Analysis { query: &str, parse_only: bool, position: FilePosition, + selections: Vec, ) -> Cancelable> { self.with_db(|db| { - let edits = ssr::parse_search_replace(query, parse_only, db, position)?; + let edits = ssr::parse_search_replace(query, parse_only, db, position, selections)?; Ok(SourceChange::from(edits)) }) } diff --git a/crates/ra_ide/src/ssr.rs b/crates/ra_ide/src/ssr.rs index 95d8f79b8..63010677a 100644 --- a/crates/ra_ide/src/ssr.rs +++ b/crates/ra_ide/src/ssr.rs @@ -1,4 +1,4 @@ -use ra_db::FilePosition; +use ra_db::{FilePosition, FileRange}; use ra_ide_db::RootDatabase; use crate::SourceFileEdit; @@ -24,6 +24,9 @@ use ra_ssr::{MatchFinder, SsrError, SsrRule}; // Method calls should generally be written in UFCS form. e.g. `foo::Bar::baz($s, $a)` will match // `$s.baz($a)`, provided the method call `baz` resolves to the method `foo::Bar::baz`. // +// The scope of the search / replace will be restricted to the current selection if any, otherwise +// it will apply to the whole workspace. +// // Placeholders may be given constraints by writing them as `${::...}`. // // Supported constraints: @@ -57,9 +60,10 @@ pub fn parse_search_replace( parse_only: bool, db: &RootDatabase, position: FilePosition, + selections: Vec, ) -> Result, SsrError> { let rule: SsrRule = rule.parse()?; - let mut match_finder = MatchFinder::in_context(db, position); + let mut match_finder = MatchFinder::in_context(db, position, selections); match_finder.add_rule(rule)?; if parse_only { return Ok(Vec::new()); diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index 7014a6ac6..73abfecb2 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -52,6 +52,7 @@ pub struct MatchFinder<'db> { sema: Semantics<'db, ra_ide_db::RootDatabase>, rules: Vec, resolution_scope: resolving::ResolutionScope<'db>, + restrict_ranges: Vec, } impl<'db> MatchFinder<'db> { @@ -60,10 +61,17 @@ impl<'db> MatchFinder<'db> { pub fn in_context( db: &'db ra_ide_db::RootDatabase, lookup_context: FilePosition, + mut restrict_ranges: Vec, ) -> MatchFinder<'db> { + restrict_ranges.retain(|range| !range.range.is_empty()); let sema = Semantics::new(db); let resolution_scope = resolving::ResolutionScope::new(&sema, lookup_context); - MatchFinder { sema: Semantics::new(db), rules: Vec::new(), resolution_scope } + MatchFinder { + sema: Semantics::new(db), + rules: Vec::new(), + resolution_scope, + restrict_ranges, + } } /// Constructs an instance using the start of the first file in `db` as the lookup context. @@ -79,6 +87,7 @@ impl<'db> MatchFinder<'db> { Ok(MatchFinder::in_context( db, FilePosition { file_id: first_file_id, offset: 0.into() }, + vec![], )) } else { bail!("No files to search"); diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index 4862622bd..c1b66748e 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -706,8 +706,8 @@ mod tests { let rule: SsrRule = "foo($x) ==>> bar($x)".parse().unwrap(); let input = "fn foo() {} fn bar() {} fn main() { foo(1+2); }"; - let (db, position) = crate::tests::single_file(input); - let mut match_finder = MatchFinder::in_context(&db, position); + let (db, position, selections) = crate::tests::single_file(input); + let mut match_finder = MatchFinder::in_context(&db, position, selections); match_finder.add_rule(rule).unwrap(); let matches = match_finder.matches(); assert_eq!(matches.matches.len(), 1); diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index bcf0f0468..0f512cb62 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -5,12 +5,13 @@ use crate::{ resolving::{ResolvedPath, ResolvedPattern, ResolvedRule}, Match, MatchFinder, }; -use ra_db::FileRange; +use ra_db::{FileId, FileRange}; use ra_ide_db::{ defs::Definition, search::{Reference, SearchScope}, }; use ra_syntax::{ast, AstNode, SyntaxKind, SyntaxNode}; +use rustc_hash::FxHashSet; use test_utils::mark; /// A cache for the results of find_usages. This is for when we have multiple patterns that have the @@ -54,11 +55,7 @@ impl<'db> MatchFinder<'db> { mark::hit!(use_declaration_with_braces); continue; } - if let Ok(m) = - matching::get_match(false, rule, &node_to_match, &None, &self.sema) - { - matches_out.push(m); - } + self.try_add_match(rule, &node_to_match, &None, matches_out); } } } @@ -121,25 +118,39 @@ impl<'db> MatchFinder<'db> { // FIXME: We should ideally have a test that checks that we edit local roots and not library // roots. This probably would require some changes to fixtures, since currently everything // seems to get put into a single source root. - use ra_db::SourceDatabaseExt; - use ra_ide_db::symbol_index::SymbolsDatabase; let mut files = Vec::new(); - for &root in self.sema.db.local_roots().iter() { - let sr = self.sema.db.source_root(root); - files.extend(sr.iter()); - } + self.search_files_do(|file_id| { + files.push(file_id); + }); SearchScope::files(&files) } fn slow_scan(&self, rule: &ResolvedRule, matches_out: &mut Vec) { - use ra_db::SourceDatabaseExt; - use ra_ide_db::symbol_index::SymbolsDatabase; - for &root in self.sema.db.local_roots().iter() { - let sr = self.sema.db.source_root(root); - for file_id in sr.iter() { - let file = self.sema.parse(file_id); - let code = file.syntax(); - self.slow_scan_node(code, rule, &None, matches_out); + self.search_files_do(|file_id| { + let file = self.sema.parse(file_id); + let code = file.syntax(); + self.slow_scan_node(code, rule, &None, matches_out); + }) + } + + fn search_files_do(&self, mut callback: impl FnMut(FileId)) { + if self.restrict_ranges.is_empty() { + // Unrestricted search. + use ra_db::SourceDatabaseExt; + use ra_ide_db::symbol_index::SymbolsDatabase; + for &root in self.sema.db.local_roots().iter() { + let sr = self.sema.db.source_root(root); + for file_id in sr.iter() { + callback(file_id); + } + } + } else { + // Search is restricted, deduplicate file IDs (generally only one). + let mut files = FxHashSet::default(); + for range in &self.restrict_ranges { + if files.insert(range.file_id) { + callback(range.file_id); + } } } } @@ -154,9 +165,7 @@ impl<'db> MatchFinder<'db> { if !is_search_permitted(code) { return; } - if let Ok(m) = matching::get_match(false, rule, &code, restrict_range, &self.sema) { - matches_out.push(m); - } + self.try_add_match(rule, &code, restrict_range, matches_out); // If we've got a macro call, we already tried matching it pre-expansion, which is the only // way to match the whole macro, now try expanding it and matching the expansion. if let Some(macro_call) = ast::MacroCall::cast(code.clone()) { @@ -178,6 +187,38 @@ impl<'db> MatchFinder<'db> { self.slow_scan_node(&child, rule, restrict_range, matches_out); } } + + fn try_add_match( + &self, + rule: &ResolvedRule, + code: &SyntaxNode, + restrict_range: &Option, + matches_out: &mut Vec, + ) { + if !self.within_range_restrictions(code) { + mark::hit!(replace_nonpath_within_selection); + return; + } + if let Ok(m) = matching::get_match(false, rule, code, restrict_range, &self.sema) { + matches_out.push(m); + } + } + + /// Returns whether `code` is within one of our range restrictions if we have any. No range + /// restrictions is considered unrestricted and always returns true. + fn within_range_restrictions(&self, code: &SyntaxNode) -> bool { + if self.restrict_ranges.is_empty() { + // There is no range restriction. + return true; + } + let node_range = self.sema.original_range(code); + for range in &self.restrict_ranges { + if range.file_id == node_range.file_id && range.range.contains_range(node_range.range) { + return true; + } + } + false + } } /// Returns whether we support matching within `node` and all of its ancestors. diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 18ef2506a..6a44ef378 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -1,9 +1,9 @@ use crate::{MatchFinder, SsrRule}; use expect::{expect, Expect}; -use ra_db::{salsa::Durability, FileId, FilePosition, SourceDatabaseExt}; +use ra_db::{salsa::Durability, FileId, FilePosition, FileRange, SourceDatabaseExt}; use rustc_hash::FxHashSet; use std::sync::Arc; -use test_utils::mark; +use test_utils::{mark, RangeOrOffset}; fn parse_error_text(query: &str) -> String { format!("{}", query.parse::().unwrap_err()) @@ -60,20 +60,32 @@ fn parser_undefined_placeholder_in_replacement() { } /// `code` may optionally contain a cursor marker `<|>`. If it doesn't, then the position will be -/// the start of the file. -pub(crate) fn single_file(code: &str) -> (ra_ide_db::RootDatabase, FilePosition) { +/// the start of the file. If there's a second cursor marker, then we'll return a single range. +pub(crate) fn single_file(code: &str) -> (ra_ide_db::RootDatabase, FilePosition, Vec) { use ra_db::fixture::WithFixture; use ra_ide_db::symbol_index::SymbolsDatabase; - let (mut db, position) = if code.contains(test_utils::CURSOR_MARKER) { - ra_ide_db::RootDatabase::with_position(code) + let (mut db, file_id, range_or_offset) = if code.contains(test_utils::CURSOR_MARKER) { + ra_ide_db::RootDatabase::with_range_or_offset(code) } else { let (db, file_id) = ra_ide_db::RootDatabase::with_single_file(code); - (db, FilePosition { file_id, offset: 0.into() }) + (db, file_id, RangeOrOffset::Offset(0.into())) }; + let selections; + let position; + match range_or_offset { + RangeOrOffset::Range(range) => { + position = FilePosition { file_id, offset: range.start() }; + selections = vec![FileRange { file_id, range: range }]; + } + RangeOrOffset::Offset(offset) => { + position = FilePosition { file_id, offset }; + selections = vec![]; + } + } let mut local_roots = FxHashSet::default(); local_roots.insert(ra_db::fixture::WORKSPACE); db.set_local_roots_with_durability(Arc::new(local_roots), Durability::HIGH); - (db, position) + (db, position, selections) } fn assert_ssr_transform(rule: &str, input: &str, expected: Expect) { @@ -81,8 +93,8 @@ fn assert_ssr_transform(rule: &str, input: &str, expected: Expect) { } fn assert_ssr_transforms(rules: &[&str], input: &str, expected: Expect) { - let (db, position) = single_file(input); - let mut match_finder = MatchFinder::in_context(&db, position); + let (db, position, selections) = single_file(input); + let mut match_finder = MatchFinder::in_context(&db, position, selections); for rule in rules { let rule: SsrRule = rule.parse().unwrap(); match_finder.add_rule(rule).unwrap(); @@ -112,8 +124,8 @@ fn print_match_debug_info(match_finder: &MatchFinder, file_id: FileId, snippet: } fn assert_matches(pattern: &str, code: &str, expected: &[&str]) { - let (db, position) = single_file(code); - let mut match_finder = MatchFinder::in_context(&db, position); + let (db, position, selections) = single_file(code); + let mut match_finder = MatchFinder::in_context(&db, position, selections); match_finder.add_search_pattern(pattern.parse().unwrap()).unwrap(); let matched_strings: Vec = match_finder.matches().flattened().matches.iter().map(|m| m.matched_text()).collect(); @@ -124,8 +136,8 @@ fn assert_matches(pattern: &str, code: &str, expected: &[&str]) { } fn assert_no_match(pattern: &str, code: &str) { - let (db, position) = single_file(code); - let mut match_finder = MatchFinder::in_context(&db, position); + let (db, position, selections) = single_file(code); + let mut match_finder = MatchFinder::in_context(&db, position, selections); match_finder.add_search_pattern(pattern.parse().unwrap()).unwrap(); let matches = match_finder.matches().flattened().matches; if !matches.is_empty() { @@ -135,8 +147,8 @@ fn assert_no_match(pattern: &str, code: &str) { } fn assert_match_failure_reason(pattern: &str, code: &str, snippet: &str, expected_reason: &str) { - let (db, position) = single_file(code); - let mut match_finder = MatchFinder::in_context(&db, position); + let (db, position, selections) = single_file(code); + let mut match_finder = MatchFinder::in_context(&db, position, selections); match_finder.add_search_pattern(pattern.parse().unwrap()).unwrap(); let mut reasons = Vec::new(); for d in match_finder.debug_where_text_equal(position.file_id, snippet) { @@ -490,9 +502,10 @@ fn no_match_split_expression() { #[test] fn replace_function_call() { + // This test also makes sure that we ignore empty-ranges. assert_ssr_transform( "foo() ==>> bar()", - "fn foo() {} fn bar() {} fn f1() {foo(); foo();}", + "fn foo() {<|><|>} fn bar() {} fn f1() {foo(); foo();}", expect![["fn foo() {} fn bar() {} fn f1() {bar(); bar();}"]], ); } @@ -922,3 +935,52 @@ fn replace_local_variable_reference() { "#]], ) } + +#[test] +fn replace_path_within_selection() { + assert_ssr_transform( + "foo ==>> bar", + r#" + fn main() { + let foo = 41; + let bar = 42; + do_stuff(foo); + do_stuff(foo);<|> + do_stuff(foo); + do_stuff(foo);<|> + do_stuff(foo); + }"#, + expect![[r#" + fn main() { + let foo = 41; + let bar = 42; + do_stuff(foo); + do_stuff(foo); + do_stuff(bar); + do_stuff(bar); + do_stuff(foo); + }"#]], + ); +} + +#[test] +fn replace_nonpath_within_selection() { + mark::check!(replace_nonpath_within_selection); + assert_ssr_transform( + "$a + $b ==>> $b * $a", + r#" + fn main() { + let v = 1 + 2;<|> + let v2 = 3 + 3; + let v3 = 4 + 5;<|> + let v4 = 6 + 7; + }"#, + expect![[r#" + fn main() { + let v = 1 + 2; + let v2 = 3 * 3; + let v3 = 5 * 4; + let v4 = 6 + 7; + }"#]], + ); +} diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index cd309ed74..1350bd400 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -1026,9 +1026,18 @@ pub(crate) fn handle_ssr( params: lsp_ext::SsrParams, ) -> Result { let _p = profile("handle_ssr"); + let selections = params + .selections + .iter() + .map(|range| from_proto::file_range(&snap, params.position.text_document.clone(), *range)) + .collect::, _>>()?; let position = from_proto::file_position(&snap, params.position)?; - let source_change = - snap.analysis.structural_search_replace(¶ms.query, params.parse_only, position)??; + let source_change = snap.analysis.structural_search_replace( + ¶ms.query, + params.parse_only, + position, + selections, + )??; to_proto::workspace_edit(&snap, source_change) } diff --git a/crates/rust-analyzer/src/lsp_ext.rs b/crates/rust-analyzer/src/lsp_ext.rs index 113e0e070..3976b6529 100644 --- a/crates/rust-analyzer/src/lsp_ext.rs +++ b/crates/rust-analyzer/src/lsp_ext.rs @@ -221,6 +221,9 @@ pub struct SsrParams { /// position. #[serde(flatten)] pub position: lsp_types::TextDocumentPositionParams, + + /// Current selections. Search/replace will be restricted to these if non-empty. + pub selections: Vec, } pub enum StatusNotification {} -- cgit v1.2.3