Merge #5567

5567: SSR: Wrap placeholder expansions in parenthesis when necessary r=matklad a=davidlattimore

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.

Co-authored-by: David Lattimore <dml@google.com>

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<TextRange, SyntaxToken>,
+    // 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<SyntaxToken>,
 }
 
 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 };
+    let mut renderer = ReplacementRenderer {
-    renderer.render_node(&template.node, &mut out);
+        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<SyntaxNode> {
+    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
+}

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#"