From 2b53639e381b1f17c829fb33f6e4135a9c930f41 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Tue, 21 Jul 2020 21:32:09 +1000 Subject: [PATCH 01/14] SSR: Use expect! in tests --- Cargo.lock | 1 + crates/ra_ssr/Cargo.toml | 3 ++ crates/ra_ssr/src/tests.rs | 71 ++++++++++++++++++-------------------- 3 files changed, 38 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0a35eb7932..8bed2b1afc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1164,6 +1164,7 @@ dependencies = [ name = "ra_ssr" version = "0.1.0" dependencies = [ + "expect", "ra_db", "ra_hir", "ra_ide_db", diff --git a/crates/ra_ssr/Cargo.toml b/crates/ra_ssr/Cargo.toml index fe098aaee3..84e4b171e1 100644 --- a/crates/ra_ssr/Cargo.toml +++ b/crates/ra_ssr/Cargo.toml @@ -18,3 +18,6 @@ ra_ide_db = { path = "../ra_ide_db" } hir = { path = "../ra_hir", package = "ra_hir" } rustc-hash = "1.1.0" test_utils = { path = "../test_utils" } + +[dev-dependencies] +expect = { path = "../expect" } diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index f20ae2cdf9..9f53065929 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -1,4 +1,5 @@ use crate::{MatchFinder, SsrRule}; +use expect::{expect, Expect}; use ra_db::{FileId, SourceDatabaseExt}; use test_utils::mark; @@ -61,16 +62,11 @@ fn single_file(code: &str) -> (ra_ide_db::RootDatabase, FileId) { ra_ide_db::RootDatabase::with_single_file(code) } -fn assert_ssr_transform(rule: &str, input: &str, result: &str) { - assert_ssr_transforms(&[rule], input, result); +fn assert_ssr_transform(rule: &str, input: &str, expected: Expect) { + assert_ssr_transforms(&[rule], input, expected); } -fn normalize_code(code: &str) -> String { - let (db, file_id) = single_file(code); - db.file_text(file_id).to_string() -} - -fn assert_ssr_transforms(rules: &[&str], input: &str, result: &str) { +fn assert_ssr_transforms(rules: &[&str], input: &str, expected: Expect) { let (db, file_id) = single_file(input); let mut match_finder = MatchFinder::new(&db); for rule in rules { @@ -80,12 +76,9 @@ fn assert_ssr_transforms(rules: &[&str], input: &str, result: &str) { if let Some(edits) = match_finder.edits_for_file(file_id) { // Note, db.file_text is not necessarily the same as `input`, since fixture parsing alters // stuff. - let mut after = db.file_text(file_id).to_string(); - edits.apply(&mut after); - // Likewise, we need to make sure that whatever transformations fixture parsing applies, - // also get applied to our expected result. - let result = normalize_code(result); - assert_eq!(after, result); + let mut actual = db.file_text(file_id).to_string(); + edits.apply(&mut actual); + expected.assert_eq(&actual); } else { panic!("No edits were made"); } @@ -149,7 +142,7 @@ fn ssr_function_to_method() { assert_ssr_transform( "my_function($a, $b) ==>> ($a).my_method($b)", "fn my_function() {} fn main() { loop { my_function( other_func(x, y), z + w) } }", - "fn my_function() {} fn main() { loop { (other_func(x, y)).my_method(z + w) } }", + expect![["fn my_function() {} fn main() { loop { (other_func(x, y)).my_method(z + w) } }"]], ) } @@ -158,7 +151,7 @@ fn ssr_nested_function() { assert_ssr_transform( "foo($a, $b, $c) ==>> bar($c, baz($a, $b))", "fn foo() {} fn main { foo (x + value.method(b), x+y-z, true && false) }", - "fn foo() {} fn main { bar(true && false, baz(x + value.method(b), x+y-z)) }", + expect![["fn foo() {} fn main { bar(true && false, baz(x + value.method(b), x+y-z)) }"]], ) } @@ -167,7 +160,7 @@ fn ssr_expected_spacing() { assert_ssr_transform( "foo($x) + bar() ==>> bar($x)", "fn foo() {} fn bar() {} fn main() { foo(5) + bar() }", - "fn foo() {} fn bar() {} fn main() { bar(5) }", + expect![["fn foo() {} fn bar() {} fn main() { bar(5) }"]], ); } @@ -176,7 +169,7 @@ fn ssr_with_extra_space() { assert_ssr_transform( "foo($x ) + bar() ==>> bar($x)", "fn foo() {} fn bar() {} fn main() { foo( 5 ) +bar( ) }", - "fn foo() {} fn bar() {} fn main() { bar(5) }", + expect![["fn foo() {} fn bar() {} fn main() { bar(5) }"]], ); } @@ -185,7 +178,7 @@ fn ssr_keeps_nested_comment() { assert_ssr_transform( "foo($x) ==>> bar($x)", "fn foo() {} fn main() { foo(other(5 /* using 5 */)) }", - "fn foo() {} fn main() { bar(other(5 /* using 5 */)) }", + expect![["fn foo() {} fn main() { bar(other(5 /* using 5 */)) }"]], ) } @@ -194,7 +187,7 @@ fn ssr_keeps_comment() { assert_ssr_transform( "foo($x) ==>> bar($x)", "fn foo() {} fn main() { foo(5 /* using 5 */) }", - "fn foo() {} fn main() { bar(5)/* using 5 */ }", + expect![["fn foo() {} fn main() { bar(5)/* using 5 */ }"]], ) } @@ -203,7 +196,7 @@ fn ssr_struct_lit() { assert_ssr_transform( "foo{a: $a, b: $b} ==>> foo::new($a, $b)", "fn foo() {} fn main() { foo{b:2, a:1} }", - "fn foo() {} fn main() { foo::new(1, 2) }", + expect![["fn foo() {} fn main() { foo::new(1, 2) }"]], ) } @@ -417,7 +410,7 @@ fn replace_function_call() { assert_ssr_transform( "foo() ==>> bar()", "fn foo() {} fn f1() {foo(); foo();}", - "fn foo() {} fn f1() {bar(); bar();}", + expect![["fn foo() {} fn f1() {bar(); bar();}"]], ); } @@ -426,7 +419,7 @@ fn replace_function_call_with_placeholders() { assert_ssr_transform( "foo($a, $b) ==>> bar($b, $a)", "fn foo() {} fn f1() {foo(5, 42)}", - "fn foo() {} fn f1() {bar(42, 5)}", + expect![["fn foo() {} fn f1() {bar(42, 5)}"]], ); } @@ -435,7 +428,7 @@ fn replace_nested_function_calls() { assert_ssr_transform( "foo($a) ==>> bar($a)", "fn foo() {} fn f1() {foo(foo(42))}", - "fn foo() {} fn f1() {bar(bar(42))}", + expect![["fn foo() {} fn f1() {bar(bar(42))}"]], ); } @@ -444,7 +437,7 @@ fn replace_type() { assert_ssr_transform( "Result<(), $a> ==>> Option<$a>", "struct Result {} fn f1() -> Result<(), Vec> {foo()}", - "struct Result {} fn f1() -> Option> {foo()}", + expect![["struct Result {} fn f1() -> Option> {foo()}"]], ); } @@ -453,7 +446,7 @@ fn replace_struct_init() { assert_ssr_transform( "Foo {a: $a, b: $b} ==>> Foo::new($a, $b)", "struct Foo {} fn f1() {Foo{b: 1, a: 2}}", - "struct Foo {} fn f1() {Foo::new(2, 1)}", + expect![["struct Foo {} fn f1() {Foo::new(2, 1)}"]], ); } @@ -462,12 +455,12 @@ fn replace_macro_invocations() { assert_ssr_transform( "try!($a) ==>> $a?", "macro_rules! try {() => {}} fn f1() -> Result<(), E> {bar(try!(foo()));}", - "macro_rules! try {() => {}} fn f1() -> Result<(), E> {bar(foo()?);}", + expect![["macro_rules! try {() => {}} fn f1() -> Result<(), E> {bar(foo()?);}"]], ); assert_ssr_transform( "foo!($a($b)) ==>> foo($b, $a)", "macro_rules! foo {() => {}} fn f1() {foo!(abc(def() + 2));}", - "macro_rules! foo {() => {}} fn f1() {foo(def() + 2, abc);}", + expect![["macro_rules! foo {() => {}} fn f1() {foo(def() + 2, abc);}"]], ); } @@ -476,12 +469,12 @@ fn replace_binary_op() { assert_ssr_transform( "$a + $b ==>> $b + $a", "fn f() {2 * 3 + 4 * 5}", - "fn f() {4 * 5 + 2 * 3}", + expect![["fn f() {4 * 5 + 2 * 3}"]], ); assert_ssr_transform( "$a + $b ==>> $b + $a", "fn f() {1 + 2 + 3 + 4}", - "fn f() {4 + 3 + 2 + 1}", + expect![["fn f() {4 + 3 + 2 + 1}"]], ); } @@ -495,7 +488,7 @@ fn multiple_rules() { assert_ssr_transforms( &["$a + 1 ==>> add_one($a)", "$a + $b ==>> add($a, $b)"], "fn f() -> i32 {3 + 2 + 1}", - "fn f() -> i32 {add_one(add(3, 2))}", + expect![["fn f() -> i32 {add_one(add(3, 2))}"]], ) } @@ -527,12 +520,14 @@ fn replace_within_macro_expansion() { macro_rules! macro1 { ($a:expr) => {$a} } - fn f() {macro1!(5.x().foo().o2())}"#, - r#" + fn f() {macro1!(5.x().foo().o2())} + "#, + expect![[r#" macro_rules! macro1 { ($a:expr) => {$a} } - fn f() {macro1!(bar(5.x()).o2())}"#, + fn f() {macro1!(bar(5.x()).o2())} + "#]], ) } @@ -544,12 +539,14 @@ fn preserves_whitespace_within_macro_expansion() { macro_rules! macro1 { ($a:expr) => {$a} } - fn f() {macro1!(1 * 2 + 3 + 4}"#, - r#" + fn f() {macro1!(1 * 2 + 3 + 4} + "#, + expect![[r#" macro_rules! macro1 { ($a:expr) => {$a} } - fn f() {macro1!(4 - 3 - 1 * 2}"#, + fn f() {macro1!(4 - 3 - 1 * 2} + "#]], ) } From 1fce8b6ba32bebba36d588d07781e9e578845728 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Fri, 3 Jul 2020 12:57:17 +1000 Subject: [PATCH 02/14] SSR: Change the way rules are stored internally. Previously we had: - Multiple rules - Each rule had its pattern parsed as an expression, path etc This meant that there were two levels at which there could be multiple rules. Now we just have multiple rules. If a pattern can parse as more than one kind of thing, then they get stored as multiple separate rules. We also now don't have separate fields for the different kinds of things that a pattern can parse as. This makes adding new kinds of things simpler. Previously, add_search_pattern would construct a rule with a dummy replacement. Now the replacement is an Option. This is slightly cleaner and also opens the way for parsing the replacement template as the same kind of thing as the search pattern. --- crates/ra_ssr/src/lib.rs | 76 ++++++++++------------- crates/ra_ssr/src/matching.rs | 42 +++---------- crates/ra_ssr/src/parsing.rs | 106 +++++++++++++++++++++++---------- crates/ra_ssr/src/replacing.rs | 6 +- 4 files changed, 123 insertions(+), 107 deletions(-) diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index cca4576ce0..3009dcb93b 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -13,35 +13,27 @@ mod tests; pub use crate::errors::SsrError; pub use crate::matching::Match; -use crate::matching::{record_match_fails_reasons_scope, MatchFailureReason}; +use crate::matching::MatchFailureReason; use hir::Semantics; +use parsing::SsrTemplate; use ra_db::{FileId, FileRange}; -use ra_syntax::{ast, AstNode, SmolStr, SyntaxKind, SyntaxNode, TextRange}; +use ra_syntax::{ast, AstNode, SyntaxNode, TextRange}; use ra_text_edit::TextEdit; -use rustc_hash::FxHashMap; // A structured search replace rule. Create by calling `parse` on a str. #[derive(Debug)] pub struct SsrRule { /// A structured pattern that we're searching for. - pattern: SsrPattern, + pattern: parsing::RawPattern, /// What we'll replace it with. - template: parsing::SsrTemplate, + template: SsrTemplate, + parsed_rules: Vec, } #[derive(Debug)] pub struct SsrPattern { - raw: parsing::RawSearchPattern, - /// Placeholders keyed by the stand-in ident that we use in Rust source code. - placeholders_by_stand_in: FxHashMap, - // We store our search pattern, parsed as each different kind of thing we can look for. As we - // traverse the AST, we get the appropriate one of these for the type of node we're on. For many - // search patterns, only some of these will be present. - expr: Option, - type_ref: Option, - item: Option, - path: Option, - pattern: Option, + raw: parsing::RawPattern, + parsed_rules: Vec, } #[derive(Debug, Default)] @@ -53,7 +45,7 @@ pub struct SsrMatches { pub struct MatchFinder<'db> { /// Our source of information about the user's code. sema: Semantics<'db, ra_ide_db::RootDatabase>, - rules: Vec, + rules: Vec, } impl<'db> MatchFinder<'db> { @@ -61,14 +53,17 @@ impl<'db> MatchFinder<'db> { MatchFinder { sema: Semantics::new(db), rules: Vec::new() } } + /// Adds a rule to be applied. The order in which rules are added matters. Earlier rules take + /// precedence. If a node is matched by an earlier rule, then later rules won't be permitted to + /// match to it. pub fn add_rule(&mut self, rule: SsrRule) { - self.rules.push(rule); + self.add_parsed_rules(rule.parsed_rules); } /// Adds a search pattern. For use if you intend to only call `find_matches_in_file`. If you /// intend to do replacement, use `add_rule` instead. pub fn add_search_pattern(&mut self, pattern: SsrPattern) { - self.add_rule(SsrRule { pattern, template: "()".parse().unwrap() }) + self.add_parsed_rules(pattern.parsed_rules); } pub fn edits_for_file(&self, file_id: FileId) -> Option { @@ -115,6 +110,14 @@ impl<'db> MatchFinder<'db> { res } + fn add_parsed_rules(&mut self, parsed_rules: Vec) { + // FIXME: This doesn't need to be a for loop, but does in a subsequent commit. Justify it + // being a for-loop. + for parsed_rule in parsed_rules { + self.rules.push(parsed_rule); + } + } + fn find_matches( &self, code: &SyntaxNode, @@ -177,8 +180,13 @@ impl<'db> MatchFinder<'db> { } if node_range.range == range.range { for rule in &self.rules { - let pattern = - rule.pattern.tree_for_kind_with_reason(node.kind()).map(|p| p.clone()); + // For now we ignore rules that have a different kind than our node, otherwise + // we get lots of noise. If at some point we add support for restricting rules + // to a particular kind of thing (e.g. only match type references), then we can + // relax this. + if rule.pattern.kind() != node.kind() { + continue; + } out.push(MatchDebugInfo { matched: matching::get_match(true, rule, &node, restrict_range, &self.sema) .map_err(|e| MatchFailureReason { @@ -186,7 +194,7 @@ impl<'db> MatchFinder<'db> { "Match failed, but no reason was given".to_owned() }), }), - pattern, + pattern: rule.pattern.clone(), node: node.clone(), }); } @@ -209,9 +217,8 @@ impl<'db> MatchFinder<'db> { pub struct MatchDebugInfo { node: SyntaxNode, - /// Our search pattern parsed as the same kind of syntax node as `node`. e.g. expression, item, - /// etc. Will be absent if the pattern can't be parsed as that kind. - pattern: Result, + /// Our search pattern parsed as an expression or item, etc + pattern: SyntaxNode, matched: Result, } @@ -228,29 +235,12 @@ impl std::fmt::Debug for MatchDebugInfo { self.node )?; writeln!(f, "========= PATTERN ==========")?; - match &self.pattern { - Ok(pattern) => { - writeln!(f, "{:#?}", pattern)?; - } - Err(err) => { - writeln!(f, "{}", err.reason)?; - } - } + writeln!(f, "{:#?}", self.pattern)?; writeln!(f, "============================")?; Ok(()) } } -impl SsrPattern { - fn tree_for_kind_with_reason( - &self, - kind: SyntaxKind, - ) -> Result<&SyntaxNode, MatchFailureReason> { - record_match_fails_reasons_scope(true, || self.tree_for_kind(kind)) - .map_err(|e| MatchFailureReason { reason: e.reason.unwrap() }) - } -} - impl SsrMatches { /// Returns `self` with any nested matches removed and made into top-level matches. pub fn flattened(self) -> SsrMatches { diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index 50b29eab2d..842f4b6f35 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -2,8 +2,8 @@ //! process of matching, placeholder values are recorded. use crate::{ - parsing::{Constraint, NodeKind, Placeholder, SsrTemplate}, - SsrMatches, SsrPattern, SsrRule, + parsing::{Constraint, NodeKind, ParsedRule, Placeholder, SsrTemplate}, + SsrMatches, }; use hir::Semantics; use ra_db::FileRange; @@ -50,7 +50,7 @@ pub struct Match { pub(crate) ignored_comments: Vec, // A copy of the template for the rule that produced this match. We store this on the match for // if/when we do replacement. - pub(crate) template: SsrTemplate, + pub(crate) template: Option, } /// Represents a `$var` in an SSR query. @@ -86,7 +86,7 @@ pub(crate) struct MatchFailed { /// parent module, we don't populate nested matches. pub(crate) fn get_match( debug_active: bool, - rule: &SsrRule, + rule: &ParsedRule, code: &SyntaxNode, restrict_range: &Option, sema: &Semantics, @@ -102,7 +102,7 @@ struct Matcher<'db, 'sema> { /// If any placeholders come from anywhere outside of this range, then the match will be /// rejected. restrict_range: Option, - rule: &'sema SsrRule, + rule: &'sema ParsedRule, } /// Which phase of matching we're currently performing. We do two phases because most attempted @@ -117,15 +117,14 @@ enum Phase<'a> { impl<'db, 'sema> Matcher<'db, 'sema> { fn try_match( - rule: &'sema SsrRule, + rule: &ParsedRule, code: &SyntaxNode, restrict_range: &Option, sema: &'sema Semantics<'db, ra_ide_db::RootDatabase>, ) -> Result { let match_state = Matcher { sema, restrict_range: restrict_range.clone(), rule }; - let pattern_tree = rule.pattern.tree_for_kind(code.kind())?; // First pass at matching, where we check that node types and idents match. - match_state.attempt_match_node(&mut Phase::First, &pattern_tree, code)?; + match_state.attempt_match_node(&mut Phase::First, &rule.pattern, code)?; match_state.validate_range(&sema.original_range(code))?; let mut the_match = Match { range: sema.original_range(code), @@ -136,7 +135,7 @@ impl<'db, 'sema> Matcher<'db, 'sema> { }; // Second matching pass, where we record placeholder matches, ignored comments and maybe do // any other more expensive checks that we didn't want to do on the first pass. - match_state.attempt_match_node(&mut Phase::Second(&mut the_match), &pattern_tree, code)?; + match_state.attempt_match_node(&mut Phase::Second(&mut the_match), &rule.pattern, code)?; Ok(the_match) } @@ -444,8 +443,7 @@ impl<'db, 'sema> Matcher<'db, 'sema> { } fn get_placeholder(&self, element: &SyntaxElement) -> Option<&Placeholder> { - only_ident(element.clone()) - .and_then(|ident| self.rule.pattern.placeholders_by_stand_in.get(ident.text())) + only_ident(element.clone()).and_then(|ident| self.rule.get_placeholder(&ident)) } } @@ -510,28 +508,6 @@ impl PlaceholderMatch { } } -impl SsrPattern { - pub(crate) fn tree_for_kind(&self, kind: SyntaxKind) -> Result<&SyntaxNode, MatchFailed> { - let (tree, kind_name) = if ast::Expr::can_cast(kind) { - (&self.expr, "expression") - } else if ast::TypeRef::can_cast(kind) { - (&self.type_ref, "type reference") - } else if ast::ModuleItem::can_cast(kind) { - (&self.item, "item") - } else if ast::Path::can_cast(kind) { - (&self.path, "path") - } else if ast::Pat::can_cast(kind) { - (&self.pattern, "pattern") - } else { - fail_match!("Matching nodes of kind {:?} is not supported", kind); - }; - match tree { - Some(tree) => Ok(tree), - None => fail_match!("Pattern cannot be parsed as a {}", kind_name), - } - } -} - impl NodeKind { fn matches(&self, node: &SyntaxNode) -> Result<(), MatchFailed> { let ok = match self { diff --git a/crates/ra_ssr/src/parsing.rs b/crates/ra_ssr/src/parsing.rs index 4aee97bb2b..682b7011a0 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -7,17 +7,24 @@ use crate::errors::bail; use crate::{SsrError, SsrPattern, SsrRule}; -use ra_syntax::{ast, AstNode, SmolStr, SyntaxKind, T}; +use ra_syntax::{ast, AstNode, SmolStr, SyntaxKind, SyntaxNode, SyntaxToken, T}; use rustc_hash::{FxHashMap, FxHashSet}; use std::str::FromStr; +#[derive(Debug)] +pub(crate) struct ParsedRule { + pub(crate) placeholders_by_stand_in: FxHashMap, + pub(crate) pattern: SyntaxNode, + pub(crate) template: Option, +} + #[derive(Clone, Debug)] pub(crate) struct SsrTemplate { pub(crate) tokens: Vec, } #[derive(Debug)] -pub(crate) struct RawSearchPattern { +pub(crate) struct RawPattern { tokens: Vec, } @@ -54,6 +61,50 @@ pub(crate) struct Token { pub(crate) text: SmolStr, } +impl ParsedRule { + fn new( + pattern: &RawPattern, + template: Option<&SsrTemplate>, + ) -> Result, SsrError> { + let raw_pattern = pattern.as_rust_code(); + let mut builder = RuleBuilder { + placeholders_by_stand_in: pattern.placeholders_by_stand_in(), + rules: Vec::new(), + }; + builder.try_add(ast::Expr::parse(&raw_pattern), template); + builder.try_add(ast::TypeRef::parse(&raw_pattern), template); + builder.try_add(ast::ModuleItem::parse(&raw_pattern), template); + builder.try_add(ast::Path::parse(&raw_pattern), template); + builder.try_add(ast::Pat::parse(&raw_pattern), template); + builder.build() + } +} + +struct RuleBuilder { + placeholders_by_stand_in: FxHashMap, + rules: Vec, +} + +impl RuleBuilder { + fn try_add(&mut self, pattern: Result, template: Option<&SsrTemplate>) { + match pattern { + Ok(pattern) => self.rules.push(ParsedRule { + placeholders_by_stand_in: self.placeholders_by_stand_in.clone(), + pattern: pattern.syntax().clone(), + template: template.cloned(), + }), + _ => {} + } + } + + fn build(self) -> Result, SsrError> { + if self.rules.is_empty() { + bail!("Pattern is not a valid Rust expression, type, item, path or pattern"); + } + Ok(self.rules) + } +} + impl FromStr for SsrRule { type Err = SsrError; @@ -68,21 +119,24 @@ impl FromStr for SsrRule { if it.next().is_some() { return Err(SsrError("More than one delimiter found".into())); } - let rule = SsrRule { pattern: pattern.parse()?, template: template.parse()? }; + let raw_pattern = pattern.parse()?; + let raw_template = template.parse()?; + let parsed_rules = ParsedRule::new(&raw_pattern, Some(&raw_template))?; + let rule = SsrRule { pattern: raw_pattern, template: raw_template, parsed_rules }; validate_rule(&rule)?; Ok(rule) } } -impl FromStr for RawSearchPattern { +impl FromStr for RawPattern { type Err = SsrError; - fn from_str(pattern_str: &str) -> Result { - Ok(RawSearchPattern { tokens: parse_pattern(pattern_str)? }) + fn from_str(pattern_str: &str) -> Result { + Ok(RawPattern { tokens: parse_pattern(pattern_str)? }) } } -impl RawSearchPattern { +impl RawPattern { /// Returns this search pattern as Rust source code that we can feed to the Rust parser. fn as_rust_code(&self) -> String { let mut res = String::new(); @@ -95,7 +149,7 @@ impl RawSearchPattern { res } - fn placeholders_by_stand_in(&self) -> FxHashMap { + pub(crate) fn placeholders_by_stand_in(&self) -> FxHashMap { let mut res = FxHashMap::default(); for t in &self.tokens { if let PatternElement::Placeholder(placeholder) = t { @@ -106,30 +160,22 @@ impl RawSearchPattern { } } +impl ParsedRule { + pub(crate) fn get_placeholder(&self, token: &SyntaxToken) -> Option<&Placeholder> { + if token.kind() != SyntaxKind::IDENT { + return None; + } + self.placeholders_by_stand_in.get(token.text()) + } +} + impl FromStr for SsrPattern { type Err = SsrError; fn from_str(pattern_str: &str) -> Result { - let raw: RawSearchPattern = pattern_str.parse()?; - let raw_str = raw.as_rust_code(); - let res = SsrPattern { - expr: ast::Expr::parse(&raw_str).ok().map(|n| n.syntax().clone()), - type_ref: ast::TypeRef::parse(&raw_str).ok().map(|n| n.syntax().clone()), - item: ast::ModuleItem::parse(&raw_str).ok().map(|n| n.syntax().clone()), - path: ast::Path::parse(&raw_str).ok().map(|n| n.syntax().clone()), - pattern: ast::Pat::parse(&raw_str).ok().map(|n| n.syntax().clone()), - placeholders_by_stand_in: raw.placeholders_by_stand_in(), - raw, - }; - if res.expr.is_none() - && res.type_ref.is_none() - && res.item.is_none() - && res.path.is_none() - && res.pattern.is_none() - { - bail!("Pattern is not a valid Rust expression, type, item, path or pattern"); - } - Ok(res) + let raw_pattern = pattern_str.parse()?; + let parsed_rules = ParsedRule::new(&raw_pattern, None)?; + Ok(SsrPattern { raw: raw_pattern, parsed_rules }) } } @@ -173,7 +219,7 @@ fn parse_pattern(pattern_str: &str) -> Result, SsrError> { /// pattern didn't define. fn validate_rule(rule: &SsrRule) -> Result<(), SsrError> { let mut defined_placeholders = FxHashSet::default(); - for p in &rule.pattern.raw.tokens { + for p in &rule.pattern.tokens { if let PatternElement::Placeholder(placeholder) = p { defined_placeholders.insert(&placeholder.ident); } @@ -316,7 +362,7 @@ mod tests { } let result: SsrRule = "foo($a, $b) ==>> bar($b, $a)".parse().unwrap(); assert_eq!( - result.pattern.raw.tokens, + result.pattern.tokens, vec![ token(SyntaxKind::IDENT, "foo"), token(T!['('], "("), diff --git a/crates/ra_ssr/src/replacing.rs b/crates/ra_ssr/src/replacing.rs index e43cc51674..81f8634baa 100644 --- a/crates/ra_ssr/src/replacing.rs +++ b/crates/ra_ssr/src/replacing.rs @@ -31,7 +31,11 @@ fn matches_to_edit_at_offset( fn render_replace(match_info: &Match, file_src: &str) -> String { let mut out = String::new(); - for r in &match_info.template.tokens { + let template = match_info + .template + .as_ref() + .expect("You called MatchFinder::edits after calling MatchFinder::add_search_pattern"); + for r in &template.tokens { match r { PatternElement::Token(t) => out.push_str(t.text.as_str()), PatternElement::Placeholder(p) => { From 113abbeefee671266d2d9bebdbd517eb8b802ef8 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 22 Jul 2020 19:15:19 +1000 Subject: [PATCH 03/14] SSR: Parse template as Rust code. This is in preparation for a subsequent commit where we add special handling for paths in the template, allowing them to be qualified differently in different contexts. --- crates/ra_ssr/src/lib.rs | 14 +++-- crates/ra_ssr/src/matching.rs | 10 ++-- crates/ra_ssr/src/parsing.rs | 60 +++++++++---------- crates/ra_ssr/src/replacing.rs | 106 ++++++++++++++++++++++----------- crates/ra_ssr/src/tests.rs | 4 +- 5 files changed, 112 insertions(+), 82 deletions(-) diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index 3009dcb93b..b28913a650 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -15,7 +15,6 @@ pub use crate::errors::SsrError; pub use crate::matching::Match; use crate::matching::MatchFailureReason; use hir::Semantics; -use parsing::SsrTemplate; use ra_db::{FileId, FileRange}; use ra_syntax::{ast, AstNode, SyntaxNode, TextRange}; use ra_text_edit::TextEdit; @@ -26,7 +25,7 @@ pub struct SsrRule { /// A structured pattern that we're searching for. pattern: parsing::RawPattern, /// What we'll replace it with. - template: SsrTemplate, + template: parsing::RawPattern, parsed_rules: Vec, } @@ -72,7 +71,11 @@ impl<'db> MatchFinder<'db> { None } else { use ra_db::SourceDatabaseExt; - Some(replacing::matches_to_edit(&matches, &self.sema.db.file_text(file_id))) + Some(replacing::matches_to_edit( + &matches, + &self.sema.db.file_text(file_id), + &self.rules, + )) } } @@ -111,9 +114,8 @@ impl<'db> MatchFinder<'db> { } fn add_parsed_rules(&mut self, parsed_rules: Vec) { - // FIXME: This doesn't need to be a for loop, but does in a subsequent commit. Justify it - // being a for-loop. - for parsed_rule in parsed_rules { + for mut parsed_rule in parsed_rules { + parsed_rule.index = self.rules.len(); self.rules.push(parsed_rule); } } diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index 842f4b6f35..486191635d 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -2,7 +2,7 @@ //! process of matching, placeholder values are recorded. use crate::{ - parsing::{Constraint, NodeKind, ParsedRule, Placeholder, SsrTemplate}, + parsing::{Constraint, NodeKind, ParsedRule, Placeholder}, SsrMatches, }; use hir::Semantics; @@ -48,9 +48,7 @@ pub struct Match { pub(crate) matched_node: SyntaxNode, pub(crate) placeholder_values: FxHashMap, pub(crate) ignored_comments: Vec, - // A copy of the template for the rule that produced this match. We store this on the match for - // if/when we do replacement. - pub(crate) template: Option, + pub(crate) rule_index: usize, } /// Represents a `$var` in an SSR query. @@ -131,7 +129,7 @@ impl<'db, 'sema> Matcher<'db, 'sema> { matched_node: code.clone(), placeholder_values: FxHashMap::default(), ignored_comments: Vec::new(), - template: rule.template.clone(), + rule_index: rule.index, }; // Second matching pass, where we record placeholder matches, ignored comments and maybe do // any other more expensive checks that we didn't want to do on the first pass. @@ -591,7 +589,7 @@ mod tests { "1+2" ); - let edit = crate::replacing::matches_to_edit(&matches, input); + let edit = crate::replacing::matches_to_edit(&matches, input, &match_finder.rules); let mut after = input.to_string(); edit.apply(&mut after); assert_eq!(after, "fn foo() {} fn main() { bar(1+2); }"); diff --git a/crates/ra_ssr/src/parsing.rs b/crates/ra_ssr/src/parsing.rs index 682b7011a0..cf7fb517fa 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -15,12 +15,8 @@ use std::str::FromStr; pub(crate) struct ParsedRule { pub(crate) placeholders_by_stand_in: FxHashMap, pub(crate) pattern: SyntaxNode, - pub(crate) template: Option, -} - -#[derive(Clone, Debug)] -pub(crate) struct SsrTemplate { - pub(crate) tokens: Vec, + pub(crate) template: Option, + pub(crate) index: usize, } #[derive(Debug)] @@ -64,18 +60,23 @@ pub(crate) struct Token { impl ParsedRule { fn new( pattern: &RawPattern, - template: Option<&SsrTemplate>, + template: Option<&RawPattern>, ) -> Result, SsrError> { let raw_pattern = pattern.as_rust_code(); + let raw_template = template.map(|t| t.as_rust_code()); + let raw_template = raw_template.as_ref().map(|s| s.as_str()); let mut builder = RuleBuilder { placeholders_by_stand_in: pattern.placeholders_by_stand_in(), rules: Vec::new(), }; - builder.try_add(ast::Expr::parse(&raw_pattern), template); - builder.try_add(ast::TypeRef::parse(&raw_pattern), template); - builder.try_add(ast::ModuleItem::parse(&raw_pattern), template); - builder.try_add(ast::Path::parse(&raw_pattern), template); - builder.try_add(ast::Pat::parse(&raw_pattern), template); + 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::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() } } @@ -86,12 +87,22 @@ struct RuleBuilder { } impl RuleBuilder { - fn try_add(&mut self, pattern: Result, template: Option<&SsrTemplate>) { - match pattern { - Ok(pattern) => self.rules.push(ParsedRule { + fn try_add(&mut self, pattern: Result, template: Option>) { + match (pattern, template) { + (Ok(pattern), Some(Ok(template))) => self.rules.push(ParsedRule { placeholders_by_stand_in: self.placeholders_by_stand_in.clone(), pattern: pattern.syntax().clone(), - template: template.cloned(), + template: Some(template.syntax().clone()), + // For now we give the rule an index of 0. It's given a proper index when the rule + // is added to the SsrMatcher. Using an Option, instead would be slightly + // more correct, but we delete this field from ParsedRule in a subsequent commit. + index: 0, + }), + (Ok(pattern), None) => self.rules.push(ParsedRule { + placeholders_by_stand_in: self.placeholders_by_stand_in.clone(), + pattern: pattern.syntax().clone(), + template: None, + index: 0, }), _ => {} } @@ -99,7 +110,7 @@ impl RuleBuilder { fn build(self) -> Result, SsrError> { if self.rules.is_empty() { - bail!("Pattern is not a valid Rust expression, type, item, path or pattern"); + bail!("Not a valid Rust expression, type, item, path or pattern"); } Ok(self.rules) } @@ -179,21 +190,6 @@ impl FromStr for SsrPattern { } } -impl FromStr for SsrTemplate { - type Err = SsrError; - - fn from_str(pattern_str: &str) -> Result { - let tokens = parse_pattern(pattern_str)?; - // Validate that the template is a valid fragment of Rust code. We reuse the validation - // logic for search patterns since the only thing that differs is the error message. - if SsrPattern::from_str(pattern_str).is_err() { - bail!("Replacement is not a valid Rust expression, type, item, path or pattern"); - } - // Our actual template needs to preserve whitespace, so we can't reuse `tokens`. - Ok(SsrTemplate { tokens }) - } -} - /// Returns `pattern_str`, parsed as a search or replace pattern. If `remove_whitespace` is true, /// then any whitespace tokens will be removed, which we do for the search pattern, but not for the /// replace pattern. diff --git a/crates/ra_ssr/src/replacing.rs b/crates/ra_ssr/src/replacing.rs index 81f8634baa..f1c5bdf14e 100644 --- a/crates/ra_ssr/src/replacing.rs +++ b/crates/ra_ssr/src/replacing.rs @@ -1,70 +1,104 @@ //! Code for applying replacement templates for matches that have previously been found. use crate::matching::Var; -use crate::parsing::PatternElement; -use crate::{Match, SsrMatches}; +use crate::{parsing::ParsedRule, Match, SsrMatches}; use ra_syntax::ast::AstToken; -use ra_syntax::TextSize; +use ra_syntax::{SyntaxElement, SyntaxNode, SyntaxToken, TextSize}; use ra_text_edit::TextEdit; /// 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 /// in the original code. -pub(crate) fn matches_to_edit(matches: &SsrMatches, file_src: &str) -> TextEdit { - matches_to_edit_at_offset(matches, file_src, 0.into()) +pub(crate) fn matches_to_edit( + matches: &SsrMatches, + file_src: &str, + rules: &[ParsedRule], +) -> TextEdit { + matches_to_edit_at_offset(matches, file_src, 0.into(), rules) } fn matches_to_edit_at_offset( matches: &SsrMatches, file_src: &str, relative_start: TextSize, + rules: &[ParsedRule], ) -> TextEdit { let mut edit_builder = ra_text_edit::TextEditBuilder::default(); for m in &matches.matches { edit_builder.replace( m.range.range.checked_sub(relative_start).unwrap(), - render_replace(m, file_src), + render_replace(m, file_src, rules), ); } edit_builder.finish() } -fn render_replace(match_info: &Match, file_src: &str) -> String { +struct ReplacementRenderer<'a> { + match_info: &'a Match, + file_src: &'a str, + rules: &'a [ParsedRule], + rule: &'a ParsedRule, +} + +fn render_replace(match_info: &Match, file_src: &str, rules: &[ParsedRule]) -> String { let mut out = String::new(); - let template = match_info + let rule = &rules[match_info.rule_index]; + let template = rule .template .as_ref() .expect("You called MatchFinder::edits after calling MatchFinder::add_search_pattern"); - for r in &template.tokens { - match r { - PatternElement::Token(t) => out.push_str(t.text.as_str()), - PatternElement::Placeholder(p) => { - if let Some(placeholder_value) = - match_info.placeholder_values.get(&Var(p.ident.to_string())) - { - let range = &placeholder_value.range.range; - let mut matched_text = - file_src[usize::from(range.start())..usize::from(range.end())].to_owned(); - let edit = matches_to_edit_at_offset( - &placeholder_value.inner_matches, - file_src, - range.start(), - ); - edit.apply(&mut matched_text); - out.push_str(&matched_text); - } else { - // We validated that all placeholder references were valid before we - // started, so this shouldn't happen. - panic!( - "Internal error: replacement referenced unknown placeholder {}", - p.ident - ); - } - } - } - } + let renderer = ReplacementRenderer { match_info, file_src, rules, rule }; + renderer.render_node_children(&template, &mut out); for comment in &match_info.ignored_comments { out.push_str(&comment.syntax().to_string()); } out } + +impl ReplacementRenderer<'_> { + fn render_node_children(&self, node: &SyntaxNode, out: &mut String) { + for node_or_token in node.children_with_tokens() { + self.render_node_or_token(&node_or_token, out); + } + } + + fn render_node_or_token(&self, node_or_token: &SyntaxElement, out: &mut String) { + match node_or_token { + SyntaxElement::Token(token) => { + self.render_token(&token, out); + } + SyntaxElement::Node(child_node) => { + self.render_node_children(&child_node, out); + } + } + } + + fn render_token(&self, token: &SyntaxToken, out: &mut String) { + 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())) + { + let range = &placeholder_value.range.range; + let mut matched_text = + self.file_src[usize::from(range.start())..usize::from(range.end())].to_owned(); + let edit = matches_to_edit_at_offset( + &placeholder_value.inner_matches, + self.file_src, + range.start(), + self.rules, + ); + edit.apply(&mut matched_text); + out.push_str(&matched_text); + } else { + // We validated that all placeholder references were valid before we + // started, so this shouldn't happen. + panic!( + "Internal error: replacement referenced unknown placeholder {}", + placeholder.ident + ); + } + } else { + out.push_str(token.text().as_str()); + } + } +} diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 9f53065929..1b03b7f4bb 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -37,7 +37,7 @@ fn parser_repeated_name() { fn parser_invalid_pattern() { assert_eq!( parse_error_text(" ==>> ()"), - "Parse error: Pattern is not a valid Rust expression, type, item, path or pattern" + "Parse error: Not a valid Rust expression, type, item, path or pattern" ); } @@ -45,7 +45,7 @@ fn parser_invalid_pattern() { fn parser_invalid_template() { assert_eq!( parse_error_text("() ==>> )"), - "Parse error: Replacement is not a valid Rust expression, type, item, path or pattern" + "Parse error: Not a valid Rust expression, type, item, path or pattern" ); } From 13f901f636846e330699a4414059591ec2e67cd1 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 22 Jul 2020 16:31:32 +1000 Subject: [PATCH 04/14] SSR: Move search code into a submodule Also renamed find_matches to slow_scan_node to reflect that it's a slow way to do things. Actually the name came from a later commit and probably makes more sense once there's an alternative. --- crates/ra_ssr/src/lib.rs | 50 ++-------------------------------- crates/ra_ssr/src/search.rs | 54 +++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 48 deletions(-) create mode 100644 crates/ra_ssr/src/search.rs diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index b28913a650..dac73c07c6 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -6,6 +6,7 @@ mod matching; mod parsing; mod replacing; +mod search; #[macro_use] mod errors; #[cfg(test)] @@ -83,7 +84,7 @@ impl<'db> MatchFinder<'db> { let file = self.sema.parse(file_id); let code = file.syntax(); let mut matches = SsrMatches::default(); - self.find_matches(code, &None, &mut matches); + self.slow_scan_node(code, &None, &mut matches.matches); matches } @@ -120,53 +121,6 @@ impl<'db> MatchFinder<'db> { } } - fn find_matches( - &self, - code: &SyntaxNode, - restrict_range: &Option, - matches_out: &mut SsrMatches, - ) { - for rule in &self.rules { - if let Ok(mut m) = matching::get_match(false, rule, &code, restrict_range, &self.sema) { - // Continue searching in each of our placeholders. - for placeholder_value in m.placeholder_values.values_mut() { - if let Some(placeholder_node) = &placeholder_value.node { - // Don't search our placeholder if it's the entire matched node, otherwise we'd - // find the same match over and over until we got a stack overflow. - if placeholder_node != code { - self.find_matches( - placeholder_node, - restrict_range, - &mut placeholder_value.inner_matches, - ); - } - } - } - matches_out.matches.push(m); - return; - } - } - // 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()) { - if let Some(expanded) = self.sema.expand(¯o_call) { - if let Some(tt) = macro_call.token_tree() { - // When matching within a macro expansion, we only want to allow matches of - // nodes that originated entirely from within the token tree of the macro call. - // i.e. we don't want to match something that came from the macro itself. - self.find_matches( - &expanded, - &Some(self.sema.original_range(tt.syntax())), - matches_out, - ); - } - } - } - for child in code.children() { - self.find_matches(&child, restrict_range, matches_out); - } - } - fn output_debug_for_nodes_at_range( &self, node: &SyntaxNode, diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs new file mode 100644 index 0000000000..6f21452ac4 --- /dev/null +++ b/crates/ra_ssr/src/search.rs @@ -0,0 +1,54 @@ +//! Searching for matches. + +use crate::{matching, Match, MatchFinder}; +use ra_db::FileRange; +use ra_syntax::{ast, AstNode, SyntaxNode}; + +impl<'db> MatchFinder<'db> { + pub(crate) fn slow_scan_node( + &self, + code: &SyntaxNode, + restrict_range: &Option, + matches_out: &mut Vec, + ) { + for rule in &self.rules { + if let Ok(mut m) = matching::get_match(false, rule, &code, restrict_range, &self.sema) { + // Continue searching in each of our placeholders. + for placeholder_value in m.placeholder_values.values_mut() { + if let Some(placeholder_node) = &placeholder_value.node { + // Don't search our placeholder if it's the entire matched node, otherwise we'd + // find the same match over and over until we got a stack overflow. + if placeholder_node != code { + self.slow_scan_node( + placeholder_node, + restrict_range, + &mut placeholder_value.inner_matches.matches, + ); + } + } + } + matches_out.push(m); + return; + } + } + // 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()) { + if let Some(expanded) = self.sema.expand(¯o_call) { + if let Some(tt) = macro_call.token_tree() { + // When matching within a macro expansion, we only want to allow matches of + // nodes that originated entirely from within the token tree of the macro call. + // i.e. we don't want to match something that came from the macro itself. + self.slow_scan_node( + &expanded, + &Some(self.sema.original_range(tt.syntax())), + matches_out, + ); + } + } + } + for child in code.children() { + self.slow_scan_node(&child, restrict_range, matches_out); + } + } +} From a45682ed96f18f962ac403419b4d143d59ba5283 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 22 Jul 2020 16:23:43 +1000 Subject: [PATCH 05/14] Move iteration over all files into the SSR crate The methods `edits_for_file` and `find_matches_in_file` are replaced with just `edits` and `matches`. This simplifies the API a bit, but more importantly it makes it possible in a subsequent commit for SSR to decide to not search all files. --- crates/ra_ide/src/ssr.rs | 16 ++-------- crates/ra_ssr/src/lib.rs | 48 ++++++++++++++++------------- crates/ra_ssr/src/matching.rs | 15 ++++----- crates/ra_ssr/src/search.rs | 21 ++++++++++++- crates/ra_ssr/src/tests.rs | 40 +++++++++++++----------- crates/rust-analyzer/src/cli/ssr.rs | 41 ++++++++---------------- 6 files changed, 92 insertions(+), 89 deletions(-) diff --git a/crates/ra_ide/src/ssr.rs b/crates/ra_ide/src/ssr.rs index b3e9e5dfe1..ca7e0ad866 100644 --- a/crates/ra_ide/src/ssr.rs +++ b/crates/ra_ide/src/ssr.rs @@ -1,5 +1,4 @@ -use ra_db::SourceDatabaseExt; -use ra_ide_db::{symbol_index::SymbolsDatabase, RootDatabase}; +use ra_ide_db::RootDatabase; use crate::SourceFileEdit; use ra_ssr::{MatchFinder, SsrError, SsrRule}; @@ -44,20 +43,11 @@ pub fn parse_search_replace( parse_only: bool, db: &RootDatabase, ) -> Result, SsrError> { - let mut edits = vec![]; let rule: SsrRule = rule.parse()?; if parse_only { - return Ok(edits); + return Ok(Vec::new()); } let mut match_finder = MatchFinder::new(db); match_finder.add_rule(rule); - for &root in db.local_roots().iter() { - let sr = db.source_root(root); - for file_id in sr.iter() { - if let Some(edit) = match_finder.edits_for_file(file_id) { - edits.push(SourceFileEdit { file_id, edit }); - } - } - } - Ok(edits) + Ok(match_finder.edits()) } diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index dac73c07c6..7b64098062 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -17,8 +17,9 @@ pub use crate::matching::Match; use crate::matching::MatchFailureReason; use hir::Semantics; use ra_db::{FileId, FileRange}; +use ra_ide_db::source_change::SourceFileEdit; use ra_syntax::{ast, AstNode, SyntaxNode, TextRange}; -use ra_text_edit::TextEdit; +use rustc_hash::FxHashMap; // A structured search replace rule. Create by calling `parse` on a str. #[derive(Debug)] @@ -60,32 +61,37 @@ impl<'db> MatchFinder<'db> { self.add_parsed_rules(rule.parsed_rules); } + /// Finds matches for all added rules and returns edits for all found matches. + pub fn edits(&self) -> Vec { + use ra_db::SourceDatabaseExt; + let mut matches_by_file = FxHashMap::default(); + for m in self.matches().matches { + matches_by_file + .entry(m.range.file_id) + .or_insert_with(|| SsrMatches::default()) + .matches + .push(m); + } + let mut edits = vec![]; + for (file_id, matches) in matches_by_file { + let edit = + replacing::matches_to_edit(&matches, &self.sema.db.file_text(file_id), &self.rules); + edits.push(SourceFileEdit { file_id, edit }); + } + edits + } + /// Adds a search pattern. For use if you intend to only call `find_matches_in_file`. If you /// intend to do replacement, use `add_rule` instead. pub fn add_search_pattern(&mut self, pattern: SsrPattern) { self.add_parsed_rules(pattern.parsed_rules); } - pub fn edits_for_file(&self, file_id: FileId) -> Option { - let matches = self.find_matches_in_file(file_id); - if matches.matches.is_empty() { - None - } else { - use ra_db::SourceDatabaseExt; - Some(replacing::matches_to_edit( - &matches, - &self.sema.db.file_text(file_id), - &self.rules, - )) - } - } - - pub fn find_matches_in_file(&self, file_id: FileId) -> SsrMatches { - let file = self.sema.parse(file_id); - let code = file.syntax(); - let mut matches = SsrMatches::default(); - self.slow_scan_node(code, &None, &mut matches.matches); - matches + /// Returns matches for all added rules. + pub fn matches(&self) -> SsrMatches { + let mut matches = Vec::new(); + self.find_all_matches(&mut matches); + SsrMatches { matches } } /// Finds all nodes in `file_id` whose text is exactly equal to `snippet` and attempts to match diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index 486191635d..064e3a204d 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -570,13 +570,12 @@ mod tests { #[test] fn parse_match_replace() { let rule: SsrRule = "foo($x) ==>> bar($x)".parse().unwrap(); - let input = "fn foo() {} fn main() { foo(1+2); }"; + let input = "fn foo() {} fn bar() {} fn main() { foo(1+2); }"; - use ra_db::fixture::WithFixture; - let (db, file_id) = ra_ide_db::RootDatabase::with_single_file(input); + let (db, _) = crate::tests::single_file(input); let mut match_finder = MatchFinder::new(&db); match_finder.add_rule(rule); - let matches = match_finder.find_matches_in_file(file_id); + let matches = match_finder.matches(); assert_eq!(matches.matches.len(), 1); assert_eq!(matches.matches[0].matched_node.text(), "foo(1+2)"); assert_eq!(matches.matches[0].placeholder_values.len(), 1); @@ -589,9 +588,11 @@ mod tests { "1+2" ); - let edit = crate::replacing::matches_to_edit(&matches, input, &match_finder.rules); + let edits = match_finder.edits(); + assert_eq!(edits.len(), 1); + let edit = &edits[0]; let mut after = input.to_string(); - edit.apply(&mut after); - assert_eq!(after, "fn foo() {} fn main() { bar(1+2); }"); + edit.edit.apply(&mut after); + assert_eq!(after, "fn foo() {} fn bar() {} fn main() { bar(1+2); }"); } } diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index 6f21452ac4..ec3addcf89 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -5,7 +5,26 @@ use ra_db::FileRange; use ra_syntax::{ast, AstNode, SyntaxNode}; impl<'db> MatchFinder<'db> { - pub(crate) fn slow_scan_node( + pub(crate) fn find_all_matches(&self, matches_out: &mut Vec) { + // FIXME: Use resolved paths in the pattern to find places to search instead of always + // scanning every node. + self.slow_scan(matches_out); + } + + fn slow_scan(&self, 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, &None, matches_out); + } + } + } + + fn slow_scan_node( &self, code: &SyntaxNode, restrict_range: &Option, diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 1b03b7f4bb..c7c37af2f5 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -1,6 +1,8 @@ use crate::{MatchFinder, SsrRule}; use expect::{expect, Expect}; -use ra_db::{FileId, SourceDatabaseExt}; +use ra_db::{salsa::Durability, FileId, SourceDatabaseExt}; +use rustc_hash::FxHashSet; +use std::sync::Arc; use test_utils::mark; fn parse_error_text(query: &str) -> String { @@ -57,9 +59,15 @@ fn parser_undefined_placeholder_in_replacement() { ); } -fn single_file(code: &str) -> (ra_ide_db::RootDatabase, FileId) { +pub(crate) fn single_file(code: &str) -> (ra_ide_db::RootDatabase, FileId) { use ra_db::fixture::WithFixture; - ra_ide_db::RootDatabase::with_single_file(code) + use ra_ide_db::symbol_index::SymbolsDatabase; + let (db, file_id) = ra_ide_db::RootDatabase::with_single_file(code); + let mut db = db; + 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, file_id) } fn assert_ssr_transform(rule: &str, input: &str, expected: Expect) { @@ -73,15 +81,16 @@ fn assert_ssr_transforms(rules: &[&str], input: &str, expected: Expect) { let rule: SsrRule = rule.parse().unwrap(); match_finder.add_rule(rule); } - if let Some(edits) = match_finder.edits_for_file(file_id) { - // Note, db.file_text is not necessarily the same as `input`, since fixture parsing alters - // stuff. - let mut actual = db.file_text(file_id).to_string(); - edits.apply(&mut actual); - expected.assert_eq(&actual); - } else { + let edits = match_finder.edits(); + if edits.is_empty() { panic!("No edits were made"); } + assert_eq!(edits[0].file_id, file_id); + // Note, db.file_text is not necessarily the same as `input`, since fixture parsing alters + // stuff. + let mut actual = db.file_text(file_id).to_string(); + edits[0].edit.apply(&mut actual); + expected.assert_eq(&actual); } fn print_match_debug_info(match_finder: &MatchFinder, file_id: FileId, snippet: &str) { @@ -100,13 +109,8 @@ fn assert_matches(pattern: &str, code: &str, expected: &[&str]) { let (db, file_id) = single_file(code); let mut match_finder = MatchFinder::new(&db); match_finder.add_search_pattern(pattern.parse().unwrap()); - let matched_strings: Vec = match_finder - .find_matches_in_file(file_id) - .flattened() - .matches - .iter() - .map(|m| m.matched_text()) - .collect(); + let matched_strings: Vec = + match_finder.matches().flattened().matches.iter().map(|m| m.matched_text()).collect(); if matched_strings != expected && !expected.is_empty() { print_match_debug_info(&match_finder, file_id, &expected[0]); } @@ -117,7 +121,7 @@ fn assert_no_match(pattern: &str, code: &str) { let (db, file_id) = single_file(code); let mut match_finder = MatchFinder::new(&db); match_finder.add_search_pattern(pattern.parse().unwrap()); - let matches = match_finder.find_matches_in_file(file_id).flattened().matches; + let matches = match_finder.matches().flattened().matches; if !matches.is_empty() { print_match_debug_info(&match_finder, file_id, &matches[0].matched_text()); panic!("Got {} matches when we expected none: {:#?}", matches.len(), matches); diff --git a/crates/rust-analyzer/src/cli/ssr.rs b/crates/rust-analyzer/src/cli/ssr.rs index 4fb829ea5c..014bc70a45 100644 --- a/crates/rust-analyzer/src/cli/ssr.rs +++ b/crates/rust-analyzer/src/cli/ssr.rs @@ -1,27 +1,17 @@ //! Applies structured search replace rules from the command line. use crate::cli::{load_cargo::load_cargo, Result}; -use ra_ide::SourceFileEdit; use ra_ssr::{MatchFinder, SsrPattern, SsrRule}; pub fn apply_ssr_rules(rules: Vec) -> Result<()> { use ra_db::SourceDatabaseExt; - use ra_ide_db::symbol_index::SymbolsDatabase; let (host, vfs) = load_cargo(&std::env::current_dir()?, true, true)?; let db = host.raw_database(); let mut match_finder = MatchFinder::new(db); for rule in rules { match_finder.add_rule(rule); } - let mut edits = Vec::new(); - for &root in db.local_roots().iter() { - let sr = db.source_root(root); - for file_id in sr.iter() { - if let Some(edit) = match_finder.edits_for_file(file_id) { - edits.push(SourceFileEdit { file_id, edit }); - } - } - } + let edits = match_finder.edits(); for edit in edits { if let Some(path) = vfs.file_path(edit.file_id).as_path() { let mut contents = db.file_text(edit.file_id).to_string(); @@ -38,34 +28,27 @@ pub fn apply_ssr_rules(rules: Vec) -> Result<()> { pub fn search_for_patterns(patterns: Vec, debug_snippet: Option) -> Result<()> { use ra_db::SourceDatabaseExt; use ra_ide_db::symbol_index::SymbolsDatabase; - let (host, vfs) = load_cargo(&std::env::current_dir()?, true, true)?; + let (host, _vfs) = load_cargo(&std::env::current_dir()?, true, true)?; let db = host.raw_database(); let mut match_finder = MatchFinder::new(db); for pattern in patterns { match_finder.add_search_pattern(pattern); } - for &root in db.local_roots().iter() { - let sr = db.source_root(root); - for file_id in sr.iter() { - if let Some(debug_snippet) = &debug_snippet { + if let Some(debug_snippet) = &debug_snippet { + for &root in db.local_roots().iter() { + let sr = db.source_root(root); + for file_id in sr.iter() { for debug_info in match_finder.debug_where_text_equal(file_id, debug_snippet) { println!("{:#?}", debug_info); } - } else { - let matches = match_finder.find_matches_in_file(file_id); - if !matches.matches.is_empty() { - let matches = matches.flattened().matches; - if let Some(path) = vfs.file_path(file_id).as_path() { - println!("{} matches in '{}'", matches.len(), path.to_string_lossy()); - } - // We could possibly at some point do something more useful than just printing - // the matched text. For now though, that's the easiest thing to do. - for m in matches { - println!("{}", m.matched_text()); - } - } } } + } else { + for m in match_finder.matches().flattened().matches { + // We could possibly at some point do something more useful than just printing + // the matched text. For now though, that's the easiest thing to do. + println!("{}", m.matched_text()); + } } Ok(()) } From 6fcaaa1201c650ce22b71160f6e9bf2288d10a1a Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 22 Jul 2020 16:06:14 +1000 Subject: [PATCH 06/14] SSR tests: Define all paths needed for templates In a later commit, paths in templates will be resolved. This allows us to render the path with appropriate qualifiers for its context. Here we prepare for that change by updating existing tests where I'd previously not bothered to define the items that the template referred to. --- crates/ra_ssr/src/tests.rs | 108 +++++++++++++++++++++++++++---------- 1 file changed, 79 insertions(+), 29 deletions(-) diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index c7c37af2f5..11512c8ccd 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -154,8 +154,19 @@ fn ssr_function_to_method() { fn ssr_nested_function() { assert_ssr_transform( "foo($a, $b, $c) ==>> bar($c, baz($a, $b))", - "fn foo() {} fn main { foo (x + value.method(b), x+y-z, true && false) }", - expect![["fn foo() {} fn main { bar(true && false, baz(x + value.method(b), x+y-z)) }"]], + r#" + //- /lib.rs crate:foo + fn foo() {} + fn bar() {} + fn baz() {} + fn main { foo (x + value.method(b), x+y-z, true && false) } + "#, + expect![[r#" + fn foo() {} + fn bar() {} + fn baz() {} + fn main { bar(true && false, baz(x + value.method(b), x+y-z)) } + "#]], ) } @@ -181,8 +192,8 @@ fn ssr_with_extra_space() { fn ssr_keeps_nested_comment() { assert_ssr_transform( "foo($x) ==>> bar($x)", - "fn foo() {} fn main() { foo(other(5 /* using 5 */)) }", - expect![["fn foo() {} fn main() { bar(other(5 /* using 5 */)) }"]], + "fn foo() {} fn bar() {} fn main() { foo(other(5 /* using 5 */)) }", + expect![["fn foo() {} fn bar() {} fn main() { bar(other(5 /* using 5 */)) }"]], ) } @@ -190,17 +201,25 @@ fn ssr_keeps_nested_comment() { fn ssr_keeps_comment() { assert_ssr_transform( "foo($x) ==>> bar($x)", - "fn foo() {} fn main() { foo(5 /* using 5 */) }", - expect![["fn foo() {} fn main() { bar(5)/* using 5 */ }"]], + "fn foo() {} fn bar() {} fn main() { foo(5 /* using 5 */) }", + expect![["fn foo() {} fn bar() {} fn main() { bar(5)/* using 5 */ }"]], ) } #[test] fn ssr_struct_lit() { assert_ssr_transform( - "foo{a: $a, b: $b} ==>> foo::new($a, $b)", - "fn foo() {} fn main() { foo{b:2, a:1} }", - expect![["fn foo() {} fn main() { foo::new(1, 2) }"]], + "Foo{a: $a, b: $b} ==>> Foo::new($a, $b)", + r#" + struct Foo() {} + impl Foo { fn new() {} } + fn main() { Foo{b:2, a:1} } + "#, + expect![[r#" + struct Foo() {} + impl Foo { fn new() {} } + fn main() { Foo::new(1, 2) } + "#]], ) } @@ -312,7 +331,7 @@ fn match_struct_instantiation() { fn match_path() { let code = r#" mod foo { - fn bar() {} + pub fn bar() {} } fn f() {foo::bar(42)}"#; assert_matches("foo::bar", code, &["foo::bar"]); @@ -413,8 +432,8 @@ fn no_match_split_expression() { fn replace_function_call() { assert_ssr_transform( "foo() ==>> bar()", - "fn foo() {} fn f1() {foo(); foo();}", - expect![["fn foo() {} fn f1() {bar(); bar();}"]], + "fn foo() {} fn bar() {} fn f1() {foo(); foo();}", + expect![["fn foo() {} fn bar() {} fn f1() {bar(); bar();}"]], ); } @@ -422,8 +441,8 @@ fn replace_function_call() { fn replace_function_call_with_placeholders() { assert_ssr_transform( "foo($a, $b) ==>> bar($b, $a)", - "fn foo() {} fn f1() {foo(5, 42)}", - expect![["fn foo() {} fn f1() {bar(42, 5)}"]], + "fn foo() {} fn bar() {} fn f1() {foo(5, 42)}", + expect![["fn foo() {} fn bar() {} fn f1() {bar(42, 5)}"]], ); } @@ -431,8 +450,29 @@ fn replace_function_call_with_placeholders() { fn replace_nested_function_calls() { assert_ssr_transform( "foo($a) ==>> bar($a)", - "fn foo() {} fn f1() {foo(foo(42))}", - expect![["fn foo() {} fn f1() {bar(bar(42))}"]], + "fn foo() {} fn bar() {} fn f1() {foo(foo(42))}", + expect![["fn foo() {} fn bar() {} fn f1() {bar(bar(42))}"]], + ); +} + +#[test] +fn replace_associated_function_call() { + assert_ssr_transform( + "Foo::new() ==>> Bar::new()", + r#" + struct Foo {} + impl Foo { fn new() {} } + struct Bar {} + impl Bar { fn new() {} } + fn f1() {Foo::new();} + "#, + expect![[r#" + struct Foo {} + impl Foo { fn new() {} } + struct Bar {} + impl Bar { fn new() {} } + fn f1() {Bar::new();} + "#]], ); } @@ -440,17 +480,10 @@ fn replace_nested_function_calls() { fn replace_type() { assert_ssr_transform( "Result<(), $a> ==>> Option<$a>", - "struct Result {} fn f1() -> Result<(), Vec> {foo()}", - expect![["struct Result {} fn f1() -> Option> {foo()}"]], - ); -} - -#[test] -fn replace_struct_init() { - assert_ssr_transform( - "Foo {a: $a, b: $b} ==>> Foo::new($a, $b)", - "struct Foo {} fn f1() {Foo{b: 1, a: 2}}", - expect![["struct Foo {} fn f1() {Foo::new(2, 1)}"]], + "struct Result {} struct Option {} fn f1() -> Result<(), Vec> {foo()}", + expect![[ + "struct Result {} struct Option {} fn f1() -> Option> {foo()}" + ]], ); } @@ -491,8 +524,23 @@ fn match_binary_op() { fn multiple_rules() { assert_ssr_transforms( &["$a + 1 ==>> add_one($a)", "$a + $b ==>> add($a, $b)"], - "fn f() -> i32 {3 + 2 + 1}", - expect![["fn f() -> i32 {add_one(add(3, 2))}"]], + "fn add() {} fn add_one() {} fn f() -> i32 {3 + 2 + 1}", + expect![["fn add() {} fn add_one() {} fn f() -> i32 {add_one(add(3, 2))}"]], + ) +} + +#[test] +fn multiple_rules_with_nested_matches() { + assert_ssr_transforms( + &["foo1($a) ==>> bar1($a)", "foo2($a) ==>> bar2($a)"], + r#" + fn foo1() {} fn foo2() {} fn bar1() {} fn bar2() {} + fn f() {foo1(foo2(foo1(foo2(foo1(42)))))} + "#, + expect![[r#" + fn foo1() {} fn foo2() {} fn bar1() {} fn bar2() {} + fn f() {bar1(bar2(bar1(bar2(bar1(42)))))} + "#]], ) } @@ -524,12 +572,14 @@ fn replace_within_macro_expansion() { macro_rules! macro1 { ($a:expr) => {$a} } + fn bar() {} fn f() {macro1!(5.x().foo().o2())} "#, expect![[r#" macro_rules! macro1 { ($a:expr) => {$a} } + fn bar() {} fn f() {macro1!(bar(5.x()).o2())} "#]], ) From 699619a65cf816b927fffa77b2b38f611d8460bc Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 22 Jul 2020 16:00:57 +1000 Subject: [PATCH 07/14] SSR: Add a couple of tests for non-recursive search These tests already pass, however once we switch to non-recursive search, it'd be easy for these tests to not pass. --- crates/ra_ssr/src/tests.rs | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 11512c8ccd..523035b310 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -585,6 +585,27 @@ fn replace_within_macro_expansion() { ) } +#[test] +fn replace_outside_and_within_macro_expansion() { + assert_ssr_transform( + "foo($a) ==>> bar($a)", + r#" + fn foo() {} fn bar() {} + macro_rules! macro1 { + ($a:expr) => {$a} + } + fn f() {foo(foo(macro1!(foo(foo(42)))))} + "#, + expect![[r#" + fn foo() {} fn bar() {} + macro_rules! macro1 { + ($a:expr) => {$a} + } + fn f() {bar(bar(macro1!(bar(bar(42)))))} + "#]], + ) +} + #[test] fn preserves_whitespace_within_macro_expansion() { assert_ssr_transform( @@ -631,3 +652,15 @@ fn match_failure_reasons() { r#"Pattern wanted token '42' (INT_NUMBER), but code had token '43' (INT_NUMBER)"#, ); } + +#[test] +fn overlapping_possible_matches() { + // There are three possible matches here, however the middle one, `foo(foo(foo(42)))` shouldn't + // match because it overlaps with the outer match. The inner match is permitted since it's is + // contained entirely within the placeholder of the outer match. + assert_matches( + "foo(foo($a))", + "fn foo() {} fn main() {foo(foo(foo(foo(42))))}", + &["foo(foo(42))", "foo(foo(foo(foo(42))))"], + ); +} From 02fc3d50ee4d179cc5a443a790544c2a5e439cb0 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 22 Jul 2020 16:48:12 +1000 Subject: [PATCH 08/14] SSR: Refactor to not rely on recursive search for nesting of matches Previously, submatches were handled simply by searching in placeholders for more matches. That only works if we search all nodes in the tree recursively. In a subsequent commit, I intend to make search not always be recursive recursive. This commit prepares for that by finding all matches, even if they overlap, then nesting them and removing overlapping matches. --- crates/ra_ssr/src/lib.rs | 7 ++- crates/ra_ssr/src/matching.rs | 4 ++ crates/ra_ssr/src/nester.rs | 98 +++++++++++++++++++++++++++++++++++ crates/ra_ssr/src/search.rs | 38 +++++--------- 4 files changed, 120 insertions(+), 27 deletions(-) create mode 100644 crates/ra_ssr/src/nester.rs diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index 7b64098062..6d578610b4 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -4,6 +4,7 @@ //! based on a template. mod matching; +mod nester; mod parsing; mod replacing; mod search; @@ -90,8 +91,10 @@ impl<'db> MatchFinder<'db> { /// Returns matches for all added rules. pub fn matches(&self) -> SsrMatches { let mut matches = Vec::new(); - self.find_all_matches(&mut matches); - SsrMatches { matches } + for rule in &self.rules { + self.find_matches_for_rule(rule, &mut matches); + } + nester::nest_and_remove_collisions(matches, &self.sema) } /// Finds all nodes in `file_id` whose text is exactly equal to `snippet` and attempts to match diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index 064e3a204d..005569f6fa 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -49,6 +49,8 @@ pub struct Match { pub(crate) placeholder_values: FxHashMap, pub(crate) ignored_comments: Vec, pub(crate) rule_index: usize, + /// The depth of matched_node. + pub(crate) depth: usize, } /// Represents a `$var` in an SSR query. @@ -130,10 +132,12 @@ impl<'db, 'sema> Matcher<'db, 'sema> { placeholder_values: FxHashMap::default(), ignored_comments: Vec::new(), rule_index: rule.index, + depth: 0, }; // Second matching pass, where we record placeholder matches, ignored comments and maybe do // any other more expensive checks that we didn't want to do on the first pass. match_state.attempt_match_node(&mut Phase::Second(&mut the_match), &rule.pattern, code)?; + the_match.depth = sema.ancestors_with_macros(the_match.matched_node.clone()).count(); Ok(the_match) } diff --git a/crates/ra_ssr/src/nester.rs b/crates/ra_ssr/src/nester.rs new file mode 100644 index 0000000000..b3e20579bd --- /dev/null +++ b/crates/ra_ssr/src/nester.rs @@ -0,0 +1,98 @@ +//! Converts a flat collection of matches into a nested form suitable for replacement. When there +//! are multiple matches for a node, or that overlap, priority is given to the earlier rule. Nested +//! matches are only permitted if the inner match is contained entirely within a placeholder of an +//! outer match. +//! +//! For example, if our search pattern is `foo(foo($a))` and the code had `foo(foo(foo(foo(42))))`, +//! then we'll get 3 matches, however only the outermost and innermost matches can be accepted. The +//! middle match would take the second `foo` from the outer match. + +use crate::{Match, SsrMatches}; +use ra_syntax::SyntaxNode; +use rustc_hash::FxHashMap; + +pub(crate) fn nest_and_remove_collisions( + mut matches: Vec, + sema: &hir::Semantics, +) -> SsrMatches { + // We sort the matches by depth then by rule index. Sorting by depth means that by the time we + // see a match, any parent matches or conflicting matches will have already been seen. Sorting + // by rule_index means that if there are two matches for the same node, the rule added first + // will take precedence. + matches.sort_by(|a, b| a.depth.cmp(&b.depth).then_with(|| a.rule_index.cmp(&b.rule_index))); + let mut collector = MatchCollector::default(); + for m in matches { + collector.add_match(m, sema); + } + collector.into() +} + +#[derive(Default)] +struct MatchCollector { + matches_by_node: FxHashMap, +} + +impl MatchCollector { + /// Attempts to add `m` to matches. If it conflicts with an existing match, it is discarded. If + /// it is entirely within the a placeholder of an existing match, then it is added as a child + /// match of the existing match. + fn add_match(&mut self, m: Match, sema: &hir::Semantics) { + let matched_node = m.matched_node.clone(); + if let Some(existing) = self.matches_by_node.get_mut(&matched_node) { + try_add_sub_match(m, existing, sema); + return; + } + for ancestor in sema.ancestors_with_macros(m.matched_node.clone()) { + if let Some(existing) = self.matches_by_node.get_mut(&ancestor) { + try_add_sub_match(m, existing, sema); + return; + } + } + self.matches_by_node.insert(matched_node, m); + } +} + +/// Attempts to add `m` as a sub-match of `existing`. +fn try_add_sub_match( + m: Match, + existing: &mut Match, + sema: &hir::Semantics, +) { + for p in existing.placeholder_values.values_mut() { + // Note, no need to check if p.range.file is equal to m.range.file, since we + // already know we're within `existing`. + if p.range.range.contains_range(m.range.range) { + // Convert the inner matches in `p` into a temporary MatchCollector. When + // we're done, we then convert it back into an SsrMatches. If we expected + // lots of inner matches, it might be worthwhile keeping a MatchCollector + // around for each placeholder match. However we expect most placeholder + // will have 0 and a few will have 1. More than that should hopefully be + // exceptional. + let mut collector = MatchCollector::default(); + for m in std::mem::replace(&mut p.inner_matches.matches, Vec::new()) { + collector.matches_by_node.insert(m.matched_node.clone(), m); + } + collector.add_match(m, sema); + p.inner_matches = collector.into(); + break; + } + } +} + +impl From for SsrMatches { + fn from(mut match_collector: MatchCollector) -> Self { + let mut matches = SsrMatches::default(); + for (_, m) in match_collector.matches_by_node.drain() { + matches.matches.push(m); + } + matches.matches.sort_by(|a, b| { + // Order matches by file_id then by start range. This should be sufficient since ranges + // shouldn't be overlapping. + a.range + .file_id + .cmp(&b.range.file_id) + .then_with(|| a.range.range.start().cmp(&b.range.range.start())) + }); + matches + } +} diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index ec3addcf89..a28e9f3414 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -1,17 +1,20 @@ //! Searching for matches. -use crate::{matching, Match, MatchFinder}; +use crate::{matching, parsing::ParsedRule, Match, MatchFinder}; use ra_db::FileRange; use ra_syntax::{ast, AstNode, SyntaxNode}; impl<'db> MatchFinder<'db> { - pub(crate) fn find_all_matches(&self, matches_out: &mut Vec) { + /// Adds all matches for `rule` to `matches_out`. Matches may overlap in ways that make + /// replacement impossible, so further processing is required in order to properly nest matches + /// and remove overlapping matches. This is done in the `nesting` module. + pub(crate) fn find_matches_for_rule(&self, rule: &ParsedRule, matches_out: &mut Vec) { // FIXME: Use resolved paths in the pattern to find places to search instead of always // scanning every node. - self.slow_scan(matches_out); + self.slow_scan(rule, matches_out); } - fn slow_scan(&self, matches_out: &mut Vec) { + fn slow_scan(&self, rule: &ParsedRule, matches_out: &mut Vec) { use ra_db::SourceDatabaseExt; use ra_ide_db::symbol_index::SymbolsDatabase; for &root in self.sema.db.local_roots().iter() { @@ -19,7 +22,7 @@ impl<'db> MatchFinder<'db> { for file_id in sr.iter() { let file = self.sema.parse(file_id); let code = file.syntax(); - self.slow_scan_node(code, &None, matches_out); + self.slow_scan_node(code, rule, &None, matches_out); } } } @@ -27,28 +30,12 @@ impl<'db> MatchFinder<'db> { fn slow_scan_node( &self, code: &SyntaxNode, + rule: &ParsedRule, restrict_range: &Option, matches_out: &mut Vec, ) { - for rule in &self.rules { - if let Ok(mut m) = matching::get_match(false, rule, &code, restrict_range, &self.sema) { - // Continue searching in each of our placeholders. - for placeholder_value in m.placeholder_values.values_mut() { - if let Some(placeholder_node) = &placeholder_value.node { - // Don't search our placeholder if it's the entire matched node, otherwise we'd - // find the same match over and over until we got a stack overflow. - if placeholder_node != code { - self.slow_scan_node( - placeholder_node, - restrict_range, - &mut placeholder_value.inner_matches.matches, - ); - } - } - } - matches_out.push(m); - return; - } + if let Ok(m) = matching::get_match(false, rule, &code, restrict_range, &self.sema) { + matches_out.push(m); } // 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. @@ -60,6 +47,7 @@ impl<'db> MatchFinder<'db> { // i.e. we don't want to match something that came from the macro itself. self.slow_scan_node( &expanded, + rule, &Some(self.sema.original_range(tt.syntax())), matches_out, ); @@ -67,7 +55,7 @@ impl<'db> MatchFinder<'db> { } } for child in code.children() { - self.slow_scan_node(&child, restrict_range, matches_out); + self.slow_scan_node(&child, rule, restrict_range, matches_out); } } } From 3975952601888d9f77e466c12e8e389748984b33 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 22 Jul 2020 15:00:28 +1000 Subject: [PATCH 09/14] SSR: Pass current file position through to SSR code. In a subsequent commit, it will be used for resolving paths. --- crates/ra_ide/src/lib.rs | 3 +- crates/ra_ide/src/ssr.rs | 6 ++-- crates/ra_ssr/src/lib.rs | 30 ++++++++++++++++++-- crates/ra_ssr/src/matching.rs | 4 +-- crates/ra_ssr/src/tests.rs | 42 ++++++++++++++++------------ crates/rust-analyzer/src/cli/ssr.rs | 4 +-- crates/rust-analyzer/src/handlers.rs | 3 +- crates/rust-analyzer/src/lsp_ext.rs | 5 ++++ docs/dev/lsp-extensions.md | 7 ++++- editors/code/src/commands.ts | 14 ++++++++-- editors/code/src/lsp_ext.ts | 2 ++ 11 files changed, 88 insertions(+), 32 deletions(-) diff --git a/crates/ra_ide/src/lib.rs b/crates/ra_ide/src/lib.rs index dc9192d42c..7356e947b9 100644 --- a/crates/ra_ide/src/lib.rs +++ b/crates/ra_ide/src/lib.rs @@ -505,9 +505,10 @@ impl Analysis { &self, query: &str, parse_only: bool, + position: FilePosition, ) -> Cancelable> { self.with_db(|db| { - let edits = ssr::parse_search_replace(query, parse_only, db)?; + let edits = ssr::parse_search_replace(query, parse_only, db, position)?; Ok(SourceChange::from(edits)) }) } diff --git a/crates/ra_ide/src/ssr.rs b/crates/ra_ide/src/ssr.rs index ca7e0ad866..3e2705d62b 100644 --- a/crates/ra_ide/src/ssr.rs +++ b/crates/ra_ide/src/ssr.rs @@ -1,3 +1,4 @@ +use ra_db::FilePosition; use ra_ide_db::RootDatabase; use crate::SourceFileEdit; @@ -42,12 +43,13 @@ pub fn parse_search_replace( rule: &str, parse_only: bool, db: &RootDatabase, + position: FilePosition, ) -> Result, SsrError> { let rule: SsrRule = rule.parse()?; + let mut match_finder = MatchFinder::in_context(db, position); + match_finder.add_rule(rule); if parse_only { return Ok(Vec::new()); } - let mut match_finder = MatchFinder::new(db); - match_finder.add_rule(rule); Ok(match_finder.edits()) } diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index 6d578610b4..a0a5c97627 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -13,11 +13,12 @@ mod errors; #[cfg(test)] mod tests; +use crate::errors::bail; pub use crate::errors::SsrError; pub use crate::matching::Match; use crate::matching::MatchFailureReason; use hir::Semantics; -use ra_db::{FileId, FileRange}; +use ra_db::{FileId, FilePosition, FileRange}; use ra_ide_db::source_change::SourceFileEdit; use ra_syntax::{ast, AstNode, SyntaxNode, TextRange}; use rustc_hash::FxHashMap; @@ -51,10 +52,35 @@ pub struct MatchFinder<'db> { } impl<'db> MatchFinder<'db> { - pub fn new(db: &'db ra_ide_db::RootDatabase) -> MatchFinder<'db> { + /// Constructs a new instance where names will be looked up as if they appeared at + /// `lookup_context`. + pub fn in_context( + db: &'db ra_ide_db::RootDatabase, + _lookup_context: FilePosition, + ) -> MatchFinder<'db> { + // FIXME: Use lookup_context MatchFinder { sema: Semantics::new(db), rules: Vec::new() } } + /// Constructs an instance using the start of the first file in `db` as the lookup context. + pub fn at_first_file(db: &'db ra_ide_db::RootDatabase) -> Result, SsrError> { + use ra_db::SourceDatabaseExt; + use ra_ide_db::symbol_index::SymbolsDatabase; + if let Some(first_file_id) = db + .local_roots() + .iter() + .next() + .and_then(|root| db.source_root(root.clone()).iter().next()) + { + Ok(MatchFinder::in_context( + db, + FilePosition { file_id: first_file_id, offset: 0.into() }, + )) + } else { + bail!("No files to search"); + } + } + /// Adds a rule to be applied. The order in which rules are added matters. Earlier rules take /// precedence. If a node is matched by an earlier rule, then later rules won't be permitted to /// match to it. diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index 005569f6fa..a43d57c342 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -576,8 +576,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, _) = crate::tests::single_file(input); - let mut match_finder = MatchFinder::new(&db); + let (db, position) = crate::tests::single_file(input); + let mut match_finder = MatchFinder::in_context(&db, position); match_finder.add_rule(rule); let matches = match_finder.matches(); assert_eq!(matches.matches.len(), 1); diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 523035b310..63d5278947 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -1,6 +1,6 @@ use crate::{MatchFinder, SsrRule}; use expect::{expect, Expect}; -use ra_db::{salsa::Durability, FileId, SourceDatabaseExt}; +use ra_db::{salsa::Durability, FileId, FilePosition, SourceDatabaseExt}; use rustc_hash::FxHashSet; use std::sync::Arc; use test_utils::mark; @@ -59,15 +59,21 @@ fn parser_undefined_placeholder_in_replacement() { ); } -pub(crate) fn single_file(code: &str) -> (ra_ide_db::RootDatabase, FileId) { +/// `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) { use ra_db::fixture::WithFixture; use ra_ide_db::symbol_index::SymbolsDatabase; - let (db, file_id) = ra_ide_db::RootDatabase::with_single_file(code); - let mut db = db; + let (mut db, position) = if code.contains(test_utils::CURSOR_MARKER) { + ra_ide_db::RootDatabase::with_position(code) + } else { + let (db, file_id) = ra_ide_db::RootDatabase::with_single_file(code); + (db, FilePosition { file_id, offset: 0.into() }) + }; 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, file_id) + (db, position) } fn assert_ssr_transform(rule: &str, input: &str, expected: Expect) { @@ -75,8 +81,8 @@ fn assert_ssr_transform(rule: &str, input: &str, expected: Expect) { } fn assert_ssr_transforms(rules: &[&str], input: &str, expected: Expect) { - let (db, file_id) = single_file(input); - let mut match_finder = MatchFinder::new(&db); + let (db, position) = single_file(input); + let mut match_finder = MatchFinder::in_context(&db, position); for rule in rules { let rule: SsrRule = rule.parse().unwrap(); match_finder.add_rule(rule); @@ -85,10 +91,10 @@ fn assert_ssr_transforms(rules: &[&str], input: &str, expected: Expect) { if edits.is_empty() { panic!("No edits were made"); } - assert_eq!(edits[0].file_id, file_id); + assert_eq!(edits[0].file_id, position.file_id); // Note, db.file_text is not necessarily the same as `input`, since fixture parsing alters // stuff. - let mut actual = db.file_text(file_id).to_string(); + let mut actual = db.file_text(position.file_id).to_string(); edits[0].edit.apply(&mut actual); expected.assert_eq(&actual); } @@ -106,34 +112,34 @@ fn print_match_debug_info(match_finder: &MatchFinder, file_id: FileId, snippet: } fn assert_matches(pattern: &str, code: &str, expected: &[&str]) { - let (db, file_id) = single_file(code); - let mut match_finder = MatchFinder::new(&db); + let (db, position) = single_file(code); + let mut match_finder = MatchFinder::in_context(&db, position); match_finder.add_search_pattern(pattern.parse().unwrap()); let matched_strings: Vec = match_finder.matches().flattened().matches.iter().map(|m| m.matched_text()).collect(); if matched_strings != expected && !expected.is_empty() { - print_match_debug_info(&match_finder, file_id, &expected[0]); + print_match_debug_info(&match_finder, position.file_id, &expected[0]); } assert_eq!(matched_strings, expected); } fn assert_no_match(pattern: &str, code: &str) { - let (db, file_id) = single_file(code); - let mut match_finder = MatchFinder::new(&db); + let (db, position) = single_file(code); + let mut match_finder = MatchFinder::in_context(&db, position); match_finder.add_search_pattern(pattern.parse().unwrap()); let matches = match_finder.matches().flattened().matches; if !matches.is_empty() { - print_match_debug_info(&match_finder, file_id, &matches[0].matched_text()); + print_match_debug_info(&match_finder, position.file_id, &matches[0].matched_text()); panic!("Got {} matches when we expected none: {:#?}", matches.len(), matches); } } fn assert_match_failure_reason(pattern: &str, code: &str, snippet: &str, expected_reason: &str) { - let (db, file_id) = single_file(code); - let mut match_finder = MatchFinder::new(&db); + let (db, position) = single_file(code); + let mut match_finder = MatchFinder::in_context(&db, position); match_finder.add_search_pattern(pattern.parse().unwrap()); let mut reasons = Vec::new(); - for d in match_finder.debug_where_text_equal(file_id, snippet) { + for d in match_finder.debug_where_text_equal(position.file_id, snippet) { if let Some(reason) = d.match_failure_reason() { reasons.push(reason.to_owned()); } diff --git a/crates/rust-analyzer/src/cli/ssr.rs b/crates/rust-analyzer/src/cli/ssr.rs index 014bc70a45..22f5b4be04 100644 --- a/crates/rust-analyzer/src/cli/ssr.rs +++ b/crates/rust-analyzer/src/cli/ssr.rs @@ -7,7 +7,7 @@ pub fn apply_ssr_rules(rules: Vec) -> Result<()> { use ra_db::SourceDatabaseExt; let (host, vfs) = load_cargo(&std::env::current_dir()?, true, true)?; let db = host.raw_database(); - let mut match_finder = MatchFinder::new(db); + let mut match_finder = MatchFinder::at_first_file(db)?; for rule in rules { match_finder.add_rule(rule); } @@ -30,7 +30,7 @@ pub fn search_for_patterns(patterns: Vec, debug_snippet: Option Result { let _p = profile("handle_ssr"); + let position = from_proto::file_position(&snap, params.position)?; let source_change = - snap.analysis.structural_search_replace(¶ms.query, params.parse_only)??; + snap.analysis.structural_search_replace(¶ms.query, params.parse_only, position)??; 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 13ebb18fbb..113e0e070a 100644 --- a/crates/rust-analyzer/src/lsp_ext.rs +++ b/crates/rust-analyzer/src/lsp_ext.rs @@ -216,6 +216,11 @@ impl Request for Ssr { pub struct SsrParams { pub query: String, pub parse_only: bool, + + /// File position where SSR was invoked. Paths in `query` will be resolved relative to this + /// position. + #[serde(flatten)] + pub position: lsp_types::TextDocumentPositionParams, } pub enum StatusNotification {} diff --git a/docs/dev/lsp-extensions.md b/docs/dev/lsp-extensions.md index 98d14450b1..1be01fd884 100644 --- a/docs/dev/lsp-extensions.md +++ b/docs/dev/lsp-extensions.md @@ -274,6 +274,11 @@ interface SsrParams { query: string, /// If true, only check the syntax of the query and don't compute the actual edit. parseOnly: bool, + /// The current text document. This and `position` will be used to determine in what scope + /// paths in `query` should be resolved. + textDocument: lc.TextDocumentIdentifier; + /// Position where SSR was invoked. + position: lc.Position; } ``` @@ -285,7 +290,7 @@ WorkspaceEdit ### Example -SSR with query `foo($a:expr, $b:expr) ==>> ($a).foo($b)` will transform, eg `foo(y + 5, z)` into `(y + 5).foo(z)`. +SSR with query `foo($a, $b) ==>> ($a).foo($b)` will transform, eg `foo(y + 5, z)` into `(y + 5).foo(z)`. ### Unresolved Question diff --git a/editors/code/src/commands.ts b/editors/code/src/commands.ts index 1f3a7cf7e8..3ae995705f 100644 --- a/editors/code/src/commands.ts +++ b/editors/code/src/commands.ts @@ -185,15 +185,21 @@ export function parentModule(ctx: Ctx): Cmd { export function ssr(ctx: Ctx): Cmd { return async () => { + const editor = vscode.window.activeTextEditor; const client = ctx.client; - if (!client) return; + if (!editor || !client) return; + + const position = editor.selection.active; + let textDocument = { uri: editor.document.uri.toString() }; const options: vscode.InputBoxOptions = { value: "() ==>> ()", prompt: "Enter request, for example 'Foo($a) ==> Foo::new($a)' ", validateInput: async (x: string) => { try { - await client.sendRequest(ra.ssr, { query: x, parseOnly: true }); + await client.sendRequest(ra.ssr, { + query: x, parseOnly: true, textDocument, position, + }); } catch (e) { return e.toString(); } @@ -208,7 +214,9 @@ export function ssr(ctx: Ctx): Cmd { title: "Structured search replace in progress...", cancellable: false, }, async (_progress, _token) => { - const edit = await client.sendRequest(ra.ssr, { query: request, parseOnly: false }); + const edit = await client.sendRequest(ra.ssr, { + query: request, parseOnly: false, textDocument, position + }); await vscode.workspace.applyEdit(client.protocol2CodeConverter.asWorkspaceEdit(edit)); }); diff --git a/editors/code/src/lsp_ext.ts b/editors/code/src/lsp_ext.ts index 5f32cb40e9..149f9a0d64 100644 --- a/editors/code/src/lsp_ext.ts +++ b/editors/code/src/lsp_ext.ts @@ -93,6 +93,8 @@ export const inlayHints = new lc.RequestType('experimental/ssr'); From 757f755c29e041fd319af466d7d0418f54cb090a Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 22 Jul 2020 16:46:29 +1000 Subject: [PATCH 10/14] SSR: Match paths based on what they resolve to Also render template paths appropriately for their context. --- crates/ra_ide/src/ssr.rs | 12 ++- crates/ra_ssr/src/lib.rs | 61 +++++++---- crates/ra_ssr/src/matching.rs | 106 +++++++++++++++++-- crates/ra_ssr/src/parsing.rs | 17 +--- crates/ra_ssr/src/replacing.rs | 40 ++++++-- crates/ra_ssr/src/resolving.rs | 153 ++++++++++++++++++++++++++++ crates/ra_ssr/src/search.rs | 8 +- crates/ra_ssr/src/tests.rs | 142 +++++++++++++++++++++++++- crates/rust-analyzer/src/cli/ssr.rs | 4 +- 9 files changed, 482 insertions(+), 61 deletions(-) create mode 100644 crates/ra_ssr/src/resolving.rs diff --git a/crates/ra_ide/src/ssr.rs b/crates/ra_ide/src/ssr.rs index 3e2705d62b..2f40bac08b 100644 --- a/crates/ra_ide/src/ssr.rs +++ b/crates/ra_ide/src/ssr.rs @@ -11,6 +11,16 @@ use ra_ssr::{MatchFinder, SsrError, SsrRule}; // A `$` placeholder in the search pattern will match any AST node and `$` will reference it in the replacement. // Within a macro call, a placeholder will match up until whatever token follows the placeholder. // +// All paths in both the search pattern and the replacement template must resolve in the context +// in which this command is invoked. Paths in the search pattern will then match the code if they +// resolve to the same item, even if they're written differently. For example if we invoke the +// command in the module `foo` with a pattern of `Bar`, then code in the parent module that refers +// to `foo::Bar` will match. +// +// Paths in the replacement template will be rendered appropriately for the context in which the +// replacement occurs. For example if our replacement template is `foo::Bar` and we match some +// code in the `foo` module, we'll insert just `Bar`. +// // Placeholders may be given constraints by writing them as `${::...}`. // // Supported constraints: @@ -47,7 +57,7 @@ pub fn parse_search_replace( ) -> Result, SsrError> { let rule: SsrRule = rule.parse()?; let mut match_finder = MatchFinder::in_context(db, position); - match_finder.add_rule(rule); + 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 a0a5c97627..286619f59a 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -7,6 +7,7 @@ mod matching; mod nester; mod parsing; mod replacing; +mod resolving; mod search; #[macro_use] mod errors; @@ -21,6 +22,7 @@ use hir::Semantics; use ra_db::{FileId, FilePosition, FileRange}; use ra_ide_db::source_change::SourceFileEdit; use ra_syntax::{ast, AstNode, SyntaxNode, TextRange}; +use resolving::ResolvedRule; use rustc_hash::FxHashMap; // A structured search replace rule. Create by calling `parse` on a str. @@ -48,7 +50,9 @@ pub struct SsrMatches { pub struct MatchFinder<'db> { /// Our source of information about the user's code. sema: Semantics<'db, ra_ide_db::RootDatabase>, - rules: Vec, + rules: Vec, + scope: hir::SemanticsScope<'db>, + hygiene: hir::Hygiene, } impl<'db> MatchFinder<'db> { @@ -56,10 +60,24 @@ impl<'db> MatchFinder<'db> { /// `lookup_context`. pub fn in_context( db: &'db ra_ide_db::RootDatabase, - _lookup_context: FilePosition, + lookup_context: FilePosition, ) -> MatchFinder<'db> { - // FIXME: Use lookup_context - MatchFinder { sema: Semantics::new(db), rules: Vec::new() } + let sema = Semantics::new(db); + let file = sema.parse(lookup_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) + .left_biased() + .map(|token| token.parent()) + .unwrap_or_else(|| file.syntax().clone()); + let scope = sema.scope(&node); + MatchFinder { + sema: Semantics::new(db), + rules: Vec::new(), + scope, + hygiene: hir::Hygiene::new(db, lookup_context.file_id.into()), + } } /// Constructs an instance using the start of the first file in `db` as the lookup context. @@ -84,8 +102,16 @@ impl<'db> MatchFinder<'db> { /// Adds a rule to be applied. The order in which rules are added matters. Earlier rules take /// precedence. If a node is matched by an earlier rule, then later rules won't be permitted to /// match to it. - pub fn add_rule(&mut self, rule: SsrRule) { - self.add_parsed_rules(rule.parsed_rules); + pub fn add_rule(&mut self, rule: SsrRule) -> Result<(), SsrError> { + for parsed_rule in rule.parsed_rules { + self.rules.push(ResolvedRule::new( + parsed_rule, + &self.scope, + &self.hygiene, + self.rules.len(), + )?); + } + Ok(()) } /// Finds matches for all added rules and returns edits for all found matches. @@ -110,8 +136,16 @@ impl<'db> MatchFinder<'db> { /// Adds a search pattern. For use if you intend to only call `find_matches_in_file`. If you /// intend to do replacement, use `add_rule` instead. - pub fn add_search_pattern(&mut self, pattern: SsrPattern) { - self.add_parsed_rules(pattern.parsed_rules); + pub fn add_search_pattern(&mut self, pattern: SsrPattern) -> Result<(), SsrError> { + for parsed_rule in pattern.parsed_rules { + self.rules.push(ResolvedRule::new( + parsed_rule, + &self.scope, + &self.hygiene, + self.rules.len(), + )?); + } + Ok(()) } /// Returns matches for all added rules. @@ -149,13 +183,6 @@ impl<'db> MatchFinder<'db> { res } - fn add_parsed_rules(&mut self, parsed_rules: Vec) { - for mut parsed_rule in parsed_rules { - parsed_rule.index = self.rules.len(); - self.rules.push(parsed_rule); - } - } - fn output_debug_for_nodes_at_range( &self, node: &SyntaxNode, @@ -175,7 +202,7 @@ impl<'db> MatchFinder<'db> { // we get lots of noise. If at some point we add support for restricting rules // to a particular kind of thing (e.g. only match type references), then we can // relax this. - if rule.pattern.kind() != node.kind() { + if rule.pattern.node.kind() != node.kind() { continue; } out.push(MatchDebugInfo { @@ -185,7 +212,7 @@ impl<'db> MatchFinder<'db> { "Match failed, but no reason was given".to_owned() }), }), - pattern: rule.pattern.clone(), + pattern: rule.pattern.node.clone(), node: node.clone(), }); } diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index a43d57c342..f3cc60c29d 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -2,7 +2,8 @@ //! process of matching, placeholder values are recorded. use crate::{ - parsing::{Constraint, NodeKind, ParsedRule, Placeholder}, + parsing::{Constraint, NodeKind, Placeholder}, + resolving::{ResolvedPattern, ResolvedRule}, SsrMatches, }; use hir::Semantics; @@ -51,6 +52,8 @@ pub struct Match { pub(crate) rule_index: usize, /// The depth of matched_node. pub(crate) depth: usize, + // Each path in the template rendered for the module in which the match was found. + pub(crate) rendered_template_paths: FxHashMap, } /// Represents a `$var` in an SSR query. @@ -86,7 +89,7 @@ pub(crate) struct MatchFailed { /// parent module, we don't populate nested matches. pub(crate) fn get_match( debug_active: bool, - rule: &ParsedRule, + rule: &ResolvedRule, code: &SyntaxNode, restrict_range: &Option, sema: &Semantics, @@ -102,7 +105,7 @@ struct Matcher<'db, 'sema> { /// If any placeholders come from anywhere outside of this range, then the match will be /// rejected. restrict_range: Option, - rule: &'sema ParsedRule, + rule: &'sema ResolvedRule, } /// Which phase of matching we're currently performing. We do two phases because most attempted @@ -117,14 +120,14 @@ enum Phase<'a> { impl<'db, 'sema> Matcher<'db, 'sema> { fn try_match( - rule: &ParsedRule, + rule: &ResolvedRule, code: &SyntaxNode, restrict_range: &Option, sema: &'sema Semantics<'db, ra_ide_db::RootDatabase>, ) -> Result { let match_state = Matcher { sema, restrict_range: restrict_range.clone(), rule }; // First pass at matching, where we check that node types and idents match. - match_state.attempt_match_node(&mut Phase::First, &rule.pattern, code)?; + match_state.attempt_match_node(&mut Phase::First, &rule.pattern.node, code)?; match_state.validate_range(&sema.original_range(code))?; let mut the_match = Match { range: sema.original_range(code), @@ -133,11 +136,19 @@ impl<'db, 'sema> Matcher<'db, 'sema> { ignored_comments: Vec::new(), rule_index: rule.index, depth: 0, + rendered_template_paths: FxHashMap::default(), }; // Second matching pass, where we record placeholder matches, ignored comments and maybe do // any other more expensive checks that we didn't want to do on the first pass. - match_state.attempt_match_node(&mut Phase::Second(&mut the_match), &rule.pattern, code)?; + match_state.attempt_match_node( + &mut Phase::Second(&mut the_match), + &rule.pattern.node, + code, + )?; the_match.depth = sema.ancestors_with_macros(the_match.matched_node.clone()).count(); + if let Some(template) = &rule.template { + the_match.render_template_paths(template, sema)?; + } Ok(the_match) } @@ -195,6 +206,7 @@ impl<'db, 'sema> Matcher<'db, 'sema> { self.attempt_match_record_field_list(phase, pattern, code) } SyntaxKind::TOKEN_TREE => self.attempt_match_token_tree(phase, pattern, code), + SyntaxKind::PATH => self.attempt_match_path(phase, pattern, code), _ => self.attempt_match_node_children(phase, pattern, code), } } @@ -311,6 +323,64 @@ impl<'db, 'sema> Matcher<'db, 'sema> { Ok(()) } + /// Paths are matched based on whether they refer to the same thing, even if they're written + /// differently. + fn attempt_match_path( + &self, + phase: &mut Phase, + pattern: &SyntaxNode, + code: &SyntaxNode, + ) -> Result<(), MatchFailed> { + if let Some(pattern_resolved) = self.rule.pattern.resolved_paths.get(pattern) { + let pattern_path = ast::Path::cast(pattern.clone()).unwrap(); + let code_path = ast::Path::cast(code.clone()).unwrap(); + if let (Some(pattern_segment), Some(code_segment)) = + (pattern_path.segment(), code_path.segment()) + { + // Match everything within the segment except for the name-ref, which is handled + // separately via comparing what the path resolves to below. + self.attempt_match_opt( + phase, + pattern_segment.type_arg_list(), + code_segment.type_arg_list(), + )?; + self.attempt_match_opt( + phase, + pattern_segment.param_list(), + code_segment.param_list(), + )?; + } + if matches!(phase, Phase::Second(_)) { + let resolution = self + .sema + .resolve_path(&code_path) + .ok_or_else(|| match_error!("Failed to resolve path `{}`", code.text()))?; + if pattern_resolved.resolution != resolution { + fail_match!("Pattern had path `{}` code had `{}`", pattern.text(), code.text()); + } + } + } else { + return self.attempt_match_node_children(phase, pattern, code); + } + Ok(()) + } + + fn attempt_match_opt( + &self, + phase: &mut Phase, + pattern: Option, + code: Option, + ) -> Result<(), MatchFailed> { + match (pattern, code) { + (Some(p), Some(c)) => self.attempt_match_node(phase, &p.syntax(), &c.syntax()), + (None, None) => Ok(()), + (Some(p), None) => fail_match!("Pattern `{}` had nothing to match", p.syntax().text()), + (None, Some(c)) => { + fail_match!("Nothing in pattern to match code `{}`", c.syntax().text()) + } + } + } + /// We want to allow the records to match in any order, so we have special matching logic for /// them. fn attempt_match_record_field_list( @@ -449,6 +519,28 @@ impl<'db, 'sema> Matcher<'db, 'sema> { } } +impl Match { + fn render_template_paths( + &mut self, + template: &ResolvedPattern, + sema: &Semantics, + ) -> Result<(), MatchFailed> { + let module = sema + .scope(&self.matched_node) + .module() + .ok_or_else(|| match_error!("Matched node isn't in a module"))?; + for (path, resolved_path) in &template.resolved_paths { + if let hir::PathResolution::Def(module_def) = resolved_path.resolution { + let mod_path = module.find_use_path(sema.db, module_def).ok_or_else(|| { + match_error!("Failed to render template path `{}` at match location") + })?; + self.rendered_template_paths.insert(path.clone(), mod_path); + } + } + Ok(()) + } +} + impl Phase<'_> { fn next_non_trivial(&mut self, code_it: &mut SyntaxElementChildren) -> Option { loop { @@ -578,7 +670,7 @@ mod tests { let (db, position) = crate::tests::single_file(input); let mut match_finder = MatchFinder::in_context(&db, position); - match_finder.add_rule(rule); + match_finder.add_rule(rule).unwrap(); let matches = match_finder.matches(); assert_eq!(matches.matches.len(), 1); assert_eq!(matches.matches[0].matched_node.text(), "foo(1+2)"); diff --git a/crates/ra_ssr/src/parsing.rs b/crates/ra_ssr/src/parsing.rs index cf7fb517fa..2d6f4e514f 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -7,7 +7,7 @@ use crate::errors::bail; use crate::{SsrError, SsrPattern, SsrRule}; -use ra_syntax::{ast, AstNode, SmolStr, SyntaxKind, SyntaxNode, SyntaxToken, T}; +use ra_syntax::{ast, AstNode, SmolStr, SyntaxKind, SyntaxNode, T}; use rustc_hash::{FxHashMap, FxHashSet}; use std::str::FromStr; @@ -16,7 +16,6 @@ pub(crate) struct ParsedRule { pub(crate) placeholders_by_stand_in: FxHashMap, pub(crate) pattern: SyntaxNode, pub(crate) template: Option, - pub(crate) index: usize, } #[derive(Debug)] @@ -93,16 +92,11 @@ impl RuleBuilder { placeholders_by_stand_in: self.placeholders_by_stand_in.clone(), pattern: pattern.syntax().clone(), template: Some(template.syntax().clone()), - // For now we give the rule an index of 0. It's given a proper index when the rule - // is added to the SsrMatcher. Using an Option, instead would be slightly - // more correct, but we delete this field from ParsedRule in a subsequent commit. - index: 0, }), (Ok(pattern), None) => self.rules.push(ParsedRule { placeholders_by_stand_in: self.placeholders_by_stand_in.clone(), pattern: pattern.syntax().clone(), template: None, - index: 0, }), _ => {} } @@ -171,15 +165,6 @@ impl RawPattern { } } -impl ParsedRule { - pub(crate) fn get_placeholder(&self, token: &SyntaxToken) -> Option<&Placeholder> { - if token.kind() != SyntaxKind::IDENT { - return None; - } - self.placeholders_by_stand_in.get(token.text()) - } -} - impl FromStr for SsrPattern { type Err = SsrError; diff --git a/crates/ra_ssr/src/replacing.rs b/crates/ra_ssr/src/replacing.rs index f1c5bdf14e..4b3f5509c3 100644 --- a/crates/ra_ssr/src/replacing.rs +++ b/crates/ra_ssr/src/replacing.rs @@ -1,9 +1,9 @@ //! Code for applying replacement templates for matches that have previously been found. use crate::matching::Var; -use crate::{parsing::ParsedRule, Match, SsrMatches}; -use ra_syntax::ast::AstToken; -use ra_syntax::{SyntaxElement, SyntaxNode, SyntaxToken, TextSize}; +use crate::{resolving::ResolvedRule, Match, SsrMatches}; +use ra_syntax::ast::{self, AstToken}; +use ra_syntax::{SyntaxElement, SyntaxKind, SyntaxNode, SyntaxToken, TextSize}; use ra_text_edit::TextEdit; /// Returns a text edit that will replace each match in `matches` with its corresponding replacement @@ -12,7 +12,7 @@ use ra_text_edit::TextEdit; pub(crate) fn matches_to_edit( matches: &SsrMatches, file_src: &str, - rules: &[ParsedRule], + rules: &[ResolvedRule], ) -> TextEdit { matches_to_edit_at_offset(matches, file_src, 0.into(), rules) } @@ -21,7 +21,7 @@ fn matches_to_edit_at_offset( matches: &SsrMatches, file_src: &str, relative_start: TextSize, - rules: &[ParsedRule], + rules: &[ResolvedRule], ) -> TextEdit { let mut edit_builder = ra_text_edit::TextEditBuilder::default(); for m in &matches.matches { @@ -36,11 +36,11 @@ fn matches_to_edit_at_offset( struct ReplacementRenderer<'a> { match_info: &'a Match, file_src: &'a str, - rules: &'a [ParsedRule], - rule: &'a ParsedRule, + rules: &'a [ResolvedRule], + rule: &'a ResolvedRule, } -fn render_replace(match_info: &Match, file_src: &str, rules: &[ParsedRule]) -> String { +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 @@ -48,7 +48,7 @@ fn render_replace(match_info: &Match, file_src: &str, rules: &[ParsedRule]) -> S .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_children(&template, &mut out); + renderer.render_node(&template.node, &mut out); for comment in &match_info.ignored_comments { out.push_str(&comment.syntax().to_string()); } @@ -68,11 +68,31 @@ impl ReplacementRenderer<'_> { self.render_token(&token, out); } SyntaxElement::Node(child_node) => { - self.render_node_children(&child_node, out); + self.render_node(&child_node, out); } } } + fn render_node(&self, node: &SyntaxNode, out: &mut String) { + 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()); + // 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); + } + } + } + } + } else { + self.render_node_children(&node, out); + } + } + fn render_token(&self, token: &SyntaxToken, out: &mut String) { if let Some(placeholder) = self.rule.get_placeholder(&token) { if let Some(placeholder_value) = diff --git a/crates/ra_ssr/src/resolving.rs b/crates/ra_ssr/src/resolving.rs new file mode 100644 index 0000000000..e9d0521117 --- /dev/null +++ b/crates/ra_ssr/src/resolving.rs @@ -0,0 +1,153 @@ +//! This module is responsible for resolving paths within rules. + +use crate::errors::error; +use crate::{parsing, SsrError}; +use parsing::Placeholder; +use ra_syntax::{ast, SmolStr, SyntaxKind, SyntaxNode, SyntaxToken}; +use rustc_hash::{FxHashMap, FxHashSet}; +use test_utils::mark; + +pub(crate) struct ResolvedRule { + pub(crate) pattern: ResolvedPattern, + pub(crate) template: Option, + pub(crate) index: usize, +} + +pub(crate) struct ResolvedPattern { + pub(crate) placeholders_by_stand_in: FxHashMap, + pub(crate) node: SyntaxNode, + // Paths in `node` that we've resolved. + pub(crate) resolved_paths: FxHashMap, +} + +pub(crate) struct ResolvedPath { + pub(crate) resolution: hir::PathResolution, +} + +impl ResolvedRule { + pub(crate) fn new( + rule: parsing::ParsedRule, + scope: &hir::SemanticsScope, + hygiene: &hir::Hygiene, + index: usize, + ) -> Result { + let resolver = + Resolver { scope, hygiene, placeholders_by_stand_in: rule.placeholders_by_stand_in }; + let resolved_template = if let Some(template) = rule.template { + Some(resolver.resolve_pattern_tree(template)?) + } else { + None + }; + Ok(ResolvedRule { + pattern: resolver.resolve_pattern_tree(rule.pattern)?, + template: resolved_template, + index, + }) + } + + pub(crate) fn get_placeholder(&self, token: &SyntaxToken) -> Option<&Placeholder> { + if token.kind() != SyntaxKind::IDENT { + return None; + } + self.pattern.placeholders_by_stand_in.get(token.text()) + } +} + +struct Resolver<'a, 'db> { + scope: &'a hir::SemanticsScope<'db>, + hygiene: &'a hir::Hygiene, + placeholders_by_stand_in: FxHashMap, +} + +impl Resolver<'_, '_> { + fn resolve_pattern_tree(&self, pattern: SyntaxNode) -> Result { + let mut resolved_paths = FxHashMap::default(); + self.resolve(pattern.clone(), &mut resolved_paths)?; + Ok(ResolvedPattern { + node: pattern, + resolved_paths, + placeholders_by_stand_in: self.placeholders_by_stand_in.clone(), + }) + } + + fn resolve( + &self, + node: SyntaxNode, + resolved_paths: &mut FxHashMap, + ) -> Result<(), SsrError> { + use ra_syntax::ast::AstNode; + if let Some(path) = ast::Path::cast(node.clone()) { + // 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`. + if !path_contains_type_arguments(path.qualifier()) + && !self.path_contains_placeholder(&path) + { + let resolution = self + .resolve_path(&path) + .ok_or_else(|| error!("Failed to resolve path `{}`", node.text()))?; + resolved_paths.insert(node, ResolvedPath { resolution }); + return Ok(()); + } + } + for node in node.children() { + self.resolve(node, resolved_paths)?; + } + Ok(()) + } + + /// Returns whether `path` contains a placeholder, but ignores any placeholders within type + /// arguments. + fn path_contains_placeholder(&self, path: &ast::Path) -> bool { + if let Some(segment) = path.segment() { + if let Some(name_ref) = segment.name_ref() { + if self.placeholders_by_stand_in.contains_key(name_ref.text()) { + return true; + } + } + } + if let Some(qualifier) = path.qualifier() { + return self.path_contains_placeholder(&qualifier); + } + false + } + + fn resolve_path(&self, path: &ast::Path) -> Option { + let hir_path = hir::Path::from_src(path.clone(), self.hygiene)?; + // First try resolving the whole path. This will work for things like + // `std::collections::HashMap`, but will fail for things like + // `std::collections::HashMap::new`. + if let Some(resolution) = self.scope.resolve_hir_path(&hir_path) { + return Some(resolution); + } + // Resolution failed, try resolving the qualifier (e.g. `std::collections::HashMap` and if + // that succeeds, then iterate through the candidates on the resolved type with the provided + // name. + let resolved_qualifier = self.scope.resolve_hir_path_qualifier(&hir_path.qualifier()?)?; + if let hir::PathResolution::Def(hir::ModuleDef::Adt(adt)) = resolved_qualifier { + adt.ty(self.scope.db).iterate_path_candidates( + self.scope.db, + self.scope.module()?.krate(), + &FxHashSet::default(), + Some(hir_path.segments().last()?.name), + |_ty, assoc_item| Some(hir::PathResolution::AssocItem(assoc_item)), + ) + } else { + None + } + } +} + +/// Returns whether `path` or any of its qualifiers contains type arguments. +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() { + mark::hit!(type_arguments_within_path); + return true; + } + } + return path_contains_type_arguments(path.qualifier()); + } + false +} diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index a28e9f3414..ccc2d544ab 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -1,6 +1,6 @@ //! Searching for matches. -use crate::{matching, parsing::ParsedRule, Match, MatchFinder}; +use crate::{matching, resolving::ResolvedRule, Match, MatchFinder}; use ra_db::FileRange; use ra_syntax::{ast, AstNode, SyntaxNode}; @@ -8,13 +8,13 @@ impl<'db> MatchFinder<'db> { /// Adds all matches for `rule` to `matches_out`. Matches may overlap in ways that make /// replacement impossible, so further processing is required in order to properly nest matches /// and remove overlapping matches. This is done in the `nesting` module. - pub(crate) fn find_matches_for_rule(&self, rule: &ParsedRule, matches_out: &mut Vec) { + pub(crate) fn find_matches_for_rule(&self, rule: &ResolvedRule, matches_out: &mut Vec) { // FIXME: Use resolved paths in the pattern to find places to search instead of always // scanning every node. self.slow_scan(rule, matches_out); } - fn slow_scan(&self, rule: &ParsedRule, matches_out: &mut Vec) { + 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() { @@ -30,7 +30,7 @@ impl<'db> MatchFinder<'db> { fn slow_scan_node( &self, code: &SyntaxNode, - rule: &ParsedRule, + rule: &ResolvedRule, restrict_range: &Option, matches_out: &mut Vec, ) { diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 63d5278947..33742dc8e1 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -85,7 +85,7 @@ fn assert_ssr_transforms(rules: &[&str], input: &str, expected: Expect) { let mut match_finder = MatchFinder::in_context(&db, position); for rule in rules { let rule: SsrRule = rule.parse().unwrap(); - match_finder.add_rule(rule); + match_finder.add_rule(rule).unwrap(); } let edits = match_finder.edits(); if edits.is_empty() { @@ -114,7 +114,7 @@ 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); - match_finder.add_search_pattern(pattern.parse().unwrap()); + 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(); if matched_strings != expected && !expected.is_empty() { @@ -126,7 +126,7 @@ 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); - match_finder.add_search_pattern(pattern.parse().unwrap()); + match_finder.add_search_pattern(pattern.parse().unwrap()).unwrap(); let matches = match_finder.matches().flattened().matches; if !matches.is_empty() { print_match_debug_info(&match_finder, position.file_id, &matches[0].matched_text()); @@ -137,7 +137,7 @@ 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); - match_finder.add_search_pattern(pattern.parse().unwrap()); + 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) { if let Some(reason) = d.match_failure_reason() { @@ -350,6 +350,60 @@ fn match_pattern() { assert_matches("Some($a)", "struct Some(); fn f() {if let Some(x) = foo() {}}", &["Some(x)"]); } +// If our pattern has a full path, e.g. a::b::c() and the code has c(), but c resolves to +// a::b::c, then we should match. +#[test] +fn match_fully_qualified_fn_path() { + let code = r#" + mod a { + pub mod b { + pub fn c(_: i32) {} + } + } + use a::b::c; + fn f1() { + c(42); + } + "#; + assert_matches("a::b::c($a)", code, &["c(42)"]); +} + +#[test] +fn match_resolved_type_name() { + let code = r#" + mod m1 { + pub mod m2 { + pub trait Foo {} + } + } + mod m3 { + trait Foo {} + fn f1(f: Option<&dyn Foo>) {} + } + mod m4 { + use crate::m1::m2::Foo; + fn f1(f: Option<&dyn Foo>) {} + } + "#; + assert_matches("m1::m2::Foo<$t>", code, &["Foo"]); +} + +#[test] +fn type_arguments_within_path() { + mark::check!(type_arguments_within_path); + let code = r#" + mod foo { + pub struct Bar {t: T} + impl Bar { + pub fn baz() {} + } + } + fn f1() {foo::Bar::::baz();} + "#; + assert_no_match("foo::Bar::::baz()", code); + assert_matches("foo::Bar::::baz()", code, &["foo::Bar::::baz()"]); +} + #[test] fn literal_constraint() { mark::check!(literal_constraint); @@ -482,6 +536,86 @@ fn replace_associated_function_call() { ); } +#[test] +fn replace_path_in_different_contexts() { + // Note the <|> inside module a::b which marks the point where the rule is interpreted. We + // replace foo with bar, but both need different path qualifiers in different contexts. In f4, + // foo is unqualified because of a use statement, however the replacement needs to be fully + // qualified. + assert_ssr_transform( + "c::foo() ==>> c::bar()", + r#" + mod a { + pub mod b {<|> + pub mod c { + pub fn foo() {} + pub fn bar() {} + fn f1() { foo() } + } + fn f2() { c::foo() } + } + fn f3() { b::c::foo() } + } + use a::b::c::foo; + fn f4() { foo() } + "#, + expect![[r#" + mod a { + pub mod b { + pub mod c { + pub fn foo() {} + pub fn bar() {} + fn f1() { bar() } + } + fn f2() { c::bar() } + } + fn f3() { b::c::bar() } + } + use a::b::c::foo; + fn f4() { a::b::c::bar() } + "#]], + ); +} + +#[test] +fn replace_associated_function_with_generics() { + assert_ssr_transform( + "c::Foo::<$a>::new() ==>> d::Bar::<$a>::default()", + r#" + mod c { + pub struct Foo {v: T} + impl Foo { pub fn new() {} } + fn f1() { + Foo::::new(); + } + } + mod d { + pub struct Bar {v: T} + impl Bar { pub fn default() {} } + fn f1() { + super::c::Foo::::new(); + } + } + "#, + expect![[r#" + mod c { + pub struct Foo {v: T} + impl Foo { pub fn new() {} } + fn f1() { + crate::d::Bar::::default(); + } + } + mod d { + pub struct Bar {v: T} + impl Bar { pub fn default() {} } + fn f1() { + Bar::::default(); + } + } + "#]], + ); +} + #[test] fn replace_type() { assert_ssr_transform( diff --git a/crates/rust-analyzer/src/cli/ssr.rs b/crates/rust-analyzer/src/cli/ssr.rs index 22f5b4be04..194bec008d 100644 --- a/crates/rust-analyzer/src/cli/ssr.rs +++ b/crates/rust-analyzer/src/cli/ssr.rs @@ -9,7 +9,7 @@ pub fn apply_ssr_rules(rules: Vec) -> Result<()> { let db = host.raw_database(); let mut match_finder = MatchFinder::at_first_file(db)?; for rule in rules { - match_finder.add_rule(rule); + match_finder.add_rule(rule)?; } let edits = match_finder.edits(); for edit in edits { @@ -32,7 +32,7 @@ pub fn search_for_patterns(patterns: Vec, debug_snippet: Option Date: Wed, 22 Jul 2020 14:01:21 +1000 Subject: [PATCH 11/14] SSR: Use Definition::find_usages to speed up matching. When the search pattern contains a path, this substantially speeds up finding matches, especially if the path references a private item. --- crates/ra_ide_db/src/defs.rs | 37 ++++++---- crates/ra_ide_db/src/search.rs | 4 + crates/ra_ssr/src/lib.rs | 3 +- crates/ra_ssr/src/resolving.rs | 8 +- crates/ra_ssr/src/search.rs | 131 +++++++++++++++++++++++++++++++-- 5 files changed, 158 insertions(+), 25 deletions(-) diff --git a/crates/ra_ide_db/src/defs.rs b/crates/ra_ide_db/src/defs.rs index e06b189a00..f391a8e432 100644 --- a/crates/ra_ide_db/src/defs.rs +++ b/crates/ra_ide_db/src/defs.rs @@ -290,20 +290,25 @@ pub fn classify_name_ref( let path = name_ref.syntax().ancestors().find_map(ast::Path::cast)?; let resolved = sema.resolve_path(&path)?; - let res = match resolved { - PathResolution::Def(def) => Definition::ModuleDef(def), - PathResolution::AssocItem(item) => { - let def = match item { - hir::AssocItem::Function(it) => it.into(), - hir::AssocItem::Const(it) => it.into(), - hir::AssocItem::TypeAlias(it) => it.into(), - }; - Definition::ModuleDef(def) - } - PathResolution::Local(local) => Definition::Local(local), - PathResolution::TypeParam(par) => Definition::TypeParam(par), - PathResolution::Macro(def) => Definition::Macro(def), - PathResolution::SelfType(impl_def) => Definition::SelfType(impl_def), - }; - Some(NameRefClass::Definition(res)) + Some(NameRefClass::Definition(resolved.into())) +} + +impl From for Definition { + fn from(path_resolution: PathResolution) -> Self { + match path_resolution { + PathResolution::Def(def) => Definition::ModuleDef(def), + PathResolution::AssocItem(item) => { + let def = match item { + hir::AssocItem::Function(it) => it.into(), + hir::AssocItem::Const(it) => it.into(), + hir::AssocItem::TypeAlias(it) => it.into(), + }; + Definition::ModuleDef(def) + } + PathResolution::Local(local) => Definition::Local(local), + PathResolution::TypeParam(par) => Definition::TypeParam(par), + PathResolution::Macro(def) => Definition::Macro(def), + PathResolution::SelfType(impl_def) => Definition::SelfType(impl_def), + } + } } diff --git a/crates/ra_ide_db/src/search.rs b/crates/ra_ide_db/src/search.rs index 81553150b5..a7cae37b0c 100644 --- a/crates/ra_ide_db/src/search.rs +++ b/crates/ra_ide_db/src/search.rs @@ -60,6 +60,10 @@ impl SearchScope { SearchScope::new(std::iter::once((file, None)).collect()) } + pub fn files(files: &[FileId]) -> SearchScope { + SearchScope::new(files.iter().map(|f| (*f, None)).collect()) + } + pub fn intersection(&self, other: &SearchScope) -> SearchScope { let (mut small, mut large) = (&self.entries, &other.entries); if small.len() > large.len() { diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index 286619f59a..5a71d4f31a 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -151,8 +151,9 @@ impl<'db> MatchFinder<'db> { /// Returns matches for all added rules. pub fn matches(&self) -> SsrMatches { let mut matches = Vec::new(); + let mut usage_cache = search::UsageCache::default(); for rule in &self.rules { - self.find_matches_for_rule(rule, &mut matches); + self.find_matches_for_rule(rule, &mut usage_cache, &mut matches); } nester::nest_and_remove_collisions(matches, &self.sema) } diff --git a/crates/ra_ssr/src/resolving.rs b/crates/ra_ssr/src/resolving.rs index e9d0521117..63fd217ce1 100644 --- a/crates/ra_ssr/src/resolving.rs +++ b/crates/ra_ssr/src/resolving.rs @@ -22,6 +22,7 @@ pub(crate) struct ResolvedPattern { pub(crate) struct ResolvedPath { pub(crate) resolution: hir::PathResolution, + pub(crate) depth: u32, } impl ResolvedRule { @@ -62,7 +63,7 @@ struct Resolver<'a, 'db> { impl Resolver<'_, '_> { fn resolve_pattern_tree(&self, pattern: SyntaxNode) -> Result { let mut resolved_paths = FxHashMap::default(); - self.resolve(pattern.clone(), &mut resolved_paths)?; + self.resolve(pattern.clone(), 0, &mut resolved_paths)?; Ok(ResolvedPattern { node: pattern, resolved_paths, @@ -73,6 +74,7 @@ impl Resolver<'_, '_> { fn resolve( &self, node: SyntaxNode, + depth: u32, resolved_paths: &mut FxHashMap, ) -> Result<(), SsrError> { use ra_syntax::ast::AstNode; @@ -86,12 +88,12 @@ impl Resolver<'_, '_> { let resolution = self .resolve_path(&path) .ok_or_else(|| error!("Failed to resolve path `{}`", node.text()))?; - resolved_paths.insert(node, ResolvedPath { resolution }); + resolved_paths.insert(node, ResolvedPath { resolution, depth }); return Ok(()); } } for node in node.children() { - self.resolve(node, resolved_paths)?; + self.resolve(node, depth + 1, resolved_paths)?; } Ok(()) } diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index ccc2d544ab..9a4e35e96d 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -1,17 +1,106 @@ //! Searching for matches. -use crate::{matching, resolving::ResolvedRule, Match, MatchFinder}; +use crate::{ + matching, + resolving::{ResolvedPath, ResolvedPattern, ResolvedRule}, + Match, MatchFinder, +}; use ra_db::FileRange; +use ra_ide_db::{ + defs::Definition, + search::{Reference, SearchScope}, +}; use ra_syntax::{ast, AstNode, SyntaxNode}; +/// A cache for the results of find_usages. This is for when we have multiple patterns that have the +/// same path. e.g. if the pattern was `foo::Bar` that can parse as a path, an expression, a type +/// and as a pattern. In each, the usages of `foo::Bar` are the same and we'd like to avoid finding +/// them more than once. +#[derive(Default)] +pub(crate) struct UsageCache { + usages: Vec<(Definition, Vec)>, +} + impl<'db> MatchFinder<'db> { /// Adds all matches for `rule` to `matches_out`. Matches may overlap in ways that make /// replacement impossible, so further processing is required in order to properly nest matches /// and remove overlapping matches. This is done in the `nesting` module. - pub(crate) fn find_matches_for_rule(&self, rule: &ResolvedRule, matches_out: &mut Vec) { - // FIXME: Use resolved paths in the pattern to find places to search instead of always - // scanning every node. - self.slow_scan(rule, matches_out); + pub(crate) fn find_matches_for_rule( + &self, + rule: &ResolvedRule, + usage_cache: &mut UsageCache, + matches_out: &mut Vec, + ) { + if pick_path_for_usages(&rule.pattern).is_none() { + self.slow_scan(rule, matches_out); + return; + } + self.find_matches_for_pattern_tree(rule, &rule.pattern, usage_cache, matches_out); + } + + fn find_matches_for_pattern_tree( + &self, + rule: &ResolvedRule, + pattern: &ResolvedPattern, + usage_cache: &mut UsageCache, + matches_out: &mut Vec, + ) { + if let Some(first_path) = pick_path_for_usages(pattern) { + let definition: Definition = first_path.resolution.clone().into(); + for reference in self.find_usages(usage_cache, definition) { + let file = self.sema.parse(reference.file_range.file_id); + if let Some(path) = self.sema.find_node_at_offset_with_descend::( + file.syntax(), + reference.file_range.range.start(), + ) { + if let Some(node_to_match) = self + .sema + .ancestors_with_macros(path.syntax().clone()) + .skip(first_path.depth as usize) + .next() + { + if let Ok(m) = + matching::get_match(false, rule, &node_to_match, &None, &self.sema) + { + matches_out.push(m); + } + } + } + } + } + } + + fn find_usages<'a>( + &self, + usage_cache: &'a mut UsageCache, + definition: Definition, + ) -> &'a [Reference] { + // Logically if a lookup succeeds we should just return it. Unfortunately returning it would + // extend the lifetime of the borrow, then we wouldn't be able to do the insertion on a + // cache miss. This is a limitation of NLL and is fixed with Polonius. For now we do two + // lookups in the case of a cache hit. + if usage_cache.find(&definition).is_none() { + let usages = definition.find_usages(&self.sema, Some(self.search_scope())); + usage_cache.usages.push((definition, usages)); + return &usage_cache.usages.last().unwrap().1; + } + usage_cache.find(&definition).unwrap() + } + + /// Returns the scope within which we want to search. We don't want un unrestricted search + /// scope, since we don't want to find references in external dependencies. + fn search_scope(&self) -> SearchScope { + // 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()); + } + SearchScope::files(&files) } fn slow_scan(&self, rule: &ResolvedRule, matches_out: &mut Vec) { @@ -59,3 +148,35 @@ impl<'db> MatchFinder<'db> { } } } + +impl UsageCache { + fn find(&mut self, definition: &Definition) -> Option<&[Reference]> { + // We expect a very small number of cache entries (generally 1), so a linear scan should be + // fast enough and avoids the need to implement Hash for Definition. + for (d, refs) in &self.usages { + if d == definition { + return Some(refs); + } + } + None + } +} + +/// Returns a path that's suitable for path resolution. We exclude builtin types, since they aren't +/// something that we can find references to. We then somewhat arbitrarily pick the path that is the +/// longest as this is hopefully more likely to be less common, making it faster to find. +fn pick_path_for_usages(pattern: &ResolvedPattern) -> Option<&ResolvedPath> { + // FIXME: Take the scope of the resolved path into account. e.g. if there are any paths that are + // private to the current module, then we definitely would want to pick them over say a path + // from std. Possibly we should go further than this and intersect the search scopes for all + // resolved paths then search only in that scope. + pattern + .resolved_paths + .iter() + .filter(|(_, p)| { + !matches!(p.resolution, hir::PathResolution::Def(hir::ModuleDef::BuiltinType(_))) + }) + .map(|(node, resolved)| (node.text().len(), resolved)) + .max_by(|(a, _), (b, _)| a.cmp(b)) + .map(|(_, resolved)| resolved) +} From 8d09ab86edfc01405fd0045bef82e0642efd5f01 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Thu, 23 Jul 2020 21:28:31 +1000 Subject: [PATCH 12/14] SSR: Disable matching within use declarations It currently does the wrong thing when the use declaration contains braces. --- crates/ra_ssr/src/search.rs | 29 ++++++++++++++++++++++++++++- crates/ra_ssr/src/tests.rs | 23 +++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index 9a4e35e96d..141e7d026d 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -10,7 +10,8 @@ use ra_ide_db::{ defs::Definition, search::{Reference, SearchScope}, }; -use ra_syntax::{ast, AstNode, SyntaxNode}; +use ra_syntax::{ast, AstNode, SyntaxKind, SyntaxNode}; +use test_utils::mark; /// A cache for the results of find_usages. This is for when we have multiple patterns that have the /// same path. e.g. if the pattern was `foo::Bar` that can parse as a path, an expression, a type @@ -59,6 +60,10 @@ impl<'db> MatchFinder<'db> { .skip(first_path.depth as usize) .next() { + if !is_search_permitted_ancestors(&node_to_match) { + mark::hit!(use_declaration_with_braces); + continue; + } if let Ok(m) = matching::get_match(false, rule, &node_to_match, &None, &self.sema) { @@ -123,6 +128,9 @@ impl<'db> MatchFinder<'db> { restrict_range: &Option, matches_out: &mut Vec, ) { + if !is_search_permitted(code) { + return; + } if let Ok(m) = matching::get_match(false, rule, &code, restrict_range, &self.sema) { matches_out.push(m); } @@ -149,6 +157,25 @@ impl<'db> MatchFinder<'db> { } } +/// Returns whether we support matching within `node` and all of its ancestors. +fn is_search_permitted_ancestors(node: &SyntaxNode) -> bool { + if let Some(parent) = node.parent() { + if !is_search_permitted_ancestors(&parent) { + return false; + } + } + is_search_permitted(node) +} + +/// Returns whether we support matching within this kind of node. +fn is_search_permitted(node: &SyntaxNode) -> bool { + // FIXME: Properly handle use declarations. At the moment, if our search pattern is `foo::bar` + // 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 +} + impl UsageCache { fn find(&mut self, definition: &Definition) -> Option<&[Reference]> { // We expect a very small number of cache entries (generally 1), so a linear scan should be diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 33742dc8e1..f564c61298 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -804,3 +804,26 @@ fn overlapping_possible_matches() { &["foo(foo(42))", "foo(foo(foo(foo(42))))"], ); } + +#[test] +fn use_declaration_with_braces() { + // It would be OK for a path rule to match and alter a use declaration. We shouldn't mess it up + // though. In particular, we must not change `use foo::{baz, bar}` to `use foo::{baz, + // foo2::bar2}`. + mark::check!(use_declaration_with_braces); + assert_ssr_transform( + "foo::bar ==>> foo2::bar2", + r#" + mod foo { pub fn bar() {} pub fn baz() {} } + mod foo2 { pub fn bar2() {} } + use foo::{baz, bar}; + fn main() { bar() } + "#, + expect![[" + mod foo { pub fn bar() {} pub fn baz() {} } + mod foo2 { pub fn bar2() {} } + use foo::{baz, bar}; + fn main() { foo2::bar2() } + "]], + ) +} From 3dac31fe80b9d7279e87b94615b0d55805e83412 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Fri, 24 Jul 2020 20:53:48 +1000 Subject: [PATCH 13/14] SSR: Allow function calls to match method calls This differs from how this used to work before I removed it in that: a) It's only one direction. Function calls in the pattern can match method calls in the code, but not the other way around. b) We now check that the function call in the pattern resolves to the same function as the method call in the code. The lack of (b) was the reason I felt the need to remove the feature before. --- crates/ra_ide/src/ssr.rs | 3 ++ crates/ra_ssr/src/lib.rs | 8 +++-- crates/ra_ssr/src/matching.rs | 42 ++++++++++++++++++++-- crates/ra_ssr/src/resolving.rs | 18 ++++++++++ crates/ra_ssr/src/search.rs | 65 +++++++++++++++++++++++----------- crates/ra_ssr/src/tests.rs | 58 ++++++++++++++++++++++++++++++ 6 files changed, 169 insertions(+), 25 deletions(-) diff --git a/crates/ra_ide/src/ssr.rs b/crates/ra_ide/src/ssr.rs index 2f40bac08b..95d8f79b87 100644 --- a/crates/ra_ide/src/ssr.rs +++ b/crates/ra_ide/src/ssr.rs @@ -21,6 +21,9 @@ use ra_ssr::{MatchFinder, SsrError, SsrRule}; // replacement occurs. For example if our replacement template is `foo::Bar` and we match some // code in the `foo` module, we'll insert just `Bar`. // +// 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`. +// // Placeholders may be given constraints by writing them as `${::...}`. // // Supported constraints: diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index 5a71d4f31a..2fb326b45f 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -202,8 +202,12 @@ impl<'db> MatchFinder<'db> { // For now we ignore rules that have a different kind than our node, otherwise // we get lots of noise. If at some point we add support for restricting rules // to a particular kind of thing (e.g. only match type references), then we can - // relax this. - if rule.pattern.node.kind() != node.kind() { + // relax this. We special-case expressions, since function calls can match + // method calls. + if rule.pattern.node.kind() != node.kind() + && !(ast::Expr::can_cast(rule.pattern.node.kind()) + && ast::Expr::can_cast(node.kind())) + { continue; } out.push(MatchDebugInfo { diff --git a/crates/ra_ssr/src/matching.rs b/crates/ra_ssr/src/matching.rs index f3cc60c29d..4862622bde 100644 --- a/crates/ra_ssr/src/matching.rs +++ b/crates/ra_ssr/src/matching.rs @@ -189,10 +189,17 @@ impl<'db, 'sema> Matcher<'db, 'sema> { } return Ok(()); } - // Non-placeholders. + // We allow a UFCS call to match a method call, provided they resolve to the same function. + if let Some(pattern_function) = self.rule.pattern.ufcs_function_calls.get(pattern) { + if let (Some(pattern), Some(code)) = + (ast::CallExpr::cast(pattern.clone()), ast::MethodCallExpr::cast(code.clone())) + { + return self.attempt_match_ufcs(phase, &pattern, &code, *pattern_function); + } + } if pattern.kind() != code.kind() { fail_match!( - "Pattern had a `{}` ({:?}), code had `{}` ({:?})", + "Pattern had `{}` ({:?}), code had `{}` ({:?})", pattern.text(), pattern.kind(), code.text(), @@ -514,6 +521,37 @@ impl<'db, 'sema> Matcher<'db, 'sema> { Ok(()) } + fn attempt_match_ufcs( + &self, + phase: &mut Phase, + pattern: &ast::CallExpr, + code: &ast::MethodCallExpr, + pattern_function: hir::Function, + ) -> Result<(), MatchFailed> { + use ast::ArgListOwner; + let code_resolved_function = self + .sema + .resolve_method_call(code) + .ok_or_else(|| match_error!("Failed to resolve method call"))?; + if pattern_function != code_resolved_function { + fail_match!("Method call resolved to a different function"); + } + // Check arguments. + let mut pattern_args = pattern + .arg_list() + .ok_or_else(|| match_error!("Pattern function call has no args"))? + .args(); + self.attempt_match_opt(phase, pattern_args.next(), code.expr())?; + let mut code_args = + code.arg_list().ok_or_else(|| match_error!("Code method call has no args"))?.args(); + loop { + match (pattern_args.next(), code_args.next()) { + (None, None) => return Ok(()), + (p, c) => self.attempt_match_opt(phase, p, c)?, + } + } + } + fn get_placeholder(&self, element: &SyntaxElement) -> Option<&Placeholder> { only_ident(element.clone()).and_then(|ident| self.rule.get_placeholder(&ident)) } diff --git a/crates/ra_ssr/src/resolving.rs b/crates/ra_ssr/src/resolving.rs index 63fd217ce1..75f5567856 100644 --- a/crates/ra_ssr/src/resolving.rs +++ b/crates/ra_ssr/src/resolving.rs @@ -18,10 +18,12 @@ pub(crate) struct ResolvedPattern { pub(crate) node: SyntaxNode, // Paths in `node` that we've resolved. pub(crate) resolved_paths: FxHashMap, + pub(crate) ufcs_function_calls: FxHashMap, } pub(crate) struct ResolvedPath { pub(crate) resolution: hir::PathResolution, + /// The depth of the ast::Path that was resolved within the pattern. pub(crate) depth: u32, } @@ -64,10 +66,26 @@ impl Resolver<'_, '_> { fn resolve_pattern_tree(&self, pattern: SyntaxNode) -> Result { let mut resolved_paths = FxHashMap::default(); self.resolve(pattern.clone(), 0, &mut resolved_paths)?; + let ufcs_function_calls = resolved_paths + .iter() + .filter_map(|(path_node, resolved)| { + if let Some(grandparent) = path_node.parent().and_then(|parent| parent.parent()) { + if grandparent.kind() == SyntaxKind::CALL_EXPR { + if let hir::PathResolution::AssocItem(hir::AssocItem::Function(function)) = + &resolved.resolution + { + return Some((grandparent, *function)); + } + } + } + None + }) + .collect(); Ok(ResolvedPattern { node: pattern, resolved_paths, placeholders_by_stand_in: self.placeholders_by_stand_in.clone(), + ufcs_function_calls, }) } diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index 141e7d026d..bcf0f04689 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -46,35 +46,58 @@ impl<'db> MatchFinder<'db> { usage_cache: &mut UsageCache, matches_out: &mut Vec, ) { - if let Some(first_path) = pick_path_for_usages(pattern) { - let definition: Definition = first_path.resolution.clone().into(); + if let Some(resolved_path) = pick_path_for_usages(pattern) { + let definition: Definition = resolved_path.resolution.clone().into(); for reference in self.find_usages(usage_cache, definition) { - let file = self.sema.parse(reference.file_range.file_id); - if let Some(path) = self.sema.find_node_at_offset_with_descend::( - file.syntax(), - reference.file_range.range.start(), - ) { - if let Some(node_to_match) = self - .sema - .ancestors_with_macros(path.syntax().clone()) - .skip(first_path.depth as usize) - .next() + if let Some(node_to_match) = self.find_node_to_match(resolved_path, reference) { + if !is_search_permitted_ancestors(&node_to_match) { + mark::hit!(use_declaration_with_braces); + continue; + } + if let Ok(m) = + matching::get_match(false, rule, &node_to_match, &None, &self.sema) { - if !is_search_permitted_ancestors(&node_to_match) { - 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); - } + matches_out.push(m); } } } } } + fn find_node_to_match( + &self, + resolved_path: &ResolvedPath, + reference: &Reference, + ) -> Option { + let file = self.sema.parse(reference.file_range.file_id); + let depth = resolved_path.depth as usize; + let offset = reference.file_range.range.start(); + if let Some(path) = + self.sema.find_node_at_offset_with_descend::(file.syntax(), offset) + { + self.sema.ancestors_with_macros(path.syntax().clone()).skip(depth).next() + } else if let Some(path) = + self.sema.find_node_at_offset_with_descend::(file.syntax(), offset) + { + // If the pattern contained a path and we found a reference to that path that wasn't + // itself a path, but was a method call, then we need to adjust how far up to try + // matching by how deep the path was within a CallExpr. The structure would have been + // CallExpr, PathExpr, Path - i.e. a depth offset of 2. We don't need to check if the + // path was part of a CallExpr because if it wasn't then all that will happen is we'll + // fail to match, which is the desired behavior. + const PATH_DEPTH_IN_CALL_EXPR: usize = 2; + if depth < PATH_DEPTH_IN_CALL_EXPR { + return None; + } + self.sema + .ancestors_with_macros(path.syntax().clone()) + .skip(depth - PATH_DEPTH_IN_CALL_EXPR) + .next() + } else { + None + } + } + fn find_usages<'a>( &self, usage_cache: &'a mut UsageCache, diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index f564c61298..b38807c0f9 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -827,3 +827,61 @@ fn use_declaration_with_braces() { "]], ) } + +#[test] +fn ufcs_matches_method_call() { + let code = r#" + struct Foo {} + impl Foo { + fn new(_: i32) -> Foo { Foo {} } + fn do_stuff(&self, _: i32) {} + } + struct Bar {} + impl Bar { + fn new(_: i32) -> Bar { Bar {} } + fn do_stuff(&self, v: i32) {} + } + fn main() { + let b = Bar {}; + let f = Foo {}; + b.do_stuff(1); + f.do_stuff(2); + Foo::new(4).do_stuff(3); + // Too many / too few args - should never match + f.do_stuff(2, 10); + f.do_stuff(); + } + "#; + assert_matches("Foo::do_stuff($a, $b)", code, &["f.do_stuff(2)", "Foo::new(4).do_stuff(3)"]); + // The arguments needs special handling in the case of a function call matching a method call + // and the first argument is different. + assert_matches("Foo::do_stuff($a, 2)", code, &["f.do_stuff(2)"]); + assert_matches("Foo::do_stuff(Foo::new(4), $b)", code, &["Foo::new(4).do_stuff(3)"]); + + assert_ssr_transform( + "Foo::do_stuff(Foo::new($a), $b) ==>> Bar::new($b).do_stuff($a)", + code, + expect![[r#" + struct Foo {} + impl Foo { + fn new(_: i32) -> Foo { Foo {} } + fn do_stuff(&self, _: i32) {} + } + struct Bar {} + impl Bar { + fn new(_: i32) -> Bar { Bar {} } + fn do_stuff(&self, v: i32) {} + } + fn main() { + let b = Bar {}; + let f = Foo {}; + b.do_stuff(1); + f.do_stuff(2); + Bar::new(3).do_stuff(4); + // Too many / too few args - should never match + f.do_stuff(2, 10); + f.do_stuff(); + } + "#]], + ); +} From 58680cb08ea535e1fb567416fa3466a744a01b99 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Fri, 24 Jul 2020 22:23:14 +1000 Subject: [PATCH 14/14] SSR: Fix a typescript lint warning --- editors/code/src/commands.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/editors/code/src/commands.ts b/editors/code/src/commands.ts index 3ae995705f..c21e5597cb 100644 --- a/editors/code/src/commands.ts +++ b/editors/code/src/commands.ts @@ -190,7 +190,7 @@ export function ssr(ctx: Ctx): Cmd { if (!editor || !client) return; const position = editor.selection.active; - let textDocument = { uri: editor.document.uri.toString() }; + const textDocument = { uri: editor.document.uri.toString() }; const options: vscode.InputBoxOptions = { value: "() ==>> ()",