From 6bbeffc8c56893548f5667844f59ce5a76f9fd98 Mon Sep 17 00:00:00 2001 From: David Lattimore Date: Sat, 1 Aug 2020 17:41:42 +1000 Subject: SSR: Allow `self` in patterns. It's now consistent with other variables in that if the pattern references self, only the `self` in scope where the rule is invoked will be accepted. Since `self` doesn't work the same as other paths, this is implemented by restricting the search to just the current function. Prior to this change (since path resolution was implemented), having self in a pattern would just result in no matches. --- crates/ra_ssr/src/lib.rs | 7 +------ crates/ra_ssr/src/resolving.rs | 29 +++++++++++++++++++++++++++++ crates/ra_ssr/src/search.rs | 9 +++++++++ crates/ra_ssr/src/tests.rs | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 6 deletions(-) diff --git a/crates/ra_ssr/src/lib.rs b/crates/ra_ssr/src/lib.rs index 73abfecb2..c780b460a 100644 --- a/crates/ra_ssr/src/lib.rs +++ b/crates/ra_ssr/src/lib.rs @@ -66,12 +66,7 @@ impl<'db> MatchFinder<'db> { restrict_ranges.retain(|range| !range.range.is_empty()); let sema = Semantics::new(db); let resolution_scope = resolving::ResolutionScope::new(&sema, lookup_context); - MatchFinder { - sema: Semantics::new(db), - rules: Vec::new(), - resolution_scope, - restrict_ranges, - } + MatchFinder { sema, rules: Vec::new(), resolution_scope, restrict_ranges } } /// Constructs an instance using the start of the first file in `db` as the lookup context. diff --git a/crates/ra_ssr/src/resolving.rs b/crates/ra_ssr/src/resolving.rs index 6f62000f4..0ac929bd5 100644 --- a/crates/ra_ssr/src/resolving.rs +++ b/crates/ra_ssr/src/resolving.rs @@ -11,6 +11,7 @@ use test_utils::mark; pub(crate) struct ResolutionScope<'db> { scope: hir::SemanticsScope<'db>, hygiene: hir::Hygiene, + node: SyntaxNode, } pub(crate) struct ResolvedRule { @@ -25,6 +26,7 @@ pub(crate) struct ResolvedPattern { // Paths in `node` that we've resolved. pub(crate) resolved_paths: FxHashMap, pub(crate) ufcs_function_calls: FxHashMap, + pub(crate) contains_self: bool, } pub(crate) struct ResolvedPath { @@ -68,6 +70,7 @@ struct Resolver<'a, 'db> { impl Resolver<'_, '_> { fn resolve_pattern_tree(&self, pattern: SyntaxNode) -> Result { + use ra_syntax::{SyntaxElement, T}; let mut resolved_paths = FxHashMap::default(); self.resolve(pattern.clone(), 0, &mut resolved_paths)?; let ufcs_function_calls = resolved_paths @@ -85,11 +88,17 @@ impl Resolver<'_, '_> { None }) .collect(); + let contains_self = + pattern.descendants_with_tokens().any(|node_or_token| match node_or_token { + SyntaxElement::Token(t) => t.kind() == T![self], + _ => false, + }); Ok(ResolvedPattern { node: pattern, resolved_paths, placeholders_by_stand_in: self.placeholders_by_stand_in.clone(), ufcs_function_calls, + contains_self, }) } @@ -101,6 +110,10 @@ impl Resolver<'_, '_> { ) -> Result<(), SsrError> { use ra_syntax::ast::AstNode; if let Some(path) = ast::Path::cast(node.clone()) { + if is_self(&path) { + // Self cannot be resolved like other paths. + return Ok(()); + } // Check if this is an appropriate place in the path to resolve. If the path is // something like `a::B::::c` then we want to resolve `a::B`. If the path contains // a placeholder. e.g. `a::$b::c` then we want to resolve `a`. @@ -157,6 +170,18 @@ impl<'db> ResolutionScope<'db> { ResolutionScope { scope, hygiene: hir::Hygiene::new(sema.db, resolve_context.file_id.into()), + node, + } + } + + /// Returns the function in which SSR was invoked, if any. + pub(crate) fn current_function(&self) -> Option { + let mut node = self.node.clone(); + loop { + if node.kind() == SyntaxKind::FN { + return Some(node); + } + node = node.parent()?; } } @@ -186,6 +211,10 @@ impl<'db> ResolutionScope<'db> { } } +fn is_self(path: &ast::Path) -> bool { + path.segment().map(|segment| segment.self_token().is_some()).unwrap_or(false) +} + /// Returns a suitable node for resolving paths in the current scope. If we create a scope based on /// a statement node, then we can't resolve local variables that were defined in the current scope /// (only in parent scopes). So we find another node, ideally a child of the statement where local diff --git a/crates/ra_ssr/src/search.rs b/crates/ra_ssr/src/search.rs index 213dc494f..85ffa2ac2 100644 --- a/crates/ra_ssr/src/search.rs +++ b/crates/ra_ssr/src/search.rs @@ -33,6 +33,15 @@ impl<'db> MatchFinder<'db> { usage_cache: &mut UsageCache, matches_out: &mut Vec, ) { + if rule.pattern.contains_self { + // If the pattern contains `self` we restrict the scope of the search to just the + // current method. No other method can reference the same `self`. This makes the + // behavior of `self` consistent with other variables. + if let Some(current_function) = self.resolution_scope.current_function() { + self.slow_scan_node(¤t_function, rule, &None, matches_out); + } + return; + } if pick_path_for_usages(&rule.pattern).is_none() { self.slow_scan(rule, matches_out); return; diff --git a/crates/ra_ssr/src/tests.rs b/crates/ra_ssr/src/tests.rs index 2ae03c64c..d483640df 100644 --- a/crates/ra_ssr/src/tests.rs +++ b/crates/ra_ssr/src/tests.rs @@ -1044,3 +1044,38 @@ fn replace_nonpath_within_selection() { }"#]], ); } + +#[test] +fn replace_self() { + // `foo(self)` occurs twice in the code, however only the first occurrence is the `self` that's + // in scope where the rule is invoked. + assert_ssr_transform( + "foo(self) ==>> bar(self)", + r#" + struct S1 {} + fn foo(_: &S1) {} + fn bar(_: &S1) {} + impl S1 { + fn f1(&self) { + foo(self)<|> + } + fn f2(&self) { + foo(self) + } + } + "#, + expect![[r#" + struct S1 {} + fn foo(_: &S1) {} + fn bar(_: &S1) {} + impl S1 { + fn f1(&self) { + bar(self) + } + fn f2(&self) { + foo(self) + } + } + "#]], + ); +} -- cgit v1.2.3