From 9016a4cdf7b3dda12ae0dd60c633171023ad975f Mon Sep 17 00:00:00 2001 From: roife Date: Mon, 30 Dec 2024 15:29:01 +0800 Subject: [PATCH] fix: avoid generating colliding names in extract_variable --- .../src/handlers/destructure_tuple_binding.rs | 14 +- .../src/handlers/extract_variable.rs | 41 ++++- .../replace_is_method_with_if_let_method.rs | 13 +- .../crates/ide-assists/src/tests/generated.rs | 2 +- .../ide-db/src/syntax_helpers/suggest_name.rs | 147 ++++++++++-------- 5 files changed, 126 insertions(+), 91 deletions(-) diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/destructure_tuple_binding.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/destructure_tuple_binding.rs index b9142d0318aad..7df6ca1565fd5 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/destructure_tuple_binding.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/destructure_tuple_binding.rs @@ -6,7 +6,6 @@ use ide_db::{ text_edit::TextRange, }; use itertools::Itertools; -use syntax::SmolStr; use syntax::{ ast::{self, make, AstNode, FieldExpr, HasName, IdentPat}, ted, @@ -134,17 +133,8 @@ fn collect_data(ident_pat: IdentPat, ctx: &AssistContext<'_>) -> Option, expr: &ast::Expr) -> Option { - let literal = match expr { - ast::Expr::Literal(literal) => literal, - _ => return None, + let ast::Expr::Literal(literal) = expr else { + return None; }; + let inner = match literal.kind() { ast::LiteralKind::String(string) => string.value().ok()?.into_owned(), ast::LiteralKind::ByteString(byte_string) => { @@ -2632,4 +2634,33 @@ fn foo() { "Extract into static", ); } + + #[test] + fn extract_variable_name_conflicts() { + check_assist_by_label( + extract_variable, + r#" +struct S { x: i32 }; + +fn main() { + let s = 2; + let t = $0S { x: 1 }$0; + let t2 = t; + let x = s; +} +"#, + r#" +struct S { x: i32 }; + +fn main() { + let s = 2; + let $0s1 = S { x: 1 }; + let t = s1; + let t2 = t; + let x = s; +} +"#, + "Extract into variable", + ); + } } diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/replace_is_method_with_if_let_method.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/replace_is_method_with_if_let_method.rs index a856da0921537..47972ff619acb 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/replace_is_method_with_if_let_method.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/replace_is_method_with_if_let_method.rs @@ -20,7 +20,7 @@ use crate::{AssistContext, AssistId, AssistKind, Assists}; // ``` // fn main() { // let x = Some(1); -// if let Some(${0:x}) = x {} +// if let Some(${0:x1}) = x {} // } // ``` pub(crate) fn replace_is_method_with_if_let_method( @@ -40,10 +40,13 @@ pub(crate) fn replace_is_method_with_if_let_method( "is_some" | "is_ok" => { let receiver = call_expr.receiver()?; + let mut name_generator = suggest_name::NameGenerator::new_from_scope_locals( + ctx.sema.scope(if_expr.syntax()), + ); let var_name = if let ast::Expr::PathExpr(path_expr) = receiver.clone() { - path_expr.path()?.to_string() + name_generator.suggest_name(&path_expr.path()?.to_string()) } else { - suggest_name::for_variable(&receiver, &ctx.sema) + name_generator.for_variable(&receiver, &ctx.sema) }; let (assist_id, message, text) = if name_ref.text() == "is_some" { @@ -98,7 +101,7 @@ fn main() { r#" fn main() { let x = Some(1); - if let Some(${0:x}) = x {} + if let Some(${0:x1}) = x {} } "#, ); @@ -150,7 +153,7 @@ fn main() { r#" fn main() { let x = Ok(1); - if let Ok(${0:x}) = x {} + if let Ok(${0:x1}) = x {} } "#, ); diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/tests/generated.rs b/src/tools/rust-analyzer/crates/ide-assists/src/tests/generated.rs index 78fdfba6a07d1..54e42f126bc53 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/tests/generated.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/tests/generated.rs @@ -2885,7 +2885,7 @@ fn main() { r#####" fn main() { let x = Some(1); - if let Some(${0:x}) = x {} + if let Some(${0:x1}) = x {} } "#####, ) diff --git a/src/tools/rust-analyzer/crates/ide-db/src/syntax_helpers/suggest_name.rs b/src/tools/rust-analyzer/crates/ide-db/src/syntax_helpers/suggest_name.rs index 1e08e8e309806..365d726d2a93c 100644 --- a/src/tools/rust-analyzer/crates/ide-db/src/syntax_helpers/suggest_name.rs +++ b/src/tools/rust-analyzer/crates/ide-db/src/syntax_helpers/suggest_name.rs @@ -2,13 +2,13 @@ use std::{collections::hash_map::Entry, str::FromStr}; -use hir::Semantics; +use hir::{Semantics, SemanticsScope}; use itertools::Itertools; use rustc_hash::FxHashMap; use stdx::to_lower_snake_case; use syntax::{ ast::{self, HasName}, - match_ast, AstNode, Edition, SmolStr, SmolStrBuilder, + match_ast, AstNode, Edition, SmolStr, SmolStrBuilder, ToSmolStr, }; use crate::RootDatabase; @@ -100,6 +100,19 @@ impl NameGenerator { generator } + pub fn new_from_scope_locals(scope: Option>) -> Self { + let mut generator = Self::new(); + if let Some(scope) = scope { + scope.process_all_names(&mut |name, scope| { + if let hir::ScopeDef::Local(_) = scope { + generator.insert(name.as_str()); + } + }); + } + + generator + } + /// Suggest a name without conflicts. If the name conflicts with existing names, /// it will try to resolve the conflict by adding a numeric suffix. pub fn suggest_name(&mut self, name: &str) -> SmolStr { @@ -162,6 +175,59 @@ impl NameGenerator { self.suggest_name(&c.to_string()) } + /// Suggest name of variable for given expression + /// + /// In current implementation, the function tries to get the name from + /// the following sources: + /// + /// * if expr is an argument to function/method, use parameter name + /// * if expr is a function/method call, use function name + /// * expression type name if it exists (E.g. `()`, `fn() -> ()` or `!` do not have names) + /// * fallback: `var_name` + /// + /// It also applies heuristics to filter out less informative names + /// + /// Currently it sticks to the first name found. + pub fn for_variable( + &mut self, + expr: &ast::Expr, + sema: &Semantics<'_, RootDatabase>, + ) -> SmolStr { + // `from_param` does not benefit from stripping it need the largest + // context possible so we check firstmost + if let Some(name) = from_param(expr, sema) { + return self.suggest_name(&name); + } + + let mut next_expr = Some(expr.clone()); + while let Some(expr) = next_expr { + let name = from_call(&expr) + .or_else(|| from_type(&expr, sema)) + .or_else(|| from_field_name(&expr)); + if let Some(name) = name { + return self.suggest_name(&name); + } + + match expr { + ast::Expr::RefExpr(inner) => next_expr = inner.expr(), + ast::Expr::AwaitExpr(inner) => next_expr = inner.expr(), + // ast::Expr::BlockExpr(block) => expr = block.tail_expr(), + ast::Expr::CastExpr(inner) => next_expr = inner.expr(), + ast::Expr::MethodCallExpr(method) if is_useless_method(&method) => { + next_expr = method.receiver(); + } + ast::Expr::ParenExpr(inner) => next_expr = inner.expr(), + ast::Expr::TryExpr(inner) => next_expr = inner.expr(), + ast::Expr::PrefixExpr(prefix) if prefix.op_kind() == Some(ast::UnaryOp::Deref) => { + next_expr = prefix.expr() + } + _ => break, + } + } + + self.suggest_name("var_name") + } + /// Insert a name into the pool fn insert(&mut self, name: &str) { let (prefix, suffix) = Self::split_numeric_suffix(name); @@ -191,63 +257,8 @@ impl NameGenerator { } } -/// Suggest name of variable for given expression -/// -/// **NOTE**: it is caller's responsibility to guarantee uniqueness of the name. -/// I.e. it doesn't look for names in scope. -/// -/// # Current implementation -/// -/// In current implementation, the function tries to get the name from -/// the following sources: -/// -/// * if expr is an argument to function/method, use parameter name -/// * if expr is a function/method call, use function name -/// * expression type name if it exists (E.g. `()`, `fn() -> ()` or `!` do not have names) -/// * fallback: `var_name` -/// -/// It also applies heuristics to filter out less informative names -/// -/// Currently it sticks to the first name found. -// FIXME: Microoptimize and return a `SmolStr` here. -pub fn for_variable(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> String { - // `from_param` does not benefit from stripping - // it need the largest context possible - // so we check firstmost - if let Some(name) = from_param(expr, sema) { - return name; - } - - let mut next_expr = Some(expr.clone()); - while let Some(expr) = next_expr { - let name = - from_call(&expr).or_else(|| from_type(&expr, sema)).or_else(|| from_field_name(&expr)); - if let Some(name) = name { - return name; - } - - match expr { - ast::Expr::RefExpr(inner) => next_expr = inner.expr(), - ast::Expr::AwaitExpr(inner) => next_expr = inner.expr(), - // ast::Expr::BlockExpr(block) => expr = block.tail_expr(), - ast::Expr::CastExpr(inner) => next_expr = inner.expr(), - ast::Expr::MethodCallExpr(method) if is_useless_method(&method) => { - next_expr = method.receiver(); - } - ast::Expr::ParenExpr(inner) => next_expr = inner.expr(), - ast::Expr::TryExpr(inner) => next_expr = inner.expr(), - ast::Expr::PrefixExpr(prefix) if prefix.op_kind() == Some(ast::UnaryOp::Deref) => { - next_expr = prefix.expr() - } - _ => break, - } - } - - "var_name".to_owned() -} - -fn normalize(name: &str) -> Option { - let name = to_lower_snake_case(name); +fn normalize(name: &str) -> Option { + let name = to_lower_snake_case(name).to_smolstr(); if USELESS_NAMES.contains(&name.as_str()) { return None; @@ -280,11 +291,11 @@ fn is_useless_method(method: &ast::MethodCallExpr) -> bool { } } -fn from_call(expr: &ast::Expr) -> Option { +fn from_call(expr: &ast::Expr) -> Option { from_func_call(expr).or_else(|| from_method_call(expr)) } -fn from_func_call(expr: &ast::Expr) -> Option { +fn from_func_call(expr: &ast::Expr) -> Option { let call = match expr { ast::Expr::CallExpr(call) => call, _ => return None, @@ -297,7 +308,7 @@ fn from_func_call(expr: &ast::Expr) -> Option { normalize(ident.text()) } -fn from_method_call(expr: &ast::Expr) -> Option { +fn from_method_call(expr: &ast::Expr) -> Option { let method = match expr { ast::Expr::MethodCallExpr(call) => call, _ => return None, @@ -319,7 +330,7 @@ fn from_method_call(expr: &ast::Expr) -> Option { normalize(name) } -fn from_param(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option { +fn from_param(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option { let arg_list = expr.syntax().parent().and_then(ast::ArgList::cast)?; let args_parent = arg_list.syntax().parent()?; let func = match_ast! { @@ -338,7 +349,7 @@ fn from_param(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option Option { @@ -350,7 +361,7 @@ fn var_name_from_pat(pat: &ast::Pat) -> Option { } } -fn from_type(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option { +fn from_type(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option { let ty = sema.type_of_expr(expr)?.adjusted(); let ty = ty.remove_ref().unwrap_or(ty); let edition = sema.scope(expr.syntax())?.krate().edition(sema.db); @@ -358,7 +369,7 @@ fn from_type(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option Option { +fn name_of_type(ty: &hir::Type, db: &RootDatabase, edition: Edition) -> Option { let name = if let Some(adt) = ty.as_adt() { let name = adt.name(db).display(db, edition).to_string(); @@ -393,7 +404,7 @@ fn trait_name(trait_: &hir::Trait, db: &RootDatabase, edition: Edition) -> Optio Some(name) } -fn from_field_name(expr: &ast::Expr) -> Option { +fn from_field_name(expr: &ast::Expr) -> Option { let field = match expr { ast::Expr::FieldExpr(field) => field, _ => return None, @@ -424,7 +435,7 @@ mod tests { frange.range, "selection is not an expression(yet contained in one)" ); - let name = for_variable(&expr, &sema); + let name = NameGenerator::new().for_variable(&expr, &sema); assert_eq!(&name, expected); }