From 5e533e5900d9dfb40f097811fdc63b5803ef3dbe Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 16 Aug 2021 22:48:38 +0200 Subject: [PATCH 1/3] Handle all rename special cases for all record pattern fields --- crates/ide/src/rename.rs | 68 +++++++++++++++++++++- crates/ide_db/src/helpers/import_assets.rs | 4 +- crates/ide_db/src/rename.rs | 60 +++++++++++++------ crates/text_edit/src/lib.rs | 3 + 4 files changed, 112 insertions(+), 23 deletions(-) diff --git a/crates/ide/src/rename.rs b/crates/ide/src/rename.rs index d0a49d1f37..39c5861542 100644 --- a/crates/ide/src/rename.rs +++ b/crates/ide/src/rename.rs @@ -1332,9 +1332,71 @@ fn foo(foo: Foo) { struct Foo { baz: i32 } fn foo(foo: Foo) { - let Foo { ref baz @ qux } = foo; + let Foo { baz: ref baz @ qux } = foo; let _ = qux; } +"#, + ); + check( + "baz", + r#" +struct Foo { i$0: i32 } + +fn foo(foo: Foo) { + let Foo { i: ref baz } = foo; + let _ = qux; +} +"#, + r#" +struct Foo { baz: i32 } + +fn foo(foo: Foo) { + let Foo { ref baz } = foo; + let _ = qux; +} +"#, + ); + } + + #[test] + fn test_struct_local_pat_into_shorthand() { + cov_mark::check!(test_rename_local_put_init_shorthand_pat); + check( + "field", + r#" +struct Foo { field: i32 } + +fn foo(foo: Foo) { + let Foo { field: qux$0 } = foo; + let _ = qux; +} +"#, + r#" +struct Foo { field: i32 } + +fn foo(foo: Foo) { + let Foo { field } = foo; + let _ = field; +} +"#, + ); + check( + "field", + r#" +struct Foo { field: i32 } + +fn foo(foo: Foo) { + let Foo { field: x @ qux$0 } = foo; + let _ = qux; +} +"#, + r#" +struct Foo { field: i32 } + +fn foo(foo: Foo) { + let Foo { field: x @ field } = foo; + let _ = field; +} "#, ); } @@ -1390,7 +1452,7 @@ struct Foo { i: i32 } -fn foo(Foo { i }: foo) -> i32 { +fn foo(Foo { i }: Foo) -> i32 { i$0 } "#, @@ -1399,7 +1461,7 @@ struct Foo { i: i32 } -fn foo(Foo { i: bar }: foo) -> i32 { +fn foo(Foo { i: bar }: Foo) -> i32 { bar } "#, diff --git a/crates/ide_db/src/helpers/import_assets.rs b/crates/ide_db/src/helpers/import_assets.rs index 3433398eba..a672c10917 100644 --- a/crates/ide_db/src/helpers/import_assets.rs +++ b/crates/ide_db/src/helpers/import_assets.rs @@ -120,11 +120,11 @@ impl ImportAssets { } pub fn for_ident_pat(pat: &ast::IdentPat, sema: &Semantics) -> Option { - let name = pat.name()?; - let candidate_node = pat.syntax().clone(); if !pat.is_simple_ident() { return None; } + let name = pat.name()?; + let candidate_node = pat.syntax().clone(); Some(Self { import_candidate: ImportCandidate::for_name(sema, &name)?, module_with_candidate: sema.scope(&candidate_node).module()?, diff --git a/crates/ide_db/src/rename.rs b/crates/ide_db/src/rename.rs index 4ca3260877..6d8d8a2b8f 100644 --- a/crates/ide_db/src/rename.rs +++ b/crates/ide_db/src/rename.rs @@ -320,6 +320,7 @@ pub fn source_edit_from_references( .unwrap_or_else(|| (reference.range, new_name.to_string())); edit.replace(range, replacement); } + edit.finish() } @@ -334,6 +335,7 @@ fn source_edit_from_name(name: &ast::Name, new_name: &str) -> Option<(TextRange, )); } } + None } @@ -387,7 +389,9 @@ fn source_edit_from_name_ref( let rcf_pat = record_field.pat(); match (rcf_name_ref, rcf_pat) { // field: rename - (Some(field_name), Some(ast::Pat::IdentPat(pat))) if field_name == *name_ref => { + (Some(field_name), Some(ast::Pat::IdentPat(pat))) + if field_name == *name_ref && pat.at_token().is_none() => + { // field name is being renamed if pat.name().map_or(false, |it| it.text() == new_name) { cov_mark::hit!(test_rename_field_put_init_shorthand_pat); @@ -412,32 +416,52 @@ fn source_edit_from_def( def: Definition, new_name: &str, ) -> Result<(FileId, TextEdit)> { - let frange = def + let FileRange { file_id, range } = def .range_for_rename(sema) .ok_or_else(|| format_err!("No identifier available to rename"))?; - let mut replacement_text = String::new(); - let mut repl_range = frange.range; + let mut edit = TextEdit::builder(); if let Definition::Local(local) = def { if let Either::Left(pat) = local.source(sema.db).value { - if matches!( - pat.syntax().parent().and_then(ast::RecordPatField::cast), - Some(pat_field) if pat_field.name_ref().is_none() - ) { - replacement_text.push_str(": "); - replacement_text.push_str(new_name); - repl_range = TextRange::new( - pat.syntax().text_range().end(), - pat.syntax().text_range().end(), - ); + // special cases required for renaming fields/locals in Record patterns + if let Some(pat_field) = pat.syntax().parent().and_then(ast::RecordPatField::cast) { + let name_range = pat.name().unwrap().syntax().text_range(); + if let Some(name_ref) = pat_field.name_ref() { + if new_name == name_ref.text() && pat.at_token().is_none() { + // Foo { field: ref mut local } -> Foo { ref mut field } + // ^^^^^^ delete this + // ^^^^^ replace this with `field` + cov_mark::hit!(test_rename_local_put_init_shorthand_pat); + edit.delete( + name_ref + .syntax() + .text_range() + .cover_offset(pat.syntax().text_range().start()), + ); + edit.replace(name_range, name_ref.text().to_string()); + } else { + // Foo { field: ref mut local @ local 2} -> Foo { field: ref mut new_name @ local2 } + // Foo { field: ref mut local } -> Foo { field: ref mut new_name } + // ^^^^^ replace this with `new_name` + edit.replace(name_range, new_name.to_string()); + } + } else { + // Foo { ref mut field } -> Foo { field: ref mut new_name } + // ^ insert `field: ` + // ^^^^^ replace this with `new_name` + edit.insert( + pat.syntax().text_range().start(), + format!("{}: ", pat_field.field_name().unwrap()), + ); + edit.replace(name_range, new_name.to_string()); + } } } } - if replacement_text.is_empty() { - replacement_text.push_str(new_name); + if edit.is_empty() { + edit.replace(range, new_name.to_string()); } - let edit = TextEdit::replace(repl_range, replacement_text); - Ok((frange.file_id, edit)) + Ok((file_id, edit.finish())) } #[derive(Copy, Clone, Debug, PartialEq)] diff --git a/crates/text_edit/src/lib.rs b/crates/text_edit/src/lib.rs index 685039c4be..a43ffe202f 100644 --- a/crates/text_edit/src/lib.rs +++ b/crates/text_edit/src/lib.rs @@ -159,6 +159,9 @@ impl<'a> IntoIterator for &'a TextEdit { } impl TextEditBuilder { + pub fn is_empty(&self) -> bool { + self.indels.is_empty() + } pub fn replace(&mut self, range: TextRange, replace_with: String) { self.indel(Indel::replace(range, replace_with)) } From 995c8f50a2a544da56f8320d33d44b1ab57f0935 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 16 Aug 2021 23:06:51 +0200 Subject: [PATCH 2/3] some code docs for the ide_db/rename module --- crates/ide/src/rename.rs | 1 + crates/ide_db/src/rename.rs | 33 +++++++++++++++++++++++++++------ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/crates/ide/src/rename.rs b/crates/ide/src/rename.rs index 39c5861542..d8021a0e31 100644 --- a/crates/ide/src/rename.rs +++ b/crates/ide/src/rename.rs @@ -1470,6 +1470,7 @@ fn foo(Foo { i: bar }: Foo) -> i32 { #[test] fn test_struct_field_complex_ident_pat() { + cov_mark::check!(rename_record_pat_field_name_split); check( "baz", r#" diff --git a/crates/ide_db/src/rename.rs b/crates/ide_db/src/rename.rs index 6d8d8a2b8f..7cdb31bd06 100644 --- a/crates/ide_db/src/rename.rs +++ b/crates/ide_db/src/rename.rs @@ -326,12 +326,16 @@ pub fn source_edit_from_references( fn source_edit_from_name(name: &ast::Name, new_name: &str) -> Option<(TextRange, String)> { if let Some(_) = ast::RecordPatField::for_field_name(name) { - // FIXME: instead of splitting the shorthand, recursively trigger a rename of the - // other name https://github.com/rust-analyzer/rust-analyzer/issues/6547 if let Some(ident_pat) = name.syntax().parent().and_then(ast::IdentPat::cast) { + cov_mark::hit!(rename_record_pat_field_name_split); + // Foo { ref mut field } -> Foo { new_name: ref mut field } + // ^ insert `new_name: ` + + // FIXME: instead of splitting the shorthand, recursively trigger a rename of the + // other name https://github.com/rust-analyzer/rust-analyzer/issues/6547 return Some(( TextRange::empty(ident_pat.syntax().text_range().start()), - [new_name, ": "].concat(), + format!("{}: ", new_name), )); } } @@ -347,21 +351,29 @@ fn source_edit_from_name_ref( if let Some(record_field) = ast::RecordExprField::for_name_ref(name_ref) { let rcf_name_ref = record_field.name_ref(); let rcf_expr = record_field.expr(); - match (rcf_name_ref, rcf_expr.and_then(|it| it.name_ref())) { + match &(rcf_name_ref, rcf_expr.and_then(|it| it.name_ref())) { // field: init-expr, check if we can use a field init shorthand (Some(field_name), Some(init)) => { - if field_name == *name_ref { + if field_name == name_ref { if init.text() == new_name { cov_mark::hit!(test_rename_field_put_init_shorthand); + // Foo { field: local } -> Foo { local } + // ^^^^^^^^ delete this + // FIXME: Actually delete this instead of replacing the entire thing + // same names, we can use a shorthand here instead. // we do not want to erase attributes hence this range start let s = field_name.syntax().text_range().start(); let e = record_field.syntax().text_range().end(); return Some((TextRange::new(s, e), new_name.to_owned())); } - } else if init == *name_ref { + } else if init == name_ref { if field_name.text() == new_name { cov_mark::hit!(test_rename_local_put_init_shorthand); + // Foo { field: local } -> Foo { field } + // ^^^^^^^ delete this + // FIXME: Actually delete this instead of replacing the entire thing + // same names, we can use a shorthand here instead. // we do not want to erase attributes hence this range start let s = field_name.syntax().text_range().start(); @@ -374,11 +386,15 @@ fn source_edit_from_name_ref( // init shorthand (None, Some(_)) if matches!(def, Definition::Field(_)) => { cov_mark::hit!(test_rename_field_in_field_shorthand); + // Foo { field } -> Foo { new_name: field } + // ^ insert `new_name: ` let s = name_ref.syntax().text_range().start(); Some((TextRange::empty(s), format!("{}: ", new_name))) } (None, Some(_)) if matches!(def, Definition::Local(_)) => { cov_mark::hit!(test_rename_local_in_field_shorthand); + // Foo { field } -> Foo { field: new_name } + // ^ insert `: new_name` let s = name_ref.syntax().text_range().end(); Some((TextRange::empty(s), format!(": {}", new_name))) } @@ -395,6 +411,11 @@ fn source_edit_from_name_ref( // field name is being renamed if pat.name().map_or(false, |it| it.text() == new_name) { cov_mark::hit!(test_rename_field_put_init_shorthand_pat); + // Foo { field: ref mut local } -> Foo { ref mut field } + // ^^^^^^^ delete this + // ^^^^^ replace this with `field` + // FIXME: do this the way its written here + // same names, we can use a shorthand here instead/ // we do not want to erase attributes hence this range start let s = field_name.syntax().text_range().start(); From daf3094958cbf69b0e179a0c7320ad8ef63fe347 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 17 Aug 2021 15:24:01 +0200 Subject: [PATCH 3/3] Emit more concise text edits in ide_db::rename --- crates/ide/src/rename.rs | 1 + crates/ide_db/src/rename.rs | 89 +++++++++++++++++++------------------ 2 files changed, 46 insertions(+), 44 deletions(-) diff --git a/crates/ide/src/rename.rs b/crates/ide/src/rename.rs index d8021a0e31..9d452f40ba 100644 --- a/crates/ide/src/rename.rs +++ b/crates/ide/src/rename.rs @@ -274,6 +274,7 @@ mod tests { use super::{RangeInfo, RenameError}; + #[track_caller] fn check(new_name: &str, ra_fixture_before: &str, ra_fixture_after: &str) { let ra_fixture_after = &trim_indent(ra_fixture_after); let (analysis, position) = fixture::position(ra_fixture_before); diff --git a/crates/ide_db/src/rename.rs b/crates/ide_db/src/rename.rs index 7cdb31bd06..ef8b31ba5f 100644 --- a/crates/ide_db/src/rename.rs +++ b/crates/ide_db/src/rename.rs @@ -30,7 +30,7 @@ use syntax::{ ast::{self, NameOwner}, lex_single_syntax_kind, AstNode, SyntaxKind, TextRange, T, }; -use text_edit::TextEdit; +use text_edit::{TextEdit, TextEditBuilder}; use crate::{ defs::Definition, @@ -303,28 +303,29 @@ pub fn source_edit_from_references( ) -> TextEdit { let mut edit = TextEdit::builder(); for reference in references { - let (range, replacement) = match &reference.name { + let has_emitted_edit = match &reference.name { // if the ranges differ then the node is inside a macro call, we can't really attempt // to make special rewrites like shorthand syntax and such, so just rename the node in // the macro input ast::NameLike::NameRef(name_ref) if name_ref.syntax().text_range() == reference.range => { - source_edit_from_name_ref(name_ref, new_name, def) + source_edit_from_name_ref(&mut edit, name_ref, new_name, def) } ast::NameLike::Name(name) if name.syntax().text_range() == reference.range => { - source_edit_from_name(name, new_name) + source_edit_from_name(&mut edit, name, new_name) } - _ => None, + _ => false, + }; + if !has_emitted_edit { + edit.replace(reference.range, new_name.to_string()); } - .unwrap_or_else(|| (reference.range, new_name.to_string())); - edit.replace(range, replacement); } edit.finish() } -fn source_edit_from_name(name: &ast::Name, new_name: &str) -> Option<(TextRange, String)> { +fn source_edit_from_name(edit: &mut TextEditBuilder, name: &ast::Name, new_name: &str) -> bool { if let Some(_) = ast::RecordPatField::for_field_name(name) { if let Some(ident_pat) = name.syntax().parent().and_then(ast::IdentPat::cast) { cov_mark::hit!(rename_record_pat_field_name_split); @@ -333,21 +334,20 @@ fn source_edit_from_name(name: &ast::Name, new_name: &str) -> Option<(TextRange, // FIXME: instead of splitting the shorthand, recursively trigger a rename of the // other name https://github.com/rust-analyzer/rust-analyzer/issues/6547 - return Some(( - TextRange::empty(ident_pat.syntax().text_range().start()), - format!("{}: ", new_name), - )); + edit.insert(ident_pat.syntax().text_range().start(), format!("{}: ", new_name)); + return true; } } - None + false } fn source_edit_from_name_ref( + edit: &mut TextEditBuilder, name_ref: &ast::NameRef, new_name: &str, def: Definition, -) -> Option<(TextRange, String)> { +) -> bool { if let Some(record_field) = ast::RecordExprField::for_name_ref(name_ref) { let rcf_name_ref = record_field.name_ref(); let rcf_expr = record_field.expr(); @@ -358,47 +358,48 @@ fn source_edit_from_name_ref( if init.text() == new_name { cov_mark::hit!(test_rename_field_put_init_shorthand); // Foo { field: local } -> Foo { local } - // ^^^^^^^^ delete this - // FIXME: Actually delete this instead of replacing the entire thing + // ^^^^^^^ delete this // same names, we can use a shorthand here instead. // we do not want to erase attributes hence this range start let s = field_name.syntax().text_range().start(); - let e = record_field.syntax().text_range().end(); - return Some((TextRange::new(s, e), new_name.to_owned())); + let e = init.syntax().text_range().start(); + edit.delete(TextRange::new(s, e)); + return true; } } else if init == name_ref { if field_name.text() == new_name { cov_mark::hit!(test_rename_local_put_init_shorthand); // Foo { field: local } -> Foo { field } // ^^^^^^^ delete this - // FIXME: Actually delete this instead of replacing the entire thing // same names, we can use a shorthand here instead. // we do not want to erase attributes hence this range start - let s = field_name.syntax().text_range().start(); - let e = record_field.syntax().text_range().end(); - return Some((TextRange::new(s, e), new_name.to_owned())); + let s = field_name.syntax().text_range().end(); + let e = init.syntax().text_range().end(); + edit.delete(TextRange::new(s, e)); + return true; } } - None } // init shorthand (None, Some(_)) if matches!(def, Definition::Field(_)) => { cov_mark::hit!(test_rename_field_in_field_shorthand); // Foo { field } -> Foo { new_name: field } // ^ insert `new_name: ` - let s = name_ref.syntax().text_range().start(); - Some((TextRange::empty(s), format!("{}: ", new_name))) + let offset = name_ref.syntax().text_range().start(); + edit.insert(offset, format!("{}: ", new_name)); + return true; } (None, Some(_)) if matches!(def, Definition::Local(_)) => { cov_mark::hit!(test_rename_local_in_field_shorthand); // Foo { field } -> Foo { field: new_name } // ^ insert `: new_name` - let s = name_ref.syntax().text_range().end(); - Some((TextRange::empty(s), format!(": {}", new_name))) + let offset = name_ref.syntax().text_range().end(); + edit.insert(offset, format!(": {}", new_name)); + return true; } - _ => None, + _ => (), } } else if let Some(record_field) = ast::RecordPatField::for_field_name_ref(name_ref) { let rcf_name_ref = record_field.name_ref(); @@ -409,27 +410,27 @@ fn source_edit_from_name_ref( if field_name == *name_ref && pat.at_token().is_none() => { // field name is being renamed - if pat.name().map_or(false, |it| it.text() == new_name) { - cov_mark::hit!(test_rename_field_put_init_shorthand_pat); - // Foo { field: ref mut local } -> Foo { ref mut field } - // ^^^^^^^ delete this - // ^^^^^ replace this with `field` - // FIXME: do this the way its written here + if let Some(name) = pat.name() { + if name.text() == new_name { + cov_mark::hit!(test_rename_field_put_init_shorthand_pat); + // Foo { field: ref mut local } -> Foo { ref mut field } + // ^^^^^^^ delete this + // ^^^^^ replace this with `field` - // same names, we can use a shorthand here instead/ - // we do not want to erase attributes hence this range start - let s = field_name.syntax().text_range().start(); - let e = record_field.syntax().text_range().end(); - Some((TextRange::new(s, e), pat.to_string())) - } else { - None + // same names, we can use a shorthand here instead/ + // we do not want to erase attributes hence this range start + let s = field_name.syntax().text_range().start(); + let e = pat.syntax().text_range().start(); + edit.delete(TextRange::new(s, e)); + edit.replace(name.syntax().text_range(), new_name.to_string()); + return true; + } } } - _ => None, + _ => (), } - } else { - None } + false } fn source_edit_from_def(