Merge #10645

10645: fix: make `rename` multi-token mapping aware r=Veykril a=spookyvision



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

Author: 4 people

crates/ide/src/rename.rs

@@ -3,13 +3,15 @@
 //! This is mostly front-end for [`ide_db::rename`], but it also includes the
 //! tests. This module also implements a couple of magic tricks, like renaming
 //! `self` and to `self` (to switch between associated function and method).
+
 use hir::{AsAssocItem, InFile, Semantics};
 use ide_db::{
     base_db::FileId,
     defs::{Definition, NameClass, NameRefClass},
     rename::{bail, format_err, source_edit_from_references, IdentifierKind},
     RootDatabase,
 };
+use itertools::Itertools;
 use stdx::{always, never};
 use syntax::{ast, AstNode, SyntaxNode};
 
@@ -31,14 +33,33 @@ pub(crate) fn prepare_rename(
     let source_file = sema.parse(position.file_id);
     let syntax = source_file.syntax();
 
-    let (name_like, def) = find_definition(&sema, syntax, position)?;
-    if def.range_for_rename(&sema).is_none() {
-        bail!("No references found at position")
-    }
+    let res = find_definitions(&sema, syntax, position)?
+        .map(|(name_like, def)| {
+            // ensure all ranges are valid
 
-    let frange = sema.original_range(name_like.syntax());
-    always!(frange.range.contains_inclusive(position.offset) && frange.file_id == position.file_id);
-    Ok(RangeInfo::new(frange.range, ()))
+            if def.range_for_rename(&sema).is_none() {
+                bail!("No references found at position")
+            }
+            let frange = sema.original_range(name_like.syntax());
+
+            always!(
+                frange.range.contains_inclusive(position.offset)
+                    && frange.file_id == position.file_id
+            );
+            Ok(frange.range)
+        })
+        .reduce(|acc, cur| match (acc, cur) {
+            // ensure all ranges are the same
+            (Ok(acc_inner), Ok(cur_inner)) if acc_inner == cur_inner => Ok(acc_inner),
+            (Err(e), _) => Err(e),
+            _ => bail!("inconsistent text range"),
+        });
+
+    match res {
+        // ensure at least one definition was found
+        Some(res) => res.map(|range| RangeInfo::new(range, ())),
+        None => bail!("No references found at position"),
+    }
 }
 
 // Feature: Rename
@@ -61,20 +82,27 @@ pub(crate) fn rename(
     let source_file = sema.parse(position.file_id);
     let syntax = source_file.syntax();
 
-    let (_name_like, def) = find_definition(&sema, syntax, position)?;
+    let defs = find_definitions(&sema, syntax, position)?;
 
-    if let Definition::Local(local) = def {
-        if let Some(self_param) = local.as_self_param(sema.db) {
-            cov_mark::hit!(rename_self_to_param);
-            return rename_self_to_param(&sema, local, self_param, new_name);
-        }
-        if new_name == "self" {
-            cov_mark::hit!(rename_to_self);
-            return rename_to_self(&sema, local);
-        }
-    }
+    let ops: RenameResult<Vec<SourceChange>> = defs
+        .map(|(_namelike, def)| {
+            if let Definition::Local(local) = def {
+                if let Some(self_param) = local.as_self_param(sema.db) {
+                    cov_mark::hit!(rename_self_to_param);
+                    return rename_self_to_param(&sema, local, self_param, new_name);
+                }
+                if new_name == "self" {
+                    cov_mark::hit!(rename_to_self);
+                    return rename_to_self(&sema, local);
+                }
+            }
+            def.rename(&sema, new_name)
+        })
+        .collect();
 
-    def.rename(&sema, new_name)
+    ops?.into_iter()
+        .reduce(|acc, elem| acc.merge(elem))
+        .ok_or_else(|| format_err!("No references found at position"))
 }
 
 /// Called by the client when it is about to rename a file.
@@ -91,59 +119,86 @@ pub(crate) fn will_rename_file(
     Some(change)
 }
 
