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 From 6636f56e79b55f22b88094b7edaed6ec88880500 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Thu, 30 Jul 2020 00:23:03 +0200 Subject: Rename ModuleItem -> Item --- crates/ra_ssr/src/parsing.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (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 769720bef..78e03f394 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -71,10 +71,7 @@ impl ParsedRule { }; builder.try_add(ast::Expr::parse(&raw_pattern), raw_template.map(ast::Expr::parse)); builder.try_add(ast::TypeRef::parse(&raw_pattern), raw_template.map(ast::TypeRef::parse)); - builder.try_add( - ast::ModuleItem::parse(&raw_pattern), - raw_template.map(ast::ModuleItem::parse), - ); + builder.try_add(ast::Item::parse(&raw_pattern), raw_template.map(ast::Item::parse)); builder.try_add(ast::Path::parse(&raw_pattern), raw_template.map(ast::Path::parse)); builder.try_add(ast::Pat::parse(&raw_pattern), raw_template.map(ast::Pat::parse)); builder.build() -- cgit v1.2.3 From 08ea2271e8050165d0aaf4c994ed3dd746aff3ba Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 31 Jul 2020 12:06:38 +0200 Subject: Rename TypeRef -> Type The TypeRef name comes from IntelliJ days, where you often have both type *syntax* as well as *semantical* representation of types in scope. And naming both Type is confusing. In rust-analyzer however, we use ast types as `ast::Type`, and have many more semantic counterparts to ast types, so avoiding name clash here is just confusing. --- crates/ra_ssr/src/parsing.rs | 2 +- 1 file changed, 1 insertion(+), 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 78e03f394..4e046910c 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -70,7 +70,7 @@ impl ParsedRule { rules: Vec::new(), }; builder.try_add(ast::Expr::parse(&raw_pattern), raw_template.map(ast::Expr::parse)); - builder.try_add(ast::TypeRef::parse(&raw_pattern), raw_template.map(ast::TypeRef::parse)); + builder.try_add(ast::Type::parse(&raw_pattern), raw_template.map(ast::Type::parse)); builder.try_add(ast::Item::parse(&raw_pattern), raw_template.map(ast::Item::parse)); builder.try_add(ast::Path::parse(&raw_pattern), raw_template.map(ast::Path::parse)); builder.try_add(ast::Pat::parse(&raw_pattern), raw_template.map(ast::Pat::parse)); -- cgit v1.2.3 From 98181087984157e27faba0b969e384f3c62c39d5 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Fri, 31 Jul 2020 20:09:09 +0200 Subject: Rename BindPat -> IdentPat --- crates/ra_ssr/src/parsing.rs | 2 +- 1 file changed, 1 insertion(+), 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 4e046910c..f455eb5b7 100644 --- a/crates/ra_ssr/src/parsing.rs +++ b/crates/ra_ssr/src/parsing.rs @@ -109,7 +109,7 @@ impl RuleBuilder { // 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 + // pattern (IDENT_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)) { -- cgit v1.2.3