Rename fails on renaming definitions created by macros

crates/hir_expand/src/lib.rs

@@ -22,8 +22,7 @@ use either::Either;
 pub use mbe::{ExpandError, ExpandResult};
 pub use parser::FragmentKind;
 
-use std::hash::Hash;
+use std::{hash::Hash, sync::Arc};
-use std::sync::Arc;
 
 use base_db::{impl_intern_key, salsa, CrateId, FileId, FileRange};
 use syntax::{
@@ -32,11 +31,13 @@ use syntax::{
     Direction, SyntaxNode, SyntaxToken, TextRange, TextSize,
 };
 
-use crate::ast_id_map::FileAstId;
+use crate::{
-use crate::builtin_attr::BuiltinAttrExpander;
+    ast_id_map::FileAstId,
-use crate::builtin_derive::BuiltinDeriveExpander;
+    builtin_attr::BuiltinAttrExpander,
-use crate::builtin_macro::{BuiltinFnLikeExpander, EagerExpander};
+    builtin_derive::BuiltinDeriveExpander,
-use crate::proc_macro::ProcMacroExpander;
+    builtin_macro::{BuiltinFnLikeExpander, EagerExpander},
+    proc_macro::ProcMacroExpander,
+};
 
 #[cfg(test)]
 mod test_db;
@@ -210,12 +211,12 @@ impl MacroDefId {
 
     pub fn ast_id(&self) -> Either<AstId<ast::Macro>, AstId<ast::Fn>> {
         let id = match &self.kind {
-            MacroDefKind::Declarative(id) => id,
-            MacroDefKind::BuiltIn(_, id) => id,
-            MacroDefKind::BuiltInAttr(_, id) => id,
-            MacroDefKind::BuiltInDerive(_, id) => id,
-            MacroDefKind::BuiltInEager(_, id) => id,
             MacroDefKind::ProcMacro(.., id) => return Either::Right(*id),
+            MacroDefKind::Declarative(id)
+            | MacroDefKind::BuiltIn(_, id)
+            | MacroDefKind::BuiltInAttr(_, id)
+            | MacroDefKind::BuiltInDerive(_, id)
+            | MacroDefKind::BuiltInEager(_, id) => id,
         };
         Either::Left(*id)
     }
@@ -464,15 +465,10 @@ impl InFile<SyntaxNode> {
 }
 
 impl<'a> InFile<&'a SyntaxNode> {
+    /// Falls back to the macro call range if the node cannot be mapped up fully.
     pub fn original_file_range(self, db: &dyn db::AstDatabase) -> FileRange {
-        if let Some(range) = original_range_opt(db, self) {
+        if let Some(res) = self.original_file_range_opt(db) {
-            let original_file = range.file_id.original_file(db);
+            return res;
-            if range.file_id == original_file.into() {
-                return FileRange { file_id: original_file, range: range.value };
-            }
-
-            log::error!("Fail to mapping up more for {:?}", range);
-            return FileRange { file_id: range.file_id.original_file(db), range: range.value };
         }
 
         // Fall back to whole macro call.
@@ -483,8 +479,27 @@ impl<'a> InFile<&'a SyntaxNode> {
 
         let orig_file = node.file_id.original_file(db);
         assert_eq!(node.file_id, orig_file.into());
+
         FileRange { file_id: orig_file, range: node.value.text_range() }
     }
+
+    /// Attempts to map the syntax node back up its macro calls.
+    pub fn original_file_range_opt(self, db: &dyn db::AstDatabase) -> Option<FileRange> {
+        match original_range_opt(db, self) {
+            Some(range) => {
+                let original_file = range.file_id.original_file(db);
+                if range.file_id != original_file.into() {
+                    log::error!("Failed mapping up more for {:?}", range);
+                }
+                Some(FileRange { file_id: original_file, range: range.value })
+            }
+            _ if !self.file_id.is_macro() => Some(FileRange {
+                file_id: self.file_id.original_file(db),
+                range: self.value.text_range(),
+            }),
+            _ => None,
+        }
+    }
 }
 
 fn original_range_opt(

crates/ide/src/display/navigation_target.rs

@@ -143,8 +143,11 @@ impl NavigationTarget {
         kind: SymbolKind,
     ) -> NavigationTarget {
         let name = node.value.name().map(|it| it.text().into()).unwrap_or_else(|| "_".into());
-        let focus_range =
+        let focus_range = node
-            node.value.name().map(|it| node.with_value(it.syntax()).original_file_range(db).range);
+            .value
+            .name()
+            .and_then(|it| node.with_value(it.syntax()).original_file_range_opt(db))
+            .map(|it| it.range);
         let frange = node.map(|it| it.syntax()).original_file_range(db);
 
         NavigationTarget::from_syntax(frange.file_id, name, focus_range, frange.range, kind)

crates/ide/src/rename.rs

@@ -1869,8 +1869,7 @@ fn f() { <()>::BAR$0; }"#,
     }
 
     #[test]
-    fn macros_are_broken_lol() {
+    fn defs_from_macros_arent_renamed() {
-        cov_mark::check!(macros_are_broken_lol);
         check(
             "lol",
             r#"
@@ -1878,11 +1877,7 @@ macro_rules! m { () => { fn f() {} } }
 m!();
 fn main() { f$0()  }
 "#,
-            r#"
+            "error: No identifier available to rename",
-macro_rules! m { () => { fn f() {} } }
-lol
-fn main() { lol()  }
-"#,
         )
     }
 }

crates/ide/src/runnables.rs

@@ -1446,7 +1446,6 @@ gen2!();
                                 0,
                             ),
                             full_range: 228..236,
-                            focus_range: 228..236,
                             name: "foo_test2",
                             kind: Function,
                         },
@@ -1467,7 +1466,6 @@ gen2!();
                                 0,
                             ),
                             full_range: 218..225,
-                            focus_range: 218..225,
                             name: "foo_test",
                             kind: Function,
                         },
@@ -1533,7 +1531,6 @@ foo!();
                                 0,
                             ),
                             full_range: 210..217,
