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 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) (limited to 'crates/ra_ssr/src/parsing.rs') 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; -- cgit v1.2.3