-fn find_definition(
+fn find_definitions(
     sema: &Semantics<RootDatabase>,
     syntax: &SyntaxNode,
     position: FilePosition,
-) -> RenameResult<(ast::NameLike, Definition)> {
-    let name_like = sema
-        .find_node_at_offset_with_descend::<ast::NameLike>(syntax, position.offset)
-        .ok_or_else(|| format_err!("No references found at position"))?;
-
-    let def = match &name_like {
-        // renaming aliases would rename the item being aliased as the HIR doesn't track aliases yet
-        ast::NameLike::Name(name)
-            if name.syntax().parent().map_or(false, |it| ast::Rename::can_cast(it.kind())) =>
-        {
-            bail!("Renaming aliases is currently unsupported")
-        }
-        ast::NameLike::Name(name) => NameClass::classify(sema, name).map(|class| match class {
-            NameClass::Definition(it) | NameClass::ConstReference(it) => it,
-            NameClass::PatFieldShorthand { local_def, field_ref: _ } => {
-                Definition::Local(local_def)
-            }
-        }),
-        ast::NameLike::NameRef(name_ref) => {
-            if let Some(def) = NameRefClass::classify(sema, name_ref).map(|class| match class {
-                NameRefClass::Definition(def) => def,
-                NameRefClass::FieldShorthand { local_ref, field_ref: _ } => {
-                    Definition::Local(local_ref)
+) -> RenameResult<impl Iterator<Item = (ast::NameLike, Definition)>> {
+    let symbols = sema
+        .find_nodes_at_offset_with_descend::<ast::NameLike>(syntax, position.offset)
+        .map(|name_like| {
+            let res = match &name_like {
+                // renaming aliases would rename the item being aliased as the HIR doesn't track aliases yet
+                ast::NameLike::Name(name)
+                    if name
+                        .syntax()
+                        .parent()
+                        .map_or(false, |it| ast::Rename::can_cast(it.kind())) =>
+                {
+                    bail!("Renaming aliases is currently unsupported")
+                }
+                ast::NameLike::Name(name) => NameClass::classify(sema, name)
+                    .map(|class| match class {
+                        NameClass::Definition(it) | NameClass::ConstReference(it) => it,
+                        NameClass::PatFieldShorthand { local_def, field_ref: _ } => {
+                            Definition::Local(local_def)
+                        }
+                    })
+                    .map(|def| (name_like.clone(), def))
+                    .ok_or_else(|| format_err!("No references found at position")),
+                ast::NameLike::NameRef(name_ref) => {
+                    NameRefClass::classify(sema, name_ref)
+                        .map(|class| match class {
+                            NameRefClass::Definition(def) => def,
+                            NameRefClass::FieldShorthand { local_ref, field_ref: _ } => {
+                                Definition::Local(local_ref)
+                            }
+                        })
+                        .ok_or_else(|| format_err!("No references found at position"))
+                        .and_then(|def| {
+                            // if the name differs from the definitions name it has to be an alias
+                            if def
+                                .name(sema.db)
+                                .map_or(false, |it| it.to_string() != name_ref.text())
+                            {
+                                Err(format_err!("Renaming aliases is currently unsupported"))
+                            } else {
+                                Ok((name_like.clone(), def))
+                            }
+                        })
                 }
-            }) {
-                // if the name differs from the definitions name it has to be an alias
-                if def.name(sema.db).map_or(false, |it| it.to_string() != name_ref.text()) {
-                    bail!("Renaming aliases is currently unsupported");
+                ast::NameLike::Lifetime(lifetime) => {
+                    NameRefClass::classify_lifetime(sema, lifetime)
+                        .and_then(|class| match class {
+                            NameRefClass::Definition(def) => Some(def),
+                            _ => None,
+                        })
+                        .or_else(|| {
+                            NameClass::classify_lifetime(sema, lifetime).and_then(|it| match it {
+                                NameClass::Definition(it) => Some(it),
+                                _ => None,
+                            })
+                        })
+                        .map(|def| (name_like, def))
+                        .ok_or_else(|| format_err!("No references found at position"))
                 }
-                Some(def)
+            };
+            res
+        });
+
+    let res: RenameResult<Vec<_>> = symbols.collect();
+    match res {
+        Ok(v) => {
+            if v.is_empty() {
+                // FIXME: some semantic duplication between "empty vec" and "Err()"
+                Err(format_err!("No references found at position"))
             } else {
-                None
+                // remove duplicates, comparing `Definition`s
+                Ok(v.into_iter().unique_by(|t| t.1))
             }
         }
-        ast::NameLike::Lifetime(lifetime) => NameRefClass::classify_lifetime(sema, lifetime)
-            .and_then(|class| match class {
-                NameRefClass::Definition(def) => Some(def),
-                _ => None,
-            })
-            .or_else(|| {
-                NameClass::classify_lifetime(sema, lifetime).and_then(|it| match it {
-                    NameClass::Definition(it) => Some(it),
-                    _ => None,
-                })
-            }),
+        Err(e) => Err(e),
     }
-    .ok_or_else(|| format_err!("No references found at position"))?;
-
-    Ok((name_like, def))
 }
 
 fn rename_to_self(sema: &Semantics<RootDatabase>, local: hir::Local) -> RenameResult<SourceChange> {
@@ -515,6 +570,36 @@ fn main() {
         );
     }
 
+    #[test]
+    fn test_rename_macro_multiple_occurrences() {
+        check(
+            "Baaah",
+            r#"macro_rules! foo {
+    ($ident:ident) => {
+        const $ident: () = ();
+        struct $ident {}
+    };
+}
+
+foo!($0Foo);
+const _: () = Foo;
+const _: Foo = Foo {};
+    "#,
+            r#"
+macro_rules! foo {
+    ($ident:ident) => {
+        const $ident: () = ();
+        struct $ident {}
+    };
+}
+
+foo!(Baaah);
+const _: () = Baaah;
+const _: Baaah = Baaah {};
+    "#,
+        )
+    }
+
     #[test]
     fn test_rename_for_macro_args() {
         check(

crates/ide_db/src/source_change.rs

@@ -54,6 +54,13 @@ impl SourceChange {
     pub fn get_source_edit(&self, file_id: FileId) -> Option<&TextEdit> {
         self.source_file_edits.get(&file_id)
     }
+
+    pub fn merge(mut self, other: SourceChange) -> SourceChange {
+        self.extend(other.source_file_edits);
+        self.extend(other.file_system_edits);
+        self.is_snippet |= other.is_snippet;
+        self
+    }
 }
 
 impl Extend<(FileId, TextEdit)> for SourceChange {
@@ -62,6 +69,12 @@ impl Extend<(FileId, TextEdit)> for SourceChange {
     }
 }
 
+impl Extend<FileSystemEdit> for SourceChange {
+    fn extend<T: IntoIterator<Item = FileSystemEdit>>(&mut self, iter: T) {
+        iter.into_iter().for_each(|edit| self.push_file_system_edit(edit));
+    }
+}
+
 impl From<FxHashMap<FileId, TextEdit>> for SourceChange {
     fn from(source_file_edits: FxHashMap<FileId, TextEdit>) -> SourceChange {
         SourceChange { source_file_edits, file_system_edits: Vec::new(), is_snippet: false }

crates/text_edit/src/lib.rs

@@ -3,12 +3,13 @@
 //! `rust-analyzer` never mutates text itself and only sends diffs to clients,
 //! so `TextEdit` is the ultimate representation of the work done by
 //! rust-analyzer.
+
 pub use text_size::{TextRange, TextSize};
 
 /// `InsertDelete` -- a single "atomic" change to text
 ///
 /// Must not overlap with other `InDel`s
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct Indel {
     pub insert: String,
     /// Refers to offsets in the original text
@@ -116,11 +117,14 @@ impl TextEdit {
     pub fn union(&mut self, other: TextEdit) -> Result<(), TextEdit> {
         // FIXME: can be done without allocating intermediate vector
         let mut all = self.iter().chain(other.iter()).collect::<Vec<_>>();
-        if !check_disjoint(&mut all) {
+        if !check_disjoint_and_sort(&mut all) {
             return Err(other);
         }
+
         self.indels.extend(other.indels);
-        assert_disjoint(&mut self.indels);
+        check_disjoint_and_sort(&mut self.indels);
+        // Only dedup deletions and replacements, keep all insertions
+        self.indels.dedup_by(|a, b| a == b && !a.delete.is_empty());
         Ok(())
     }
 
@@ -173,7 +177,7 @@ impl TextEditBuilder {
     }
     pub fn finish(self) -> TextEdit {
         let mut indels = self.indels;
-        assert_disjoint(&mut indels);
+        assert_disjoint_or_equal(&mut indels);
         TextEdit { indels }
     }
     pub fn invalidates_offset(&self, offset: TextSize) -> bool {
@@ -182,18 +186,20 @@ impl TextEditBuilder {
     fn indel(&mut self, indel: Indel) {
         self.indels.push(indel);
         if self.indels.len() <= 16 {
-            assert_disjoint(&mut self.indels);
+            assert_disjoint_or_equal(&mut self.indels);
         }
     }
 }
 
-fn assert_disjoint(indels: &mut [impl std::borrow::Borrow<Indel>]) {
-    assert!(check_disjoint(indels));
+fn assert_disjoint_or_equal(indels: &mut [Indel]) {
+    assert!(check_disjoint_and_sort(indels));
 }
-fn check_disjoint(indels: &mut [impl std::borrow::Borrow<Indel>]) -> bool {
+// FIXME: Remove the impl Bound here, it shouldn't be needed
+fn check_disjoint_and_sort(indels: &mut [impl std::borrow::Borrow<Indel>]) -> bool {
     indels.sort_by_key(|indel| (indel.borrow().delete.start(), indel.borrow().delete.end()));
-    indels
-        .iter()
-        .zip(indels.iter().skip(1))
-        .all(|(l, r)| l.borrow().delete.end() <= r.borrow().delete.start())
+    indels.iter().zip(indels.iter().skip(1)).all(|(l, r)| {
+        let l = l.borrow();
+        let r = r.borrow();
+        l.delete.end() <= r.delete.start() || l == r
+    })
 }