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_ssr/src/tests.rs | 96 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 79 insertions(+), 17 deletions(-) (limited to 'crates/ra_ssr/src/tests.rs') 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; + }"#]], + ); +} -- cgit v1.2.3 From 3600c43f49f9901ffc94a139a8a3655944e91e4e Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 29 Jul 2020 16:01:00 +1000 Subject: SSR: Don't mix non-path-based rules with path-based If any rules contain paths, then we reject any rules that don't contain paths. Allowing a mix leads to strange semantics, since the path-based rules only match things where the path refers to semantically the same thing, whereas the non-path-based rules could match anything. Specifically, if we have a rule like `foo ==>> bar` we only want to match the `foo` that is in the current scope, not any `foo`. However "foo" can be parsed as a pattern (BIND_PAT -> NAME -> IDENT). Allowing such a rule through would result in renaming everything called `foo` to `bar`. It'd also be slow, since without a path, we'd have to use the slow-scan search mechanism. --- crates/ra_ssr/src/tests.rs | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) (limited to 'crates/ra_ssr/src/tests.rs') diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 18ef2506a..851e573ae 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -886,6 +886,45 @@ fn ufcs_matches_method_call() { ); } +#[test] +fn pattern_is_a_single_segment_path() { + mark::check!(pattern_is_a_single_segment_path); + // The first function should not be altered because the `foo` in scope at the cursor position is + // a different `foo`. This case is special because "foo" can be parsed as a pattern (BIND_PAT -> + // NAME -> IDENT), which contains no path. If we're not careful we'll end up matching the `foo` + // in `let foo` from the first function. Whether we should match the `let foo` in the second + // function is less clear. At the moment, we don't. Doing so sounds like a rename operation, + // which isn't really what SSR is for, especially since the replacement `bar` must be able to be + // resolved, which means if we rename `foo` we'll get a name collision. + assert_ssr_transform( + "foo ==>> bar", + r#" + fn f1() -> i32 { + let foo = 1; + let bar = 2; + foo + } + fn f1() -> i32 { + let foo = 1; + let bar = 2; + foo<|> + } + "#, + expect![[r#" + fn f1() -> i32 { + let foo = 1; + let bar = 2; + foo + } + fn f1() -> i32 { + let foo = 1; + let bar = 2; + bar + } + "#]], + ); +} + #[test] fn replace_local_variable_reference() { // The pattern references a local variable `foo` in the block containing the cursor. We should -- cgit v1.2.3 From fa1e4113224c119258670538f8c3ca6c8ea8ad1e Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 29 Jul 2020 21:23:39 +1000 Subject: SSR: Wrap placeholder expansions in parenthesis when necessary e.g. `foo($a) ==> $a.to_string()` should produce `(1 + 2).to_string()` not `1 + 2.to_string()` We don't yet try to determine if the whole replacement needs to be wrapped in parenthesis. That's harder and I think perhaps less often an issue. --- crates/ra_ssr/src/tests.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) (limited to 'crates/ra_ssr/src/tests.rs') diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index f5ffff7cc..a4fa2cb44 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -664,7 +664,7 @@ fn replace_binary_op() { assert_ssr_transform( "$a + $b ==>> $b + $a", "fn f() {1 + 2 + 3 + 4}", - expect![["fn f() {4 + 3 + 2 + 1}"]], + expect![[r#"fn f() {4 + (3 + (2 + 1))}"#]], ); } @@ -773,11 +773,32 @@ fn preserves_whitespace_within_macro_expansion() { macro_rules! macro1 { ($a:expr) => {$a} } - fn f() {macro1!(4 - 3 - 1 * 2} + fn f() {macro1!(4 - (3 - 1 * 2)} "#]], ) } +#[test] +fn add_parenthesis_when_necessary() { + assert_ssr_transform( + "foo($a) ==>> $a.to_string()", + r#" + fn foo(_: i32) {} + fn bar3(v: i32) { + foo(1 + 2); + foo(-v); + } + "#, + expect![[r#" + fn foo(_: i32) {} + fn bar3(v: i32) { + (1 + 2).to_string(); + (-v).to_string(); + } + "#]], + ) +} + #[test] fn match_failure_reasons() { let code = r#" -- cgit v1.2.3 From 98181087984157e27faba0b969e384f3c62c39d5 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 31 Jul 2020 20:09:09 +0200 Subject: Rename BindPat -> IdentPat --- crates/ra_ssr/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'crates/ra_ssr/src/tests.rs') diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index a4fa2cb44..2ae03c64c 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -924,7 +924,7 @@ fn ufcs_matches_method_call() { fn pattern_is_a_single_segment_path() { mark::check!(pattern_is_a_single_segment_path); // The first function should not be altered because the `foo` in scope at the cursor position is - // a different `foo`. This case is special because "foo" can be parsed as a pattern (BIND_PAT -> + // a different `foo`. This case is special because "foo" can be parsed as a pattern (IDENT_PAT -> // NAME -> IDENT), which contains no path. If we're not careful we'll end up matching the `foo` // in `let foo` from the first function. Whether we should match the `let foo` in the second // function is less clear. At the moment, we don't. Doing so sounds like a rename operation, -- cgit v1.2.3 From 6bbeffc8c56893548f5667844f59ce5a76f9fd98 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Sat, 1 Aug 2020 17:41:42 +1000 Subject: SSR: Allow `self` in patterns. It's now consistent with other variables in that if the pattern references self, only the `self` in scope where the rule is invoked will be accepted. Since `self` doesn't work the same as other paths, this is implemented by restricting the search to just the current function. Prior to this change (since path resolution was implemented), having self in a pattern would just result in no matches. --- crates/ra_ssr/src/tests.rs | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) (limited to 'crates/ra_ssr/src/tests.rs') diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 2ae03c64c..d483640df 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -1044,3 +1044,38 @@ fn replace_nonpath_within_selection() { }"#]], ); } + +#[test] +fn replace_self() { + // `foo(self)` occurs twice in the code, however only the first occurrence is the `self` that's + // in scope where the rule is invoked. + assert_ssr_transform( + "foo(self) ==>> bar(self)", + r#" + struct S1 {} + fn foo(_: &S1) {} + fn bar(_: &S1) {} + impl S1 { + fn f1(&self) { + foo(self)<|> + } + fn f2(&self) { + foo(self) + } + } + "#, + expect![[r#" + struct S1 {} + fn foo(_: &S1) {} + fn bar(_: &S1) {} + impl S1 { + fn f1(&self) { + bar(self) + } + fn f2(&self) { + foo(self) + } + } + "#]], + ); +} -- cgit v1.2.3