From fa1e4113224c119258670538f8c3ca6c8ea8ad1e Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Wed, 29 Jul 2020 21:23:39 +1000 Subject: SSR: Wrap placeholder expansions in parenthesis when necessary e.g. `foo($a) ==> $a.to_string()` should produce `(1 + 2).to_string()` not `1 + 2.to_string()` We don't yet try to determine if the whole replacement needs to be wrapped in parenthesis. That's harder and I think perhaps less often an issue. --- crates/ra_ssr/src/replacing.rs | 106 ++++++++++++++++++++++++++++++++++------- crates/ra_ssr/src/tests.rs | 25 +++++++++- 2 files changed, 111 insertions(+), 20 deletions(-) (limited to 'crates/ra_ssr') diff --git a/crates/ra_ssr/src/replacing.rs b/crates/ra_ssr/src/replacing.rs index 4b3f5509c..0943244ff 100644 --- a/crates/ra_ssr/src/replacing.rs +++ b/crates/ra_ssr/src/replacing.rs @@ -3,8 +3,9 @@ use crate::matching::Var; use crate::{resolving::ResolvedRule, Match, SsrMatches}; use ra_syntax::ast::{self, AstToken}; -use ra_syntax::{SyntaxElement, SyntaxKind, SyntaxNode, SyntaxToken, TextSize}; +use ra_syntax::{SyntaxElement, SyntaxKind, SyntaxNode, SyntaxToken, TextRange, TextSize}; use ra_text_edit::TextEdit; +use rustc_hash::{FxHashMap, FxHashSet}; /// Returns a text edit that will replace each match in `matches` with its corresponding replacement /// template. Placeholders in the template will have been substituted with whatever they matched to @@ -38,62 +39,79 @@ struct ReplacementRenderer<'a> { file_src: &'a str, rules: &'a [ResolvedRule], rule: &'a ResolvedRule, + out: String, + // Map from a range within `out` to a token in `template` that represents a placeholder. This is + // used to validate that the generated source code doesn't split any placeholder expansions (see + // below). + placeholder_tokens_by_range: FxHashMap, + // Which placeholder tokens need to be wrapped in parenthesis in order to ensure that when `out` + // is parsed, placeholders don't get split. e.g. if a template of `$a.to_string()` results in `1 + // + 2.to_string()` then the placeholder value `1 + 2` was split and needs parenthesis. + placeholder_tokens_requiring_parenthesis: FxHashSet, } fn render_replace(match_info: &Match, file_src: &str, rules: &[ResolvedRule]) -> String { - let mut out = String::new(); let rule = &rules[match_info.rule_index]; let template = rule .template .as_ref() .expect("You called MatchFinder::edits after calling MatchFinder::add_search_pattern"); - let renderer = ReplacementRenderer { match_info, file_src, rules, rule }; - renderer.render_node(&template.node, &mut out); + let mut renderer = ReplacementRenderer { + match_info, + file_src, + rules, + rule, + out: String::new(), + placeholder_tokens_requiring_parenthesis: FxHashSet::default(), + placeholder_tokens_by_range: FxHashMap::default(), + }; + renderer.render_node(&template.node); + renderer.maybe_rerender_with_extra_parenthesis(&template.node); for comment in &match_info.ignored_comments { - out.push_str(&comment.syntax().to_string()); + renderer.out.push_str(&comment.syntax().to_string()); } - out + renderer.out } impl ReplacementRenderer<'_> { - fn render_node_children(&self, node: &SyntaxNode, out: &mut String) { + fn render_node_children(&mut self, node: &SyntaxNode) { for node_or_token in node.children_with_tokens() { - self.render_node_or_token(&node_or_token, out); + self.render_node_or_token(&node_or_token); } } - fn render_node_or_token(&self, node_or_token: &SyntaxElement, out: &mut String) { + fn render_node_or_token(&mut self, node_or_token: &SyntaxElement) { match node_or_token { SyntaxElement::Token(token) => { - self.render_token(&token, out); + self.render_token(&token); } SyntaxElement::Node(child_node) => { - self.render_node(&child_node, out); + self.render_node(&child_node); } } } - fn render_node(&self, node: &SyntaxNode, out: &mut String) { + fn render_node(&mut self, node: &SyntaxNode) { use ra_syntax::ast::AstNode; if let Some(mod_path) = self.match_info.rendered_template_paths.get(&node) { - out.push_str(&mod_path.to_string()); + self.out.push_str(&mod_path.to_string()); // Emit everything except for the segment's name-ref, since we already effectively // emitted that as part of `mod_path`. if let Some(path) = ast::Path::cast(node.clone()) { if let Some(segment) = path.segment() { for node_or_token in segment.syntax().children_with_tokens() { if node_or_token.kind() != SyntaxKind::NAME_REF { - self.render_node_or_token(&node_or_token, out); + self.render_node_or_token(&node_or_token); } } } } } else { - self.render_node_children(&node, out); + self.render_node_children(&node); } } - fn render_token(&self, token: &SyntaxToken, out: &mut String) { + fn render_token(&mut self, token: &SyntaxToken) { if let Some(placeholder) = self.rule.get_placeholder(&token) { if let Some(placeholder_value) = self.match_info.placeholder_values.get(&Var(placeholder.ident.to_string())) @@ -107,8 +125,23 @@ impl ReplacementRenderer<'_> { range.start(), self.rules, ); + let needs_parenthesis = + self.placeholder_tokens_requiring_parenthesis.contains(token); edit.apply(&mut matched_text); - out.push_str(&matched_text); + if needs_parenthesis { + self.out.push('('); + } + self.placeholder_tokens_by_range.insert( + TextRange::new( + TextSize::of(&self.out), + TextSize::of(&self.out) + TextSize::of(&matched_text), + ), + token.clone(), + ); + self.out.push_str(&matched_text); + if needs_parenthesis { + self.out.push(')'); + } } else { // We validated that all placeholder references were valid before we // started, so this shouldn't happen. @@ -118,7 +151,44 @@ impl ReplacementRenderer<'_> { ); } } else { - out.push_str(token.text().as_str()); + self.out.push_str(token.text().as_str()); + } + } + + // Checks if the resulting code, when parsed doesn't split any placeholders due to different + // order of operations between the search pattern and the replacement template. If any do, then + // we rerender the template and wrap the problematic placeholders with parenthesis. + fn maybe_rerender_with_extra_parenthesis(&mut self, template: &SyntaxNode) { + if let Some(node) = parse_as_kind(&self.out, template.kind()) { + self.remove_node_ranges(node); + if self.placeholder_tokens_by_range.is_empty() { + return; + } + self.placeholder_tokens_requiring_parenthesis = + self.placeholder_tokens_by_range.values().cloned().collect(); + self.out.clear(); + self.render_node(template); + } + } + + fn remove_node_ranges(&mut self, node: SyntaxNode) { + self.placeholder_tokens_by_range.remove(&node.text_range()); + for child in node.children() { + self.remove_node_ranges(child); + } + } +} + +fn parse_as_kind(code: &str, kind: SyntaxKind) -> Option { + use ra_syntax::ast::AstNode; + if ast::Expr::can_cast(kind) { + if let Ok(expr) = ast::Expr::parse(code) { + return Some(expr.syntax().clone()); + } + } else if ast::Item::can_cast(kind) { + if let Ok(item) = ast::Item::parse(code) { + return Some(item.syntax().clone()); } } + None } diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index f5ffff7cc..a4fa2cb44 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -664,7 +664,7 @@ fn replace_binary_op() { assert_ssr_transform( "$a + $b ==>> $b + $a", "fn f() {1 + 2 + 3 + 4}", - expect![["fn f() {4 + 3 + 2 + 1}"]], + expect![[r#"fn f() {4 + (3 + (2 + 1))}"#]], ); } @@ -773,11 +773,32 @@ fn preserves_whitespace_within_macro_expansion() { macro_rules! macro1 { ($a:expr) => {$a} } - fn f() {macro1!(4 - 3 - 1 * 2} + fn f() {macro1!(4 - (3 - 1 * 2)} "#]], ) } +#[test] +fn add_parenthesis_when_necessary() { + assert_ssr_transform( + "foo($a) ==>> $a.to_string()", + r#" + fn foo(_: i32) {} + fn bar3(v: i32) { + foo(1 + 2); + foo(-v); + } + "#, + expect![[r#" + fn foo(_: i32) {} + fn bar3(v: i32) { + (1 + 2).to_string(); + (-v).to_string(); + } + "#]], + ) +} + #[test] fn match_failure_reasons() { let code = r#" -- cgit v1.2.3