From 3600c43f49f9901ffc94a139a8a3655944e91e4e Mon Sep 17 00:00:00 2001 From: David Lattimore 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, SsrError> { + fn build(mut self) -> Result, 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