-                            focus_range: 210..217,
                             name: "foo0",
                             kind: Function,
                         },
@@ -1554,7 +1551,6 @@ foo!();
                                 0,
                             ),
                             full_range: 210..217,
-                            focus_range: 210..217,
                             name: "foo1",
                             kind: Function,
                         },
@@ -1575,7 +1571,6 @@ foo!();
                                 0,
                             ),
                             full_range: 210..217,
-                            focus_range: 210..217,
                             name: "foo2",
                             kind: Function,
                         },

crates/ide_db/src/rename.rs

@@ -81,14 +81,6 @@ impl Definition {
     /// `Definition`. Note that some definitions, like buitin types, can't be
     /// renamed.
     pub fn range_for_rename(self, sema: &Semantics<RootDatabase>) -> Option<FileRange> {
-        // FIXME: the `original_file_range` calls here are wrong -- they never fail,
-        // and _fall back_ to the entirety of the macro call. Such fall back is
-        // incorrect for renames. The safe behavior would be to return an error for
-        // such cases. The correct behavior would be to return an auxiliary list of
-        // "can't rename these occurrences in macros" items, and then show some kind
-        // of a dialog to the user. See:
-        cov_mark::hit!(macros_are_broken_lol);
-
         let res = match self {
             Definition::Macro(mac) => {
                 let src = mac.source(sema.db)?;
@@ -96,38 +88,35 @@ impl Definition {
                     Either::Left(it) => it.name()?,
                     Either::Right(it) => it.name()?,
                 };
-                src.with_value(name.syntax()).original_file_range(sema.db)
+                src.with_value(name.syntax()).original_file_range_opt(sema.db)
             }
             Definition::Field(field) => {
                 let src = field.source(sema.db)?;
-
                 match &src.value {
                     FieldSource::Named(record_field) => {
                         let name = record_field.name()?;
-                        src.with_value(name.syntax()).original_file_range(sema.db)
+                        src.with_value(name.syntax()).original_file_range_opt(sema.db)
-                    }
-                    FieldSource::Pos(_) => {
-                        return None;
                     }
+                    FieldSource::Pos(_) => None,
                 }
             }
             Definition::ModuleDef(module_def) => match module_def {
                 hir::ModuleDef::Module(module) => {
                     let src = module.declaration_source(sema.db)?;
                     let name = src.value.name()?;
-                    src.with_value(name.syntax()).original_file_range(sema.db)
+                    src.with_value(name.syntax()).original_file_range_opt(sema.db)
                 }
-                hir::ModuleDef::Function(it) => name_range(it, sema)?,
+                hir::ModuleDef::Function(it) => name_range(it, sema),
                 hir::ModuleDef::Adt(adt) => match adt {
-                    hir::Adt::Struct(it) => name_range(it, sema)?,
+                    hir::Adt::Struct(it) => name_range(it, sema),
-                    hir::Adt::Union(it) => name_range(it, sema)?,
+                    hir::Adt::Union(it) => name_range(it, sema),
-                    hir::Adt::Enum(it) => name_range(it, sema)?,
+                    hir::Adt::Enum(it) => name_range(it, sema),
                 },
-                hir::ModuleDef::Variant(it) => name_range(it, sema)?,
+                hir::ModuleDef::Variant(it) => name_range(it, sema),
-                hir::ModuleDef::Const(it) => name_range(it, sema)?,
+                hir::ModuleDef::Const(it) => name_range(it, sema),
-                hir::ModuleDef::Static(it) => name_range(it, sema)?,
+                hir::ModuleDef::Static(it) => name_range(it, sema),
-                hir::ModuleDef::Trait(it) => name_range(it, sema)?,
+                hir::ModuleDef::Trait(it) => name_range(it, sema),
-                hir::ModuleDef::TypeAlias(it) => name_range(it, sema)?,
+                hir::ModuleDef::TypeAlias(it) => name_range(it, sema),
                 hir::ModuleDef::BuiltinType(_) => return None,
             },
             Definition::SelfType(_) => return None,
@@ -137,7 +126,7 @@ impl Definition {
                     Either::Left(bind_pat) => bind_pat.name()?,
                     Either::Right(_) => return None,
                 };
-                src.with_value(name.syntax()).original_file_range(sema.db)
+                src.with_value(name.syntax()).original_file_range_opt(sema.db)
             }
             Definition::GenericParam(generic_param) => match generic_param {
                 hir::GenericParam::TypeParam(type_param) => {
@@ -146,22 +135,22 @@ impl Definition {
                         Either::Left(type_param) => type_param.name()?,
                         Either::Right(_trait) => return None,
                     };
-                    src.with_value(name.syntax()).original_file_range(sema.db)
+                    src.with_value(name.syntax()).original_file_range_opt(sema.db)
                 }
                 hir::GenericParam::LifetimeParam(lifetime_param) => {
                     let src = lifetime_param.source(sema.db)?;
                     let lifetime = src.value.lifetime()?;
-                    src.with_value(lifetime.syntax()).original_file_range(sema.db)
+                    src.with_value(lifetime.syntax()).original_file_range_opt(sema.db)
                 }
-                hir::GenericParam::ConstParam(it) => name_range(it, sema)?,
+                hir::GenericParam::ConstParam(it) => name_range(it, sema),
             },
             Definition::Label(label) => {
                 let src = label.source(sema.db);
                 let lifetime = src.value.lifetime()?;
-                src.with_value(lifetime.syntax()).original_file_range(sema.db)
+                src.with_value(lifetime.syntax()).original_file_range_opt(sema.db)
             }
         };
-        return Some(res);
+        return res;
 
         fn name_range<D>(def: D, sema: &Semantics<RootDatabase>) -> Option<FileRange>
         where
@@ -170,8 +159,7 @@ impl Definition {
         {
             let src = def.source(sema.db)?;
             let name = src.value.name()?;
-            let res = src.with_value(name.syntax()).original_file_range(sema.db);
+            src.with_value(name.syntax()).original_file_range_opt(sema.db)
-            Some(res)
         }
     }
 }