Merge #9929

9929: fix: Handle all rename special cases for record pattern fields r=Veykril a=Veykril



Co-authored-by: Lukas Wirth <[email protected]>

Author: bors[bot]

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);
@@ -1332,9 +1333,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 +1453,7 @@ struct Foo {
     i: i32
 }
 
-fn foo(Foo { i }: foo) -> i32 {
+fn foo(Foo { i }: Foo) -> i32 {
     i$0
 }
 "#,
@@ -1399,7 +1462,7 @@ struct Foo {
     i: i32
 }
 
-fn foo(Foo { i: bar }: foo) -> i32 {
+fn foo(Foo { i: bar }: Foo) -> i32 {
     bar
 }
 "#,
@@ -1408,6 +1471,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#"

crates/ide_db/src/helpers/import_assets.rs

@@ -120,11 +120,11 @@ impl ImportAssets {
     }
 
     pub fn for_ident_pat(pat: &ast::IdentPat, sema: &Semantics<RootDatabase>) -> Option<Self> {
-        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()?,

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,141 +303,187 @@ 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) {
-        // 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) {
-            return Some((
-                TextRange::empty(ident_pat.syntax().text_range().start()),
-                [new_name, ": "].concat(),
-            ));
+            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
+            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();
-        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
+
                         // 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 {
+                } 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
+
                         // 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);
-                let s = name_ref.syntax().text_range().start();
-                Some((TextRange::empty(s), format!("{}: ", new_name)))
+                // Foo { field } -> Foo { new_name: field }
+                //       ^ insert `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);
-                let s = name_ref.syntax().text_range().end();
-                Some((TextRange::empty(s), format!(": {}", new_name)))
+                // Foo { field } -> Foo { field: new_name }
+                //            ^ insert `: 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();
         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);
-                    // 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
+                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 = 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(
     sema: &Semantics<RootDatabase>,
     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)]

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))
     }