3 files changed, 172 insertions, 55 deletions

diff --git a/crates/ra_ide/src/runnables.rs b/crates/ra_ide/src/runnables.rs
index 95a35a28d..45e0a7d85 100644
--- a/crates/ra_ide/src/runnables.rs
+++ b/crates/ra_ide/src/runnables.rs
@@ -220,15 +220,7 @@ fn runnable_mod(
     module: ast::Module,
     file_id: FileId,
 ) -> Option<Runnable> {
-    let has_test_function = module
-        .item_list()?
-        .items()
-        .filter_map(|it| match it {
-            ast::ModuleItem::FnDef(it) => Some(it),
-            _ => None,
-        })
-        .any(|f| has_test_related_attribute(&f));
-    if !has_test_function {
+    if !has_test_function_or_multiple_test_submodules(&module) {
         return None;
     }
     let module_def = sema.to_def(&module)?;
@@ -246,6 +238,34 @@ fn runnable_mod(
     Some(Runnable { nav, kind: RunnableKind::TestMod { path }, cfg_exprs })
 }
 
+// We could create runnables for modules with number_of_test_submodules > 0,
+// but that bloats the runnables for no real benefit, since all tests can be run by the submodule already
+fn has_test_function_or_multiple_test_submodules(module: &ast::Module) -> bool {
+    if let Some(item_list) = module.item_list() {
+        let mut number_of_test_submodules = 0;
+
+        for item in item_list.items() {
+            match item {
+                ast::ModuleItem::FnDef(f) => {
+                    if has_test_related_attribute(&f) {
+                        return true;
+                    }
+                }
+                ast::ModuleItem::Module(submodule) => {
+                    if has_test_function_or_multiple_test_submodules(&submodule) {
+                        number_of_test_submodules += 1;
+                    }
+                }
+                _ => (),
+            }
+        }
+
+        number_of_test_submodules > 1
+    } else {
+        false
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use expect::{expect, Expect};
@@ -571,19 +591,33 @@ mod test_mod {
     }
 
     #[test]
-    fn test_runnables_one_depth_layer_module() {
+    fn only_modules_with_test_functions_or_more_than_one_test_submodule_have_runners() {
         check(
             r#"
 //- /lib.rs
 <|>
-mod foo {
-    mod test_mod {
-        #[test]
-        fn test_foo1() {}
+mod root_tests {
+    mod nested_tests_0 {
+        mod nested_tests_1 {
+            #[test]
+            fn nested_test_11() {}
+
+            #[test]
+            fn nested_test_12() {}
+        }
+
+        mod nested_tests_2 {
+            #[test]
+            fn nested_test_2() {}
+        }
+
+        mod nested_tests_3 {}
     }
+
+    mod nested_tests_4 {}
 }
 "#,
-            &[&TEST, &TEST],
+            &[&TEST, &TEST, &TEST, &TEST, &TEST, &TEST],
             expect![[r#"
                 [
                     Runnable {
@@ -591,18 +625,18 @@ mod foo {
                             file_id: FileId(
                                 1,
                             ),
-                            full_range: 15..77,
+                            full_range: 22..323,
                             focus_range: Some(
-                                19..27,
+                                26..40,
                             ),
-                            name: "test_mod",
+                            name: "nested_tests_0",
                             kind: MODULE,
                             container_name: None,
                             description: None,
                             docs: None,
                         },
                         kind: TestMod {
-                            path: "foo::test_mod",
+                            path: "root_tests::nested_tests_0",
                         },
                         cfg_exprs: [],
                     },
@@ -611,11 +645,31 @@ mod foo {
                             file_id: FileId(
                                 1,
                             ),
-                            full_range: 38..71,
+                            full_range: 51..192,
                             focus_range: Some(
-                                57..66,
+                                55..69,
                             ),
-                            name: "test_foo1",
+                            name: "nested_tests_1",
+                            kind: MODULE,
+                            container_name: None,
+                            description: None,
+                            docs: None,
+                        },
+                        kind: TestMod {
+                            path: "root_tests::nested_tests_0::nested_tests_1",
+                        },
+                        cfg_exprs: [],
+                    },
+                    Runnable {
+                        nav: NavigationTarget {
+                            file_id: FileId(
+                                1,
+                            ),
+                            full_range: 84..126,
+                            focus_range: Some(
+                                107..121,
+                            ),
+                            name: "nested_test_11",
                             kind: FN_DEF,
                             container_name: None,
                             description: None,
@@ -623,7 +677,7 @@ mod foo {
                         },
                         kind: Test {
                             test_id: Path(
-                                "foo::test_mod::test_foo1",
+                                "root_tests::nested_tests_0::nested_tests_1::nested_test_11",
                             ),
                             attr: TestAttr {
                                 ignore: false,
@@ -631,46 +685,48 @@ mod foo {
                         },
                         cfg_exprs: [],
                     },
-                ]
-            "#]],
-        );
-    }
-
-    #[test]
-    fn test_runnables_multiple_depth_module() {
-        check(
-            r#"
-//- /lib.rs
-<|>
-mod foo {
-    mod bar {
-        mod test_mod {
-            #[test]
-            fn test_foo1() {}
-        }
-    }
-}
-"#,
-            &[&TEST, &TEST],
-            expect![[r#"
-                [
                     Runnable {
                         nav: NavigationTarget {
                             file_id: FileId(
                                 1,
                             ),
-                            full_range: 33..107,
+                            full_range: 140..182,
                             focus_range: Some(
-                                37..45,
+                                163..177,
                             ),
-                            name: "test_mod",
+                            name: "nested_test_12",
+                            kind: FN_DEF,
+                            container_name: None,
+                            description: None,
+                            docs: None,
+                        },
+                        kind: Test {
+                            test_id: Path(
+                                "root_tests::nested_tests_0::nested_tests_1::nested_test_12",
+                            ),
+                            attr: TestAttr {
+                                ignore: false,
+                            },
+                        },
+                        cfg_exprs: [],
+                    },
+                    Runnable {
+                        nav: NavigationTarget {
+                            file_id: FileId(
+                                1,
+                            ),
+                            full_range: 202..286,
+                            focus_range: Some(
+                                206..220,
+                            ),
+                            name: "nested_tests_2",
                             kind: MODULE,
                             container_name: None,
                             description: None,
                             docs: None,
                         },
                         kind: TestMod {
-                            path: "foo::bar::test_mod",
+                            path: "root_tests::nested_tests_0::nested_tests_2",
                         },
                         cfg_exprs: [],
                     },
@@ -679,11 +735,11 @@ mod foo {
                             file_id: FileId(
                                 1,
                             ),
-                            full_range: 60..97,
+                            full_range: 235..276,
                             focus_range: Some(
-                                83..92,
+                                258..271,
                             ),
-                            name: "test_foo1",
+                            name: "nested_test_2",
                             kind: FN_DEF,
                             container_name: None,
                             description: None,
@@ -691,7 +747,7 @@ mod foo {
                         },
                         kind: Test {
                             test_id: Path(
-                                "foo::bar::test_mod::test_foo1",
+                                "root_tests::nested_tests_0::nested_tests_2::nested_test_2",
                             ),
                             attr: TestAttr {
                                 ignore: false,
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 6a44ef378..f5ffff7cc 100644
--- a/crates/ra_ssr/src/tests.rs
+++ b/crates/ra_ssr/src/tests.rs
@@ -900,6 +900,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
     // only replace references to this variable `foo`, not other variables that just happen to have