Mirrors/rust-analyzer
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-analyzer synced 2025-01-26 03:45:04 +00:00
Code Issues Projects Releases Packages Wiki Activity

Merge pull request #18791 from roife/fix-18786

Browse source 
fix: avoid generating colliding names in extract_variable
This commit is contained in:
Lukas Wirth 2024-12-30 10:52:53 +00:00 committed by GitHub
commit 36cd1000a5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 126 additions and 91 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

14
crates/ide-assists/src/handlers/destructure_tuple_binding.rs
View file

@ -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<TupleDat
.map(|(_, refs)| refs.to_vec())
});
let mut name_generator = {
let mut names = vec![];
if let Some(scope) = ctx.sema.scope(ident_pat.syntax()) {
scope.process_all_names(&mut |name, scope| {
if let hir::ScopeDef::Local(_) = scope {
names.push(name.as_str().into())
}
})
}
suggest_name::NameGenerator::new_with_names(names.iter().map(|s: &SmolStr| s.as_str()))
};
let mut name_generator =
suggest_name::NameGenerator::new_from_scope_locals(ctx.sema.scope(ident_pat.syntax()));
let field_names = field_types
.into_iter()

41
crates/ide-assists/src/handlers/extract_variable.rs
View file

@ -336,10 +336,12 @@ impl ExtractionKind {
}
}
let mut name_generator =
suggest_name::NameGenerator::new_from_scope_locals(ctx.sema.scope(to_extract.syntax()));
let var_name = if let Some(literal_name) = get_literal_name(ctx, to_extract) {
literal_name
name_generator.suggest_name(&literal_name)
} else {
suggest_name::for_variable(to_extract, &ctx.sema)
name_generator.for_variable(to_extract, &ctx.sema)
};
let var_name = match self {
@ -352,10 +354,10 @@ impl ExtractionKind {
}
fn get_literal_name(ctx: &AssistContext<'_>, expr: &ast::Expr) -> Option<String> {
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",
);
}
}

13
crates/ide-assists/src/handlers/replace_is_method_with_if_let_method.rs
View file

@ -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 {}
}
"#,
);

2
crates/ide-assists/src/tests/generated.rs
View file

@ -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 {}
}
"#####,
)

147
crates/ide-db/src/syntax_helpers/suggest_name.rs
View file

@ -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<SemanticsScope<'_>>) -> 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<String> {
let name = to_lower_snake_case(name);
fn normalize(name: &str) -> Option<SmolStr> {
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<String> {
fn from_call(expr: &ast::Expr) -> Option<SmolStr> {
from_func_call(expr).or_else(|| from_method_call(expr))
}
fn from_func_call(expr: &ast::Expr) -> Option<String> {
fn from_func_call(expr: &ast::Expr) -> Option<SmolStr> {
let call = match expr {
ast::Expr::CallExpr(call) => call,
_ => return None,
@ -297,7 +308,7 @@ fn from_func_call(expr: &ast::Expr) -> Option<String> {
normalize(ident.text())
}
fn from_method_call(expr: &ast::Expr) -> Option<String> {
fn from_method_call(expr: &ast::Expr) -> Option<SmolStr> {
let method = match expr {
ast::Expr::MethodCallExpr(call) => call,
_ => return None,
@ -319,7 +330,7 @@ fn from_method_call(expr: &ast::Expr) -> Option<String> {
normalize(name)
}
fn from_param(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option<String> {
fn from_param(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option<SmolStr> {
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<St
let param = func.params().into_iter().nth(idx)?;
let pat = sema.source(param)?.value.right()?.pat()?;
let name = var_name_from_pat(&pat)?;
normalize(&name.to_string())
normalize(&name.to_smolstr())
}
fn var_name_from_pat(pat: &ast::Pat) -> Option<ast::Name> {
@ -350,7 +361,7 @@ fn var_name_from_pat(pat: &ast::Pat) -> Option<ast::Name> {
}
}
fn from_type(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option<String> {
fn from_type(expr: &ast::Expr, sema: &Semantics<'_, RootDatabase>) -> Option<SmolStr> {
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<Str
name_of_type(&ty, sema.db, edition)
}
fn name_of_type(ty: &hir::Type, db: &RootDatabase, edition: Edition) -> Option<String> {
fn name_of_type(ty: &hir::Type, db: &RootDatabase, edition: Edition) -> Option<SmolStr> {
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<String> {
fn from_field_name(expr: &ast::Expr) -> Option<SmolStr> {
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);
}