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/lib.rs | 11 ++++- crates/ra_ssr/src/matching.rs | 4 +- crates/ra_ssr/src/search.rs | 87 ++++++++++++++++++++++++++++----------- crates/ra_ssr/src/tests.rs | 96 +++++++++++++++++++++++++++++++++++-------- 4 files changed, 155 insertions(+), 43 deletions(-) (limited to 'crates/ra_ssr/src') 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; + }"#]], + ); +} -- 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/parsing.rs | 24 +++++++++++++++++++++++- crates/ra_ssr/src/tests.rs | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/parsing.rs b/crates/ra_ssr/src/parsing.rs index 2d6f4e514..769720bef 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -10,6 +10,7 @@ use crate::{SsrError, SsrPattern, SsrRule}; use ra_syntax::{ast, AstNode, SmolStr, SyntaxKind, SyntaxNode, T}; use rustc_hash::{FxHashMap, FxHashSet}; use std::str::FromStr; +use test_utils::mark; #[derive(Debug)] pub(crate) struct ParsedRule { @@ -102,14 +103,35 @@ impl RuleBuilder { } } - fn build(self) -> Result, SsrError> { + fn build(mut self) -> Result, SsrError> { if self.rules.is_empty() { bail!("Not a valid Rust expression, type, item, path or pattern"); } + // 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. + if self.rules.iter().any(|rule| contains_path(&rule.pattern)) { + let old_len = self.rules.len(); + self.rules.retain(|rule| contains_path(&rule.pattern)); + if self.rules.len() < old_len { + mark::hit!(pattern_is_a_single_segment_path); + } + } Ok(self.rules) } } +/// Returns whether there are any paths in `node`. +fn contains_path(node: &SyntaxNode) -> bool { + node.kind() == SyntaxKind::PATH + || node.descendants().any(|node| node.kind() == SyntaxKind::PATH) +} + impl FromStr for SsrRule { type Err = SsrError; 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 fcb6b166fbc506950dc2689adfa4d0b728d1a745 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 29 Jul 2020 19:20:40 +1000 Subject: SSR: Rename position and lookup_context to resolve_context --- crates/ra_ssr/src/resolving.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/resolving.rs b/crates/ra_ssr/src/resolving.rs index 123bd2bb2..78d456546 100644 --- a/crates/ra_ssr/src/resolving.rs +++ b/crates/ra_ssr/src/resolving.rs @@ -141,14 +141,14 @@ impl Resolver<'_, '_> { impl<'db> ResolutionScope<'db> { pub(crate) fn new( sema: &hir::Semantics<'db, ra_ide_db::RootDatabase>, - lookup_context: FilePosition, + resolve_context: FilePosition, ) -> ResolutionScope<'db> { use ra_syntax::ast::AstNode; - let file = sema.parse(lookup_context.file_id); + let file = sema.parse(resolve_context.file_id); // Find a node at the requested position, falling back to the whole file. let node = file .syntax() - .token_at_offset(lookup_context.offset) + .token_at_offset(resolve_context.offset) .left_biased() .map(|token| token.parent()) .unwrap_or_else(|| file.syntax().clone()); @@ -156,7 +156,7 @@ impl<'db> ResolutionScope<'db> { let scope = sema.scope(&node); ResolutionScope { scope, - hygiene: hir::Hygiene::new(sema.db, lookup_context.file_id.into()), + hygiene: hir::Hygiene::new(sema.db, resolve_context.file_id.into()), } } -- cgit v1.2.3 From 6636f56e79b55f22b88094b7edaed6ec88880500 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Thu, 30 Jul 2020 00:23:03 +0200 Subject: Rename ModuleItem -> Item --- crates/ra_ssr/src/parsing.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/parsing.rs b/crates/ra_ssr/src/parsing.rs index 769720bef..78e03f394 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -71,10 +71,7 @@ impl ParsedRule { }; builder.try_add(ast::Expr::parse(&raw_pattern), raw_template.map(ast::Expr::parse)); builder.try_add(ast::TypeRef::parse(&raw_pattern), raw_template.map(ast::TypeRef::parse)); - builder.try_add( - ast::ModuleItem::parse(&raw_pattern), - raw_template.map(ast::ModuleItem::parse), - ); + builder.try_add(ast::Item::parse(&raw_pattern), raw_template.map(ast::Item::parse)); builder.try_add(ast::Path::parse(&raw_pattern), raw_template.map(ast::Path::parse)); builder.try_add(ast::Pat::parse(&raw_pattern), raw_template.map(ast::Pat::parse)); builder.build() -- 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/replacing.rs | 106 ++++++++++++++++++++++++++++++++++------- crates/ra_ssr/src/tests.rs | 25 +++++++++- 2 files changed, 111 insertions(+), 20 deletions(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/replacing.rs b/crates/ra_ssr/src/replacing.rs index 4b3f5509c..0943244ff 100644 --- a/crates/ra_ssr/src/replacing.rs +++ b/crates/ra_ssr/src/replacing.rs @@ -3,8 +3,9 @@ use crate::matching::Var; use crate::{resolving::ResolvedRule, Match, SsrMatches}; use ra_syntax::ast::{self, AstToken}; -use ra_syntax::{SyntaxElement, SyntaxKind, SyntaxNode, SyntaxToken, TextSize}; +use ra_syntax::{SyntaxElement, SyntaxKind, SyntaxNode, SyntaxToken, TextRange, TextSize}; use ra_text_edit::TextEdit; +use rustc_hash::{FxHashMap, FxHashSet}; /// Returns a text edit that will replace each match in `matches` with its corresponding replacement /// template. Placeholders in the template will have been substituted with whatever they matched to @@ -38,62 +39,79 @@ struct ReplacementRenderer<'a> { file_src: &'a str, rules: &'a [ResolvedRule], rule: &'a ResolvedRule, + out: String, + // Map from a range within `out` to a token in `template` that represents a placeholder. This is + // used to validate that the generated source code doesn't split any placeholder expansions (see + // below). + placeholder_tokens_by_range: FxHashMap, + // Which placeholder tokens need to be wrapped in parenthesis in order to ensure that when `out` + // is parsed, placeholders don't get split. e.g. if a template of `$a.to_string()` results in `1 + // + 2.to_string()` then the placeholder value `1 + 2` was split and needs parenthesis. + placeholder_tokens_requiring_parenthesis: FxHashSet, } fn render_replace(match_info: &Match, file_src: &str, rules: &[ResolvedRule]) -> String { - let mut out = String::new(); let rule = &rules[match_info.rule_index]; let template = rule .template .as_ref() .expect("You called MatchFinder::edits after calling MatchFinder::add_search_pattern"); - let renderer = ReplacementRenderer { match_info, file_src, rules, rule }; - renderer.render_node(&template.node, &mut out); + let mut renderer = ReplacementRenderer { + match_info, + file_src, + rules, + rule, + out: String::new(), + placeholder_tokens_requiring_parenthesis: FxHashSet::default(), + placeholder_tokens_by_range: FxHashMap::default(), + }; + renderer.render_node(&template.node); + renderer.maybe_rerender_with_extra_parenthesis(&template.node); for comment in &match_info.ignored_comments { - out.push_str(&comment.syntax().to_string()); + renderer.out.push_str(&comment.syntax().to_string()); } - out + renderer.out } impl ReplacementRenderer<'_> { - fn render_node_children(&self, node: &SyntaxNode, out: &mut String) { + fn render_node_children(&mut self, node: &SyntaxNode) { for node_or_token in node.children_with_tokens() { - self.render_node_or_token(&node_or_token, out); + self.render_node_or_token(&node_or_token); } } - fn render_node_or_token(&self, node_or_token: &SyntaxElement, out: &mut String) { + fn render_node_or_token(&mut self, node_or_token: &SyntaxElement) { match node_or_token { SyntaxElement::Token(token) => { - self.render_token(&token, out); + self.render_token(&token); } SyntaxElement::Node(child_node) => { - self.render_node(&child_node, out); + self.render_node(&child_node); } } } - fn render_node(&self, node: &SyntaxNode, out: &mut String) { + fn render_node(&mut self, node: &SyntaxNode) { use ra_syntax::ast::AstNode; if let Some(mod_path) = self.match_info.rendered_template_paths.get(&node) { - out.push_str(&mod_path.to_string()); + self.out.push_str(&mod_path.to_string()); // Emit everything except for the segment's name-ref, since we already effectively // emitted that as part of `mod_path`. if let Some(path) = ast::Path::cast(node.clone()) { if let Some(segment) = path.segment() { for node_or_token in segment.syntax().children_with_tokens() { if node_or_token.kind() != SyntaxKind::NAME_REF { - self.render_node_or_token(&node_or_token, out); + self.render_node_or_token(&node_or_token); } } } } } else { - self.render_node_children(&node, out); + self.render_node_children(&node); } } - fn render_token(&self, token: &SyntaxToken, out: &mut String) { + fn render_token(&mut self, token: &SyntaxToken) { if let Some(placeholder) = self.rule.get_placeholder(&token) { if let Some(placeholder_value) = self.match_info.placeholder_values.get(&Var(placeholder.ident.to_string())) @@ -107,8 +125,23 @@ impl ReplacementRenderer<'_> { range.start(), self.rules, ); + let needs_parenthesis = + self.placeholder_tokens_requiring_parenthesis.contains(token); edit.apply(&mut matched_text); - out.push_str(&matched_text); + if needs_parenthesis { + self.out.push('('); + } + self.placeholder_tokens_by_range.insert( + TextRange::new( + TextSize::of(&self.out), + TextSize::of(&self.out) + TextSize::of(&matched_text), + ), + token.clone(), + ); + self.out.push_str(&matched_text); + if needs_parenthesis { + self.out.push(')'); + } } else { // We validated that all placeholder references were valid before we // started, so this shouldn't happen. @@ -118,7 +151,44 @@ impl ReplacementRenderer<'_> { ); } } else { - out.push_str(token.text().as_str()); + self.out.push_str(token.text().as_str()); + } + } + + // Checks if the resulting code, when parsed doesn't split any placeholders due to different + // order of operations between the search pattern and the replacement template. If any do, then + // we rerender the template and wrap the problematic placeholders with parenthesis. + fn maybe_rerender_with_extra_parenthesis(&mut self, template: &SyntaxNode) { + if let Some(node) = parse_as_kind(&self.out, template.kind()) { + self.remove_node_ranges(node); + if self.placeholder_tokens_by_range.is_empty() { + return; + } + self.placeholder_tokens_requiring_parenthesis = + self.placeholder_tokens_by_range.values().cloned().collect(); + self.out.clear(); + self.render_node(template); + } + } + + fn remove_node_ranges(&mut self, node: SyntaxNode) { + self.placeholder_tokens_by_range.remove(&node.text_range()); + for child in node.children() { + self.remove_node_ranges(child); + } + } +} + +fn parse_as_kind(code: &str, kind: SyntaxKind) -> Option { + use ra_syntax::ast::AstNode; + if ast::Expr::can_cast(kind) { + if let Ok(expr) = ast::Expr::parse(code) { + return Some(expr.syntax().clone()); + } + } else if ast::Item::can_cast(kind) { + if let Ok(item) = ast::Item::parse(code) { + return Some(item.syntax().clone()); } } + None } 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 9697d23cbe7b0ad897139b15f1b1ffe1cab6ad89 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Thu, 30 Jul 2020 14:12:04 +0200 Subject: Rename UseItem -> Use --- crates/ra_ssr/src/search.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index 0f512cb62..213dc494f 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -237,7 +237,7 @@ fn is_search_permitted(node: &SyntaxNode) -> bool { // and the code is `use foo::{baz, bar}`, we'll match `bar`, since it resolves to `foo::bar`. // However we'll then replace just the part we matched `bar`. We probably need to instead remove // `bar` and insert a new use declaration. - node.kind() != SyntaxKind::USE_ITEM + node.kind() != SyntaxKind::USE } impl UsageCache { -- cgit v1.2.3 From 6f8aa75329d0a4e588e58b8f22f7932bf3d3a706 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Thu, 30 Jul 2020 16:21:30 +0200 Subject: Rename RecordLit -> RecordExpr --- crates/ra_ssr/src/matching.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index c1b66748e..74e15c631 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -209,7 +209,7 @@ impl<'db, 'sema> Matcher<'db, 'sema> { // Some kinds of nodes have special handling. For everything else, we fall back to default // matching. match code.kind() { - SyntaxKind::RECORD_FIELD_LIST => { + SyntaxKind::RECORD_EXPR_FIELD_LIST => { self.attempt_match_record_field_list(phase, pattern, code) } SyntaxKind::TOKEN_TREE => self.attempt_match_token_tree(phase, pattern, code), @@ -399,7 +399,7 @@ impl<'db, 'sema> Matcher<'db, 'sema> { // Build a map keyed by field name. let mut fields_by_name = FxHashMap::default(); for child in code.children() { - if let Some(record) = ast::RecordField::cast(child.clone()) { + if let Some(record) = ast::RecordExprField::cast(child.clone()) { if let Some(name) = record.field_name() { fields_by_name.insert(name.text().clone(), child.clone()); } -- cgit v1.2.3 From 08ea2271e8050165d0aaf4c994ed3dd746aff3ba Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 31 Jul 2020 12:06:38 +0200 Subject: Rename TypeRef -> Type The TypeRef name comes from IntelliJ days, where you often have both type *syntax* as well as *semantical* representation of types in scope. And naming both Type is confusing. In rust-analyzer however, we use ast types as `ast::Type`, and have many more semantic counterparts to ast types, so avoiding name clash here is just confusing. --- crates/ra_ssr/src/parsing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/parsing.rs b/crates/ra_ssr/src/parsing.rs index 78e03f394..4e046910c 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -70,7 +70,7 @@ impl ParsedRule { rules: Vec::new(), }; builder.try_add(ast::Expr::parse(&raw_pattern), raw_template.map(ast::Expr::parse)); - builder.try_add(ast::TypeRef::parse(&raw_pattern), raw_template.map(ast::TypeRef::parse)); + builder.try_add(ast::Type::parse(&raw_pattern), raw_template.map(ast::Type::parse)); builder.try_add(ast::Item::parse(&raw_pattern), raw_template.map(ast::Item::parse)); builder.try_add(ast::Path::parse(&raw_pattern), raw_template.map(ast::Path::parse)); builder.try_add(ast::Pat::parse(&raw_pattern), raw_template.map(ast::Pat::parse)); -- cgit v1.2.3 From 91781c7ce8201b28afd56b4e35eba47e076a8498 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 31 Jul 2020 18:29:29 +0200 Subject: Rename TypeArgList -> GenericArgList --- crates/ra_ssr/src/matching.rs | 4 ++-- crates/ra_ssr/src/resolving.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index 74e15c631..0f72fea69 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -348,8 +348,8 @@ impl<'db, 'sema> Matcher<'db, 'sema> { // separately via comparing what the path resolves to below. self.attempt_match_opt( phase, - pattern_segment.type_arg_list(), - code_segment.type_arg_list(), + pattern_segment.generic_arg_list(), + code_segment.generic_arg_list(), )?; self.attempt_match_opt( phase, diff --git a/crates/ra_ssr/src/resolving.rs b/crates/ra_ssr/src/resolving.rs index 78d456546..c2fd3b905 100644 --- a/crates/ra_ssr/src/resolving.rs +++ b/crates/ra_ssr/src/resolving.rs @@ -217,7 +217,7 @@ fn pick_node_for_resolution(node: SyntaxNode) -> SyntaxNode { fn path_contains_type_arguments(path: Option) -> bool { if let Some(path) = path { if let Some(segment) = path.segment() { - if segment.type_arg_list().is_some() { + if segment.generic_arg_list().is_some() { mark::hit!(type_arguments_within_path); return true; } -- 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/parsing.rs | 2 +- crates/ra_ssr/src/resolving.rs | 2 +- crates/ra_ssr/src/tests.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/parsing.rs b/crates/ra_ssr/src/parsing.rs index 4e046910c..f455eb5b7 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -109,7 +109,7 @@ impl RuleBuilder { // 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 + // pattern (IDENT_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. if self.rules.iter().any(|rule| contains_path(&rule.pattern)) { diff --git a/crates/ra_ssr/src/resolving.rs b/crates/ra_ssr/src/resolving.rs index c2fd3b905..6f62000f4 100644 --- a/crates/ra_ssr/src/resolving.rs +++ b/crates/ra_ssr/src/resolving.rs @@ -198,7 +198,7 @@ fn pick_node_for_resolution(node: SyntaxNode) -> SyntaxNode { return n; } } - SyntaxKind::LET_STMT | SyntaxKind::BIND_PAT => { + SyntaxKind::LET_STMT | SyntaxKind::IDENT_PAT => { if let Some(next) = node.next_sibling() { return pick_node_for_resolution(next); } 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/lib.rs | 7 +------ crates/ra_ssr/src/resolving.rs | 29 +++++++++++++++++++++++++++++ crates/ra_ssr/src/search.rs | 9 +++++++++ crates/ra_ssr/src/tests.rs | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 6 deletions(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index 73abfecb2..c780b460a 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -66,12 +66,7 @@ impl<'db> 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, - restrict_ranges, - } + MatchFinder { sema, rules: Vec::new(), resolution_scope, restrict_ranges } } /// Constructs an instance using the start of the first file in `db` as the lookup context. diff --git a/crates/ra_ssr/src/resolving.rs b/crates/ra_ssr/src/resolving.rs index 6f62000f4..0ac929bd5 100644 --- a/crates/ra_ssr/src/resolving.rs +++ b/crates/ra_ssr/src/resolving.rs @@ -11,6 +11,7 @@ use test_utils::mark; pub(crate) struct ResolutionScope<'db> { scope: hir::SemanticsScope<'db>, hygiene: hir::Hygiene, + node: SyntaxNode, } pub(crate) struct ResolvedRule { @@ -25,6 +26,7 @@ pub(crate) struct ResolvedPattern { // Paths in `node` that we've resolved. pub(crate) resolved_paths: FxHashMap, pub(crate) ufcs_function_calls: FxHashMap, + pub(crate) contains_self: bool, } pub(crate) struct ResolvedPath { @@ -68,6 +70,7 @@ struct Resolver<'a, 'db> { impl Resolver<'_, '_> { fn resolve_pattern_tree(&self, pattern: SyntaxNode) -> Result { + use ra_syntax::{SyntaxElement, T}; let mut resolved_paths = FxHashMap::default(); self.resolve(pattern.clone(), 0, &mut resolved_paths)?; let ufcs_function_calls = resolved_paths @@ -85,11 +88,17 @@ impl Resolver<'_, '_> { None }) .collect(); + let contains_self = + pattern.descendants_with_tokens().any(|node_or_token| match node_or_token { + SyntaxElement::Token(t) => t.kind() == T![self], + _ => false, + }); Ok(ResolvedPattern { node: pattern, resolved_paths, placeholders_by_stand_in: self.placeholders_by_stand_in.clone(), ufcs_function_calls, + contains_self, }) } @@ -101,6 +110,10 @@ impl Resolver<'_, '_> { ) -> Result<(), SsrError> { use ra_syntax::ast::AstNode; if let Some(path) = ast::Path::cast(node.clone()) { + if is_self(&path) { + // Self cannot be resolved like other paths. + return Ok(()); + } // Check if this is an appropriate place in the path to resolve. If the path is // something like `a::B::::c` then we want to resolve `a::B`. If the path contains // a placeholder. e.g. `a::$b::c` then we want to resolve `a`. @@ -157,6 +170,18 @@ impl<'db> ResolutionScope<'db> { ResolutionScope { scope, hygiene: hir::Hygiene::new(sema.db, resolve_context.file_id.into()), + node, + } + } + + /// Returns the function in which SSR was invoked, if any. + pub(crate) fn current_function(&self) -> Option { + let mut node = self.node.clone(); + loop { + if node.kind() == SyntaxKind::FN { + return Some(node); + } + node = node.parent()?; } } @@ -186,6 +211,10 @@ impl<'db> ResolutionScope<'db> { } } +fn is_self(path: &ast::Path) -> bool { + path.segment().map(|segment| segment.self_token().is_some()).unwrap_or(false) +} + /// Returns a suitable node for resolving paths in the current scope. If we create a scope based on /// a statement node, then we can't resolve local variables that were defined in the current scope /// (only in parent scopes). So we find another node, ideally a child of the statement where local diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index 213dc494f..85ffa2ac2 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -33,6 +33,15 @@ impl<'db> MatchFinder<'db> { usage_cache: &mut UsageCache, matches_out: &mut Vec, ) { + if rule.pattern.contains_self { + // If the pattern contains `self` we restrict the scope of the search to just the + // current method. No other method can reference the same `self`. This makes the + // behavior of `self` consistent with other variables. + if let Some(current_function) = self.resolution_scope.current_function() { + self.slow_scan_node(¤t_function, rule, &None, matches_out); + } + return; + } if pick_path_for_usages(&rule.pattern).is_none() { self.slow_scan(rule, matches_out); return; 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 From 3eea41a68ca2de28dca35b6e713cbb36aa09f0c8 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Thu, 6 Aug 2020 07:36:03 +1000 Subject: Use SyntaxNode.ancestors instead of a loop --- crates/ra_ssr/src/resolving.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) (limited to 'crates/ra_ssr/src') diff --git a/crates/ra_ssr/src/resolving.rs b/crates/ra_ssr/src/resolving.rs index 0ac929bd5..df60048eb 100644 --- a/crates/ra_ssr/src/resolving.rs +++ b/crates/ra_ssr/src/resolving.rs @@ -176,13 +176,7 @@ impl<'db> ResolutionScope<'db> { /// Returns the function in which SSR was invoked, if any. pub(crate) fn current_function(&self) -> Option { - let mut node = self.node.clone(); - loop { - if node.kind() == SyntaxKind::FN { - return Some(node); - } - node = node.parent()?; - } + self.node.ancestors().find(|node| node.kind() == SyntaxKind::FN).map(|node| node.clone()) } fn resolve_path(&self, path: &ast::Path) -> Option { -- cgit v1.2.3