aboutsummaryrefslogtreecommitdiff
path: root/crates
diff options
context:
space:
mode:
authorDavid Lattimore <[email protected]>2020-07-29 07:01:00 +0100
committerDavid Lattimore <[email protected]>2020-07-29 07:01:00 +0100
commit3600c43f49f9901ffc94a139a8a3655944e91e4e (patch)
treefab3f6475ddb107c5f19834fc914704c930a1392 /crates
parent5a8124273dd663f7f1ed43b53defc4a2c52dbc12 (diff)
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.
Diffstat (limited to 'crates')
-rw-r--r--crates/ra_ssr/src/parsing.rs24
-rw-r--r--crates/ra_ssr/src/tests.rs39
2 files changed, 62 insertions, 1 deletions
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};
10use ra_syntax::{ast, AstNode, SmolStr, SyntaxKind, SyntaxNode, T}; 10use ra_syntax::{ast, AstNode, SmolStr, SyntaxKind, SyntaxNode, T};
11use rustc_hash::{FxHashMap, FxHashSet}; 11use rustc_hash::{FxHashMap, FxHashSet};
12use std::str::FromStr; 12use std::str::FromStr;
13use test_utils::mark;
13 14
14#[derive(Debug)] 15#[derive(Debug)]
15pub(crate) struct ParsedRule { 16pub(crate) struct ParsedRule {
@@ -102,14 +103,35 @@ impl RuleBuilder {
102 } 103 }
103 } 104 }
104 105
105 fn build(self) -> Result<Vec<ParsedRule>, SsrError> { 106 fn build(mut self) -> Result<Vec<ParsedRule>, SsrError> {
106 if self.rules.is_empty() { 107 if self.rules.is_empty() {
107 bail!("Not a valid Rust expression, type, item, path or pattern"); 108 bail!("Not a valid Rust expression, type, item, path or pattern");
108 } 109 }
110 // If any rules contain paths, then we reject any rules that don't contain paths. Allowing a
111 // mix leads to strange semantics, since the path-based rules only match things where the
112 // path refers to semantically the same thing, whereas the non-path-based rules could match
113 // anything. Specifically, if we have a rule like `foo ==>> bar` we only want to match the
114 // `foo` that is in the current scope, not any `foo`. However "foo" can be parsed as a
115 // pattern (BIND_PAT -> NAME -> IDENT). Allowing such a rule through would result in
116 // renaming everything called `foo` to `bar`. It'd also be slow, since without a path, we'd
117 // have to use the slow-scan search mechanism.
118 if self.rules.iter().any(|rule| contains_path(&rule.pattern)) {
119 let old_len = self.rules.len();
120 self.rules.retain(|rule| contains_path(&rule.pattern));
121 if self.rules.len() < old_len {
122 mark::hit!(pattern_is_a_single_segment_path);
123 }
124 }
109 Ok(self.rules) 125 Ok(self.rules)
110 } 126 }
111} 127}
112 128
129/// Returns whether there are any paths in `node`.
130fn contains_path(node: &SyntaxNode) -> bool {
131 node.kind() == SyntaxKind::PATH
132 || node.descendants().any(|node| node.kind() == SyntaxKind::PATH)
133}
134
113impl FromStr for SsrRule { 135impl FromStr for SsrRule {
114 type Err = SsrError; 136 type Err = SsrError;
115 137
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
@@ -887,6 +887,45 @@ fn ufcs_matches_method_call() {
887} 887}
888 888
889#[test] 889#[test]
890fn pattern_is_a_single_segment_path() {
891 mark::check!(pattern_is_a_single_segment_path);
892 // The first function should not be altered because the `foo` in scope at the cursor position is
893 // a different `foo`. This case is special because "foo" can be parsed as a pattern (BIND_PAT ->
894 // NAME -> IDENT), which contains no path. If we're not careful we'll end up matching the `foo`
895 // in `let foo` from the first function. Whether we should match the `let foo` in the second
896 // function is less clear. At the moment, we don't. Doing so sounds like a rename operation,
897 // which isn't really what SSR is for, especially since the replacement `bar` must be able to be
898 // resolved, which means if we rename `foo` we'll get a name collision.
899 assert_ssr_transform(
900 "foo ==>> bar",
901 r#"
902 fn f1() -> i32 {
903 let foo = 1;
904 let bar = 2;
905 foo
906 }
907 fn f1() -> i32 {
908 let foo = 1;
909 let bar = 2;
910 foo<|>
911 }
912 "#,
913 expect![[r#"
914 fn f1() -> i32 {
915 let foo = 1;
916 let bar = 2;
917 foo
918 }
919 fn f1() -> i32 {
920 let foo = 1;
921 let bar = 2;
922 bar
923 }
924 "#]],
925 );
926}
927
928#[test]
890fn replace_local_variable_reference() { 929fn replace_local_variable_reference() {
891 // The pattern references a local variable `foo` in the block containing the cursor. We should 930 // The pattern references a local variable `foo` in the block containing the cursor. We should
892 // only replace references to this variable `foo`, not other variables that just happen to have 931 // only replace references to this variable `foo`, not other variables that just happen to have