3 files changed, 127 insertions, 130 deletions

diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs
index 5da6a0340..bdc1ad852 100644
--- a/crates/hir/src/lib.rs
+++ b/crates/hir/src/lib.rs
@@ -852,6 +852,7 @@ impl Function {
             })
             .collect()
     }
+
     pub fn method_params(self, db: &dyn HirDatabase) -> Option<Vec<Param>> {
         if self.self_param(db).is_none() {
             return None;
@@ -909,7 +910,7 @@ impl From<hir_ty::Mutability> for Access {
     }
 }
 
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub struct Param {
     func: Function,
     /// The index in parameter list, including self parameter.
@@ -922,13 +923,25 @@ impl Param {
         &self.ty
     }
 
+    pub fn as_local(&self, db: &dyn HirDatabase) -> Local {
+        let parent = DefWithBodyId::FunctionId(self.func.into());
+        let body = db.body(parent);
+        Local { parent, pat_id: body.params[self.idx] }
+    }
+
     pub fn pattern_source(&self, db: &dyn HirDatabase) -> Option<ast::Pat> {
-        let params = self.func.source(db)?.value.param_list()?;
+        self.source(db).and_then(|p| p.value.pat())
+    }
+
+    pub fn source(&self, db: &dyn HirDatabase) -> Option<InFile<ast::Param>> {
+        let InFile { file_id, value } = self.func.source(db)?;
+        let params = value.param_list()?;
         if params.self_param().is_some() {
-            params.params().nth(self.idx.checked_sub(1)?)?.pat()
+            params.params().nth(self.idx.checked_sub(1)?)
         } else {
-            params.params().nth(self.idx)?.pat()
+            params.params().nth(self.idx)
         }
+        .map(|value| InFile { file_id, value })
     }
 }
 
diff --git a/crates/hir_def/src/nameres/collector.rs b/crates/hir_def/src/nameres/collector.rs
index 28b73c3a1..d8fabe49b 100644
--- a/crates/hir_def/src/nameres/collector.rs
+++ b/crates/hir_def/src/nameres/collector.rs
@@ -91,7 +91,6 @@ pub(super) fn collect_defs(
         resolved_imports: Vec::new(),
 
         unexpanded_macros: Vec::new(),
-        unexpanded_attribute_macros: Vec::new(),
         mod_dirs: FxHashMap::default(),
         cfg_options,
         proc_macros,
@@ -202,15 +201,14 @@ struct ImportDirective {
 #[derive(Clone, Debug, Eq, PartialEq)]
 struct MacroDirective {
     module_id: LocalModuleId,
-    ast_id: AstIdWithPath<ast::MacroCall>,
-    legacy: Option<MacroCallId>,
     depth: usize,
+    kind: MacroDirectiveKind,
 }
 
 #[derive(Clone, Debug, Eq, PartialEq)]
-struct DeriveDirective {
-    module_id: LocalModuleId,
-    ast_id: AstIdWithPath<ast::Item>,
+enum MacroDirectiveKind {
+    FnLike { ast_id: AstIdWithPath<ast::MacroCall>, legacy: Option<MacroCallId> },
+    Derive { ast_id: AstIdWithPath<ast::Item> },
 }
 
 struct DefData<'a> {
@@ -228,7 +226,6 @@ struct DefCollector<'a> {
     unresolved_imports: Vec<ImportDirective>,
     resolved_imports: Vec<ImportDirective>,
     unexpanded_macros: Vec<MacroDirective>,
-    unexpanded_attribute_macros: Vec<DeriveDirective>,
     mod_dirs: FxHashMap<LocalModuleId, ModDir>,
     cfg_options: &'a CfgOptions,
     /// List of procedural macros defined by this crate. This is read from the dynamic library
@@ -782,60 +779,58 @@ impl DefCollector<'_> {
 
     fn resolve_macros(&mut self) -> ReachedFixedPoint {
         let mut macros = std::mem::replace(&mut self.unexpanded_macros, Vec::new());
-        let mut attribute_macros =
-            std::mem::replace(&mut self.unexpanded_attribute_macros, Vec::new());
         let mut resolved = Vec::new();
         let mut res = ReachedFixedPoint::Yes;
         macros.retain(|directive| {
-            if let Some(call_id) = directive.legacy {
-                res = ReachedFixedPoint::No;
-                resolved.push((directive.module_id, call_id, directive.depth));
-                return false;
-            }
+            match &directive.kind {
+                MacroDirectiveKind::FnLike { ast_id, legacy } => {
+                    if let Some(call_id) = legacy {
+                        res = ReachedFixedPoint::No;
+                        resolved.push((directive.module_id, *call_id, directive.depth));
+                        return false;
+                    }
 
-            match macro_call_as_call_id(
-                &directive.ast_id,
-                self.db,
-                self.def_map.krate,
-                |path| {
-                    let resolved_res = self.def_map.resolve_path_fp_with_macro(
+                    match macro_call_as_call_id(
+                        ast_id,
                         self.db,
-                        ResolveMode::Other,
-                        directive.module_id,
-                        &path,
-                        BuiltinShadowMode::Module,
-                    );
-                    resolved_res.resolved_def.take_macros()
-                },
-                &mut |_err| (),
-            ) {
-                Ok(Ok(call_id)) => {
-                    resolved.push((directive.module_id, call_id, directive.depth));
-                    res = ReachedFixedPoint::No;
-                    return false;
+                        self.def_map.krate,
+                        |path| {
+                            let resolved_res = self.def_map.resolve_path_fp_with_macro(
+                                self.db,
+                                ResolveMode::Other,
+                                directive.module_id,
+                                &path,
+                                BuiltinShadowMode::Module,
+                            );
+                            resolved_res.resolved_def.take_macros()
+                        },
+                        &mut |_err| (),
+                    ) {
+                        Ok(Ok(call_id)) => {
+                            resolved.push((directive.module_id, call_id, directive.depth));
+                            res = ReachedFixedPoint::No;
+                            return false;
+                        }
+                        Err(UnresolvedMacro) | Ok(Err(_)) => {}
+                    }
                 }
-                Err(UnresolvedMacro) | Ok(Err(_)) => {}
-            }
-
-            true
-        });
-        attribute_macros.retain(|directive| {
-            match derive_macro_as_call_id(&directive.ast_id, self.db, self.def_map.krate, |path| {
-                self.resolve_derive_macro(&directive, &path)
-            }) {
-                Ok(call_id) => {
-                    resolved.push((directive.module_id, call_id, 0));
-                    res = ReachedFixedPoint::No;
-                    return false;
+                MacroDirectiveKind::Derive { ast_id } => {
+                    match derive_macro_as_call_id(ast_id, self.db, self.def_map.krate, |path| {
+                        self.resolve_derive_macro(directive.module_id, &path)
+                    }) {
+                        Ok(call_id) => {
+                            resolved.push((directive.module_id, call_id, 0));
+                            res = ReachedFixedPoint::No;
+                            return false;
+                        }
+                        Err(UnresolvedMacro) => (),
+                    }
                 }
-                Err(UnresolvedMacro) => (),
             }
 
             true
         });
-
         self.unexpanded_macros = macros;
-        self.unexpanded_attribute_macros = attribute_macros;
 
         for (module_id, macro_call_id, depth) in resolved {
             self.collect_macro_expansion(module_id, macro_call_id, depth);
@@ -844,15 +839,11 @@ impl DefCollector<'_> {
         res
     }
 
-    fn resolve_derive_macro(
-        &self,
-        directive: &DeriveDirective,
-        path: &ModPath,
-    ) -> Option<MacroDefId> {
+    fn resolve_derive_macro(&self, module: LocalModuleId, path: &ModPath) -> Option<MacroDefId> {
         let resolved_res = self.def_map.resolve_path_fp_with_macro(
             self.db,
             ResolveMode::Other,
-            directive.module_id,
+            module,
             &path,
             BuiltinShadowMode::Module,
         );
@@ -912,33 +903,35 @@ impl DefCollector<'_> {
         // Emit diagnostics for all remaining unexpanded macros.
 
         for directive in &self.unexpanded_macros {
-            let mut error = None;
-            match macro_call_as_call_id(
-                &directive.ast_id,
-                self.db,
-                self.def_map.krate,
-                |path| {
-                    let resolved_res = self.def_map.resolve_path_fp_with_macro(
-                        self.db,
-                        ResolveMode::Other,
-                        directive.module_id,
-                        &path,
-                        BuiltinShadowMode::Module,
-                    );
-                    resolved_res.resolved_def.take_macros()
-                },
-                &mut |e| {
-                    error.get_or_insert(e);
+            match &directive.kind {
+                MacroDirectiveKind::FnLike { ast_id, .. } => match macro_call_as_call_id(
+                    ast_id,
+                    self.db,
+                    self.def_map.krate,
+                    |path| {
+                        let resolved_res = self.def_map.resolve_path_fp_with_macro(
+                            self.db,
+                            ResolveMode::Other,
+                            directive.module_id,
+                            &path,
+                            BuiltinShadowMode::Module,
+                        );
+                        resolved_res.resolved_def.take_macros()
+                    },
+                    &mut |_| (),
+                ) {
+                    Ok(_) => (),
+                    Err(UnresolvedMacro) => {
+                        self.def_map.diagnostics.push(DefDiagnostic::unresolved_macro_call(
+                            directive.module_id,
+                            ast_id.ast_id,
+                        ));
+                    }
                 },
-            ) {
-                Ok(_) => (),
-                Err(UnresolvedMacro) => {
-                    self.def_map.diagnostics.push(DefDiagnostic::unresolved_macro_call(
-                        directive.module_id,
-                        directive.ast_id.ast_id,
-                    ));
+                MacroDirectiveKind::Derive { .. } => {
+                    // FIXME: we might want to diagnose this too
                 }
-            };
+            }
         }
 
         // Emit diagnostics for all remaining unresolved imports.
@@ -1380,9 +1373,11 @@ impl ModCollector<'_, '_> {
                 Some(derive_macros) => {
                     for path in derive_macros {
                         let ast_id = AstIdWithPath::new(self.file_id, ast_id, path);
-                        self.def_collector
-                            .unexpanded_attribute_macros
-                            .push(DeriveDirective { module_id: self.module_id, ast_id });
+                        self.def_collector.unexpanded_macros.push(MacroDirective {
+                            module_id: self.module_id,
+                            depth: self.macro_depth + 1,
+                            kind: MacroDirectiveKind::Derive { ast_id },
+                        });
                     }
                 }
                 None => {
@@ -1497,9 +1492,8 @@ impl ModCollector<'_, '_> {
 
         self.def_collector.unexpanded_macros.push(MacroDirective {
             module_id: self.module_id,
-            ast_id,
-            legacy: None,
             depth: self.macro_depth + 1,
+            kind: MacroDirectiveKind::FnLike { ast_id, legacy: None },
         });
     }
 
@@ -1542,7 +1536,6 @@ mod tests {
             unresolved_imports: Vec::new(),
             resolved_imports: Vec::new(),
             unexpanded_macros: Vec::new(),
-            unexpanded_attribute_macros: Vec::new(),
             mod_dirs: FxHashMap::default(),
             cfg_options: &CfgOptions::default(),
             proc_macros: Default::default(),
diff --git a/crates/ide/src/references/rename.rs b/crates/ide/src/references/rename.rs
index 5340b638a..b1ca6d50f 100644
--- a/crates/ide/src/references/rename.rs
+++ b/crates/ide/src/references/rename.rs
@@ -1,8 +1,11 @@
-//! FIXME: write short doc here
+//! Renaming functionality
+//!
+//! All reference and file rename requests go through here where the corresponding [`SourceChange`]s
+//! will be calculated.
 use std::fmt::{self, Display};
 
 use either::Either;
-use hir::{HasSource, InFile, Module, ModuleDef, ModuleSource, Semantics};
+use hir::{AsAssocItem, InFile, Module, ModuleDef, ModuleSource, Semantics};
 use ide_db::{
     base_db::{AnchoredPathBuf, FileId},
     defs::{Definition, NameClass, NameRefClass},
@@ -196,7 +199,7 @@ fn rename_mod(
                 file_id,
                 TextEdit::replace(name.syntax().text_range(), new_name.to_string()),
             ),
-            _ => unreachable!(),
+            _ => never!("Module source node is missing a name"),
         }
     }
     let def = Definition::ModuleDef(ModuleDef::Module(module));
@@ -275,46 +278,32 @@ fn rename_to_self(sema: &Semantics<RootDatabase>, local: hir::Local) -> RenameRe
 
     let fn_def = match local.parent(sema.db) {
         hir::DefWithBody::Function(func) => func,
-        _ => bail!("Cannot rename non-param local to self"),
+        _ => bail!("Cannot rename local to self outside of function"),
     };
 
-    // FIXME: reimplement this on the hir instead
-    // as of the time of this writing params in hir don't keep their names
-    let fn_ast = fn_def
-        .source(sema.db)
-        .ok_or_else(|| format_err!("Cannot rename non-param local to self"))?
-        .value;
-
-    let first_param_range = fn_ast
-        .param_list()
-        .and_then(|p| p.params().next())
-        .ok_or_else(|| format_err!("Method has no parameters"))?
-        .syntax()
-        .text_range();
-    let InFile { file_id, value: local_source } = local.source(sema.db);
-    match local_source {
-        either::Either::Left(pat)
-            if !first_param_range.contains_range(pat.syntax().text_range()) =>
-        {
-            bail!("Only the first parameter can be self");
-        }
-        _ => (),
-    }
-
-    let impl_block = fn_ast
-        .syntax()
-        .ancestors()
-        .find_map(|node| ast::Impl::cast(node))
-        .and_then(|def| sema.to_def(&def))
-        .ok_or_else(|| format_err!("No impl block found for function"))?;
-    if fn_def.self_param(sema.db).is_some() {
+    if let Some(_) = fn_def.self_param(sema.db) {
         bail!("Method already has a self parameter");
     }
 
     let params = fn_def.assoc_fn_params(sema.db);
-    let first_param = params.first().ok_or_else(|| format_err!("Method has no parameters"))?;
+    let first_param = params
+        .first()
+        .ok_or_else(|| format_err!("Cannot rename local to self unless it is a parameter"))?;
+    if first_param.as_local(sema.db) != local {
+        bail!("Only the first parameter may be renamed to self");
+    }
+
+    let assoc_item = fn_def
+        .as_assoc_item(sema.db)
+        .ok_or_else(|| format_err!("Cannot rename parameter to self for free function"))?;
+    let impl_ = match assoc_item.container(sema.db) {
+        hir::AssocItemContainer::Trait(_) => {
+            bail!("Cannot rename parameter to self for trait functions");
+        }
+        hir::AssocItemContainer::Impl(impl_) => impl_,
+    };
     let first_param_ty = first_param.ty();
-    let impl_ty = impl_block.target_ty(sema.db);
+    let impl_ty = impl_.target_ty(sema.db);
     let (ty, self_param) = if impl_ty.remove_ref().is_some() {
         // if the impl is a ref to the type we can just match the `&T` with self directly
         (first_param_ty.clone(), "self")
@@ -328,6 +317,9 @@ fn rename_to_self(sema: &Semantics<RootDatabase>, local: hir::Local) -> RenameRe
         bail!("Parameter type differs from impl block type");
     }
 
+    let InFile { file_id, value: param_source } =
+        first_param.source(sema.db).ok_or_else(|| format_err!("No source for parameter found"))?;
+
     let def = Definition::Local(local);
     let usages = def.usages(sema).all();
     let mut source_change = SourceChange::default();
@@ -336,9 +328,8 @@ fn rename_to_self(sema: &Semantics<RootDatabase>, local: hir::Local) -> RenameRe
     }));
     source_change.insert_source_edit(
         file_id.original_file(sema.db),
-        TextEdit::replace(first_param_range, String::from(self_param)),
+        TextEdit::replace(param_source.syntax().text_range(), String::from(self_param)),
     );
-
     Ok(source_change)
 }
 
@@ -1361,7 +1352,7 @@ fn f(foo$0: &mut Foo) -> i32 {
     foo.i
 }
 "#,
-            "error: No impl block found for function",
+            "error: Cannot rename parameter to self for free function",
         );
         check(
             "self",
@@ -1391,7 +1382,7 @@ impl Foo {
     }
 }
 "#,
-            "error: Only the first parameter can be self",
+            "error: Only the first parameter may be renamed to self",
         );
     }