From 3600c43f49f9901ffc94a139a8a3655944e91e4e Mon Sep 17 00:00:00 2001
From: David Lattimore <dml@google.com>
Date: Wed, 29 Jul 2020 16:01:00 +1000
Subject: SSR: Don't mix non-path-based rules with path-based

If any rules contain paths, then we reject any rules that don't contain paths. Allowing a mix leads to strange semantics, since the path-based rules only match things where the path refers to semantically the same thing, whereas the non-path-based rules could match anything. Specifically, if we have a rule like `foo ==>> bar` we only want to match the `foo` that is in the current scope, not any `foo`. However "foo" can be parsed as a pattern (BIND_PAT -> NAME -> IDENT). Allowing such a rule through would result in renaming everything called `foo` to `bar`. It'd also be slow, since without a path, we'd have to use the slow-scan search mechanism.
---
 crates/ra_ssr/src/parsing.rs | 24 +++++++++++++++++++++++-
 crates/ra_ssr/src/tests.rs   | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 1 deletion(-)

(limited to 'crates')

diff --git a/crates/ra_ssr/src/parsing.rs b/crates/ra_ssr/src/parsing.rs
index 2d6f4e514..769720bef 100644
--- a/crates/ra_ssr/src/parsing.rs
+++ b/crates/ra_ssr/src/parsing.rs
@@ -10,6 +10,7 @@ use crate::{SsrError, SsrPattern, SsrRule};
 use ra_syntax::{ast, AstNode, SmolStr, SyntaxKind, SyntaxNode, T};
 use rustc_hash::{FxHashMap, FxHashSet};
 use std::str::FromStr;
+use test_utils::mark;
 
 #[derive(Debug)]
 pub(crate) struct ParsedRule {
@@ -102,14 +103,35 @@ impl RuleBuilder {
         }
     }
 
-    fn build(self) -> Result<Vec<ParsedRule>, SsrError> {
+    fn build(mut self) -> Result<Vec<ParsedRule>, SsrError> {
         if self.rules.is_empty() {
             bail!("Not a valid Rust expression, type, item, path or pattern");
         }
+        // If any rules contain paths, then we reject any rules that don't contain paths. Allowing a
+        // mix leads to strange semantics, since the path-based rules only match things where the
+        // path refers to semantically the same thing, whereas the non-path-based rules could match
+        // anything. Specifically, if we have a rule like `foo ==>> bar` we only want to match the
+        // `foo` that is in the current scope, not any `foo`. However "foo" can be parsed as a
+        // pattern (BIND_PAT -> NAME -> IDENT). Allowing such a rule through would result in
+        // renaming everything called `foo` to `bar`. It'd also be slow, since without a path, we'd
+        // have to use the slow-scan search mechanism.
+        if self.rules.iter().any(|rule| contains_path(&rule.pattern)) {
+            let old_len = self.rules.len();
+            self.rules.retain(|rule| contains_path(&rule.pattern));
+            if self.rules.len() < old_len {
+                mark::hit!(pattern_is_a_single_segment_path);
+            }
+        }
         Ok(self.rules)
     }
 }
 
+/// Returns whether there are any paths in `node`.
+fn contains_path(node: &SyntaxNode) -> bool {
+    node.kind() == SyntaxKind::PATH
+        || node.descendants().any(|node| node.kind() == SyntaxKind::PATH)
+}
+
 impl FromStr for SsrRule {
     type Err = SsrError;
 
diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs
index 18ef2506a..851e573ae 100644
--- a/crates/ra_ssr/src/tests.rs
+++ b/crates/ra_ssr/src/tests.rs
@@ -886,6 +886,45 @@ fn ufcs_matches_method_call() {
     );
 }
 
+#[test]
+fn pattern_is_a_single_segment_path() {
+    mark::check!(pattern_is_a_single_segment_path);
+    // The first function should not be altered because the `foo` in scope at the cursor position is
+    // a different `foo`. This case is special because "foo" can be parsed as a pattern (BIND_PAT ->
+    // NAME -> IDENT), which contains no path. If we're not careful we'll end up matching the `foo`
+    // in `let foo` from the first function. Whether we should match the `let foo` in the second
+    // function is less clear. At the moment, we don't. Doing so sounds like a rename operation,
+    // which isn't really what SSR is for, especially since the replacement `bar` must be able to be
+    // resolved, which means if we rename `foo` we'll get a name collision.
+    assert_ssr_transform(
+        "foo ==>> bar",
+        r#"
+        fn f1() -> i32 {
+            let foo = 1;
+            let bar = 2;
+            foo
+        }
+        fn f1() -> i32 {
+            let foo = 1;
+            let bar = 2;
+            foo<|>
+        }
+        "#,
+        expect![[r#"
+            fn f1() -> i32 {
+                let foo = 1;
+                let bar = 2;
+                foo
+            }
+            fn f1() -> i32 {
+                let foo = 1;
+                let bar = 2;
+                bar
+            }
+        "#]],
+    );
+}
+
 #[test]
 fn replace_local_variable_reference() {
     // The pattern references a local variable `foo` in the block containing the cursor. We should
-- 
cgit v1.2.3