8 files changed, 226 insertions, 272 deletions

diff --git a/Cargo.lock b/Cargo.lock
index 19ee689cb..7f666ca61 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -35,9 +35,9 @@ dependencies = [
 
 [[package]]
 name = "anyhow"
-version = "1.0.38"
+version = "1.0.39"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "afddf7f520a80dbf76e6f50a35bca42a2331ef227a28b3b6dc5c2e2338d114b1"
+checksum = "81cddc5f91628367664cc7c69714ff08deee8a3efc54623011c772544d7b2767"
 
 [[package]]
 name = "anymap"
@@ -111,9 +111,9 @@ checksum = "14c189c53d098945499cdfa7ecc63567cf3886b3332b312a5b4585d8d3a6a610"
 
 [[package]]
 name = "camino"
-version = "1.0.2"
+version = "1.0.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "cd065703998b183ed0b348a22555691373a9345a1431141e5778b48bb17e4703"
+checksum = "d4648c6d00a709aa069a236adcaae4f605a6241c72bf5bee79331a4b625921a9"
 dependencies = [
  "serde",
 ]
@@ -794,9 +794,9 @@ checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646"
 
 [[package]]
 name = "libc"
-version = "0.2.88"
+version = "0.2.91"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "03b07a082330a35e43f63177cc01689da34fbffa0105e1246cf0311472cac73a"
+checksum = "8916b1f6ca17130ec6568feccee27c156ad12037880833a3b842a823236502e7"
 
 [[package]]
 name = "libloading"
@@ -946,9 +946,9 @@ dependencies = [
 
 [[package]]
 name = "mio"
-version = "0.7.9"
+version = "0.7.11"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a5dede4e2065b3842b8b0af444119f3aa331cc7cc2dd20388bfb0f5d5a38823a"
+checksum = "cf80d3e903b34e0bd7282b218398aec54e082c840d9baf8339e0080a0c542956"
 dependencies = [
  "libc",
  "log",
@@ -959,11 +959,10 @@ dependencies = [
 
 [[package]]
 name = "miow"
-version = "0.3.6"
+version = "0.3.7"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5a33c1b55807fbed163481b5ba66db4b2fa6cde694a5027be10fb724206c5897"
+checksum = "b9f1c5b025cda876f66ef43a113f91ebc9f4ccef34843000e0adf6ebbab84e21"
 dependencies = [
- "socket2",
  "winapi",
 ]
 
@@ -1299,9 +1298,9 @@ dependencies = [
 
 [[package]]
 name = "regex"
-version = "1.4.3"
+version = "1.4.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d9251239e129e16308e70d853559389de218ac275b515068abc96829d05b948a"
+checksum = "957056ecddbeba1b26965114e191d2e8589ce74db242b6ea25fc4062427a5c19"
 dependencies = [
  "regex-syntax",
 ]
@@ -1318,9 +1317,9 @@ dependencies = [
 
 [[package]]
 name = "regex-syntax"
-version = "0.6.22"
+version = "0.6.23"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b5eb417147ba9860a96cfe72a0b93bf88fee1744b5636ec99ab20c1aa9376581"
+checksum = "24d5f089152e60f62d28b835fbff2cd2e8dc0baf1ac13343bef92ab7eed84548"
 
 [[package]]
 name = "rowan"
@@ -1485,18 +1484,18 @@ dependencies = [
 
 [[package]]
 name = "serde"
-version = "1.0.124"
+version = "1.0.125"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "bd761ff957cb2a45fbb9ab3da6512de9de55872866160b23c25f1a841e99d29f"
+checksum = "558dc50e1a5a5fa7112ca2ce4effcb321b0300c0d4ccf0776a9f60cd89031171"
 dependencies = [
  "serde_derive",
 ]
 
 [[package]]
 name = "serde_derive"
-version = "1.0.124"
+version = "1.0.125"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1800f7693e94e186f5e25a28291ae1570da908aff7d97a095dec1e56ff99069b"
+checksum = "b093b7a2bb58203b5da3056c05b4ec1fed827dcfdb37347a8841695263b3d06d"
 dependencies = [
  "proc-macro2",
  "quote",
@@ -1566,17 +1565,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "dc725476a1398f0480d56cd0ad381f6f32acf2642704456f8f59a35df464b59a"
 
 [[package]]
-name = "socket2"
-version = "0.3.19"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "122e570113d28d773067fab24266b66753f6ea915758651696b6e35e49f88d6e"
-dependencies = [
- "cfg-if",
- "libc",
- "winapi",
-]
-
-[[package]]
 name = "stdx"
 version = "0.0.0"
 dependencies = [
@@ -1586,9 +1574,9 @@ dependencies = [
 
 [[package]]
 name = "syn"
-version = "1.0.63"
+version = "1.0.64"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "8fd9bc7ccc2688b3344c2f48b9b546648b25ce0b20fc717ee7fa7981a8ca9717"
+checksum = "3fd9d1e9976102a03c542daa2eff1b43f9d72306342f3f8b3ed5fb8908195d6f"
 dependencies = [
  "proc-macro2",
  "quote",
@@ -1718,9 +1706,9 @@ dependencies = [
 
 [[package]]
 name = "tracing-attributes"
-version = "0.1.13"
+version = "0.1.15"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a8a9bd1db7706f2373a190b0d067146caa39350c486f3d455b0e33b431f94c07"
+checksum = "c42e6fa53307c8a17e4ccd4dc81cf5ec38db9209f59b222210375b54ee40d1e2"
 dependencies = [
  "proc-macro2",
  "quote",
@@ -1759,9 +1747,9 @@ dependencies = [
 
 [[package]]
 name = "tracing-subscriber"
-version = "0.2.16"
+version = "0.2.17"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "8ab8966ac3ca27126141f7999361cc97dd6fb4b71da04c02044fa9045d98bb96"
+checksum = "705096c6f83bf68ea5d357a6aa01829ddbdac531b357b45abeca842938085baa"
 dependencies = [
  "ansi_term",
  "chrono",
@@ -1867,9 +1855,9 @@ dependencies = [
 
 [[package]]
 name = "version_check"
-version = "0.9.2"
+version = "0.9.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b5a972e5669d67ba988ce3dc826706fb0a8b01471c088cb0b6110b805cc36aed"
+checksum = "5fecdca9a5291cc2b8dcf7dc02453fee791a280f3743cb0905f8822ae463b3fe"
 
 [[package]]
 name = "vfs"
@@ -1896,9 +1884,9 @@ dependencies = [
 
 [[package]]
 name = "walkdir"
-version = "2.3.1"
+version = "2.3.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "777182bc735b6424e1a57516d35ed72cb8019d85c8c9bf536dccb3445c1a2f7d"
+checksum = "808cf2735cd4b6866113f648b791c6adc5714537bc222d9347bb203386ffda56"
 dependencies = [
  "same-file",
  "winapi",
diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs
index bdc1ad852..eb7865c84 100644
--- a/crates/hir/src/lib.rs
+++ b/crates/hir/src/lib.rs
@@ -973,6 +973,14 @@ impl SelfParam {
             Access::Owned => "self",
         }
     }
+
+    pub fn source(&self, db: &dyn HirDatabase) -> Option<InFile<ast::SelfParam>> {
+        let InFile { file_id, value } = Function::from(self.func).source(db)?;
+        value
+            .param_list()
+            .and_then(|params| params.self_param())
+            .map(|value| InFile { file_id, value })
+    }
 }
 
 impl HasVisibility for Function {
@@ -1348,6 +1356,13 @@ impl Local {
         }
     }
 
+    pub fn as_self_param(self, db: &dyn HirDatabase) -> Option<SelfParam> {
+        match self.parent {
+            DefWithBodyId::FunctionId(func) if self.is_self(db) => Some(SelfParam { func }),
+            _ => None,
+        }
+    }
+
     // FIXME: why is this an option? It shouldn't be?
     pub fn name(self, db: &dyn HirDatabase) -> Option<Name> {
         let body = db.body(self.parent);
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 b1ca6d50f..26d6dc9c9 100644
--- a/crates/ide/src/references/rename.rs
+++ b/crates/ide/src/references/rename.rs
@@ -219,40 +219,44 @@ fn rename_reference(
 ) -> RenameResult<SourceChange> {
     let ident_kind = check_identifier(new_name)?;
 
-    let def_is_lbl_or_lt = matches!(
-        def,
+    if matches!(
+        def, // is target a lifetime?
         Definition::GenericParam(hir::GenericParam::LifetimeParam(_)) | Definition::Label(_)
-    );
-    match (ident_kind, def) {
-        (IdentifierKind::ToSelf, _)
-        | (IdentifierKind::Underscore, _)
-        | (IdentifierKind::Ident, _)
-            if def_is_lbl_or_lt =>
-        {
-            cov_mark::hit!(rename_not_a_lifetime_ident_ref);
-            bail!("Invalid name `{}`: not a lifetime identifier", new_name)
-        }
-        (IdentifierKind::Lifetime, _) if def_is_lbl_or_lt => cov_mark::hit!(rename_lifetime),
-        (IdentifierKind::Lifetime, _) => {
-            cov_mark::hit!(rename_not_an_ident_ref);
-            bail!("Invalid name `{}`: not an identifier", new_name)
-        }
-        (IdentifierKind::ToSelf, Definition::Local(local)) if local.is_self(sema.db) => {
-            // no-op
-            cov_mark::hit!(rename_self_to_self);
-            return Ok(SourceChange::default());
-        }
-        (ident_kind, Definition::Local(local)) if local.is_self(sema.db) => {
-            cov_mark::hit!(rename_self_to_param);
-            return rename_self_to_param(sema, local, new_name, ident_kind);
-        }
-        (IdentifierKind::ToSelf, Definition::Local(local)) => {
-            cov_mark::hit!(rename_to_self);
-            return rename_to_self(sema, local);
+    ) {
+        match ident_kind {
+            IdentifierKind::Ident | IdentifierKind::ToSelf | IdentifierKind::Underscore => {
+                cov_mark::hit!(rename_not_a_lifetime_ident_ref);
+                bail!("Invalid name `{}`: not a lifetime identifier", new_name);
+            }
+            IdentifierKind::Lifetime => cov_mark::hit!(rename_lifetime),
         }
-        (IdentifierKind::ToSelf, _) => bail!("Invalid name `{}`: not an identifier", new_name),
-        (IdentifierKind::Ident, _) | (IdentifierKind::Underscore, _) => {
-            cov_mark::hit!(rename_ident)
+    } else {
+        match (ident_kind, def) {
+            (IdentifierKind::Lifetime, _) => {
+                cov_mark::hit!(rename_not_an_ident_ref);
+                bail!("Invalid name `{}`: not an identifier", new_name);
+            }
+            (IdentifierKind::ToSelf, Definition::Local(local)) => {
+                if local.is_self(sema.db) {
+                    // no-op
+                    cov_mark::hit!(rename_self_to_self);
+                    return Ok(SourceChange::default());
+                } else {
+                    cov_mark::hit!(rename_to_self);
+                    return rename_to_self(sema, local);
+                }
+            }
+            (ident_kind, Definition::Local(local)) => {
+                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, ident_kind);
+                } else {
+                    cov_mark::hit!(rename_local);
+                }
+            }
+            (IdentifierKind::ToSelf, _) => bail!("Invalid name `{}`: not an identifier", new_name),
+            (IdentifierKind::Ident, _) => cov_mark::hit!(rename_non_local),
+            (IdentifierKind::Underscore, _) => (),
         }
     }
 
@@ -336,16 +340,12 @@ fn rename_to_self(sema: &Semantics<RootDatabase>, local: hir::Local) -> RenameRe
 fn rename_self_to_param(
     sema: &Semantics<RootDatabase>,
     local: hir::Local,
+    self_param: hir::SelfParam,
     new_name: &str,
     identifier_kind: IdentifierKind,
 ) -> RenameResult<SourceChange> {
-    let (file_id, self_param) = match local.source(sema.db) {
-        InFile { file_id, value: Either::Right(self_param) } => (file_id, self_param),
-        _ => {
-            never!(true, "rename_self_to_param invoked on a non-self local");
-            bail!("rename_self_to_param invoked on a non-self local");
-        }
-    };
+    let InFile { file_id, value: self_param } =
+        self_param.source(sema.db).ok_or_else(|| format_err!("cannot find function source"))?;
 
     let def = Definition::Local(local);
     let usages = def.usages(sema).all();
@@ -701,7 +701,7 @@ foo!(Foo$0);",
 
     #[test]
     fn test_rename_for_local() {
-        cov_mark::check!(rename_ident);
+        cov_mark::check!(rename_local);
         check(
             "k",
             r#"
@@ -1242,6 +1242,7 @@ pub mod foo$0;
 
     #[test]
     fn test_enum_variant_from_module_1() {
+        cov_mark::check!(rename_non_local);
         check(
             "Baz",
             r#"
diff --git a/crates/ide_assists/src/handlers/add_missing_impl_members.rs b/crates/ide_assists/src/handlers/add_missing_impl_members.rs
index 63cea754d..0148635f9 100644
--- a/crates/ide_assists/src/handlers/add_missing_impl_members.rs
+++ b/crates/ide_assists/src/handlers/add_missing_impl_members.rs
@@ -3,9 +3,9 @@ use syntax::ast::{self, AstNode};
 
 use crate::{
     assist_context::{AssistContext, Assists},
-    utils::add_trait_assoc_items_to_impl,
-    utils::DefaultMethods,
-    utils::{filter_assoc_items, render_snippet, Cursor},
+    utils::{
+        add_trait_assoc_items_to_impl, filter_assoc_items, render_snippet, Cursor, DefaultMethods,
+    },
     AssistId, AssistKind,
 };
 
diff --git a/crates/ide_assists/src/handlers/convert_comment_block.rs b/crates/ide_assists/src/handlers/convert_comment_block.rs
index 9dc3ee28f..d202a85f9 100644
--- a/crates/ide_assists/src/handlers/convert_comment_block.rs
+++ b/crates/ide_assists/src/handlers/convert_comment_block.rs
@@ -1,13 +1,6 @@
 use itertools::Itertools;
 use syntax::{
-    ast::{
-        self,
-        edit::IndentLevel,
-        Comment, CommentKind,
-        CommentPlacement::{Inner, Outer},
-        CommentShape::{self, Block, Line},
-        Whitespace,
-    },
+    ast::{self, edit::IndentLevel, Comment, CommentKind, CommentShape, Whitespace},
     AstToken, Direction, SyntaxElement, TextRange,
 };
 
@@ -29,21 +22,18 @@ use crate::{AssistContext, AssistId, AssistKind, Assists};
 ///   */
 /// ```
 pub(crate) fn convert_comment_block(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
-    if let Some(comment) = ctx.find_token_at_offset::<ast::Comment>() {
-        // Only allow comments which are alone on their line
-        if let Some(prev) = comment.syntax().prev_token() {
-            if Whitespace::cast(prev).filter(|w| w.text().contains('\n')).is_none() {
-                return None;
-            }
+    let comment = ctx.find_token_at_offset::<ast::Comment>()?;
+    // Only allow comments which are alone on their line
+    if let Some(prev) = comment.syntax().prev_token() {
+        if Whitespace::cast(prev).filter(|w| w.text().contains('\n')).is_none() {
+            return None;
         }
-
-        return match comment.kind().shape {
-            ast::CommentShape::Block => block_to_line(acc, comment),
-            ast::CommentShape::Line => line_to_block(acc, comment),
-        };
     }
 
-    return None;
+    match comment.kind().shape {
+        ast::CommentShape::Block => block_to_line(acc, comment),
+        ast::CommentShape::Line => line_to_block(acc, comment),
+    }
 }
 
 fn block_to_line(acc: &mut Assists, comment: ast::Comment) -> Option<()> {
@@ -55,8 +45,7 @@ fn block_to_line(acc: &mut Assists, comment: ast::Comment) -> Option<()> {
         target,
         |edit| {
             let indentation = IndentLevel::from_token(comment.syntax());
-            let line_prefix =
-                comment_kind_prefix(CommentKind { shape: CommentShape::Line, ..comment.kind() });
+            let line_prefix = CommentKind { shape: CommentShape::Line, ..comment.kind() }.prefix();
 
             let text = comment.text();
             let text = &text[comment.prefix().len()..(text.len() - "*/".len())].trim();
@@ -105,7 +94,7 @@ fn line_to_block(acc: &mut Assists, comment: ast::Comment) -> Option<()> {
                 comments.into_iter().map(|c| line_comment_text(indentation, c)).join("\n");
 
             let block_prefix =
-                comment_kind_prefix(CommentKind { shape: CommentShape::Block, ..comment.kind() });
+                CommentKind { shape: CommentShape::Block, ..comment.kind() }.prefix();
 
             let output =
                 format!("{}\n{}\n{}*/", block_prefix, block_comment_body, indentation.to_string());
@@ -182,17 +171,6 @@ fn line_comment_text(indentation: IndentLevel, comm: ast::Comment) -> String {
     }
 }
 
-fn comment_kind_prefix(ck: ast::CommentKind) -> &'static str {
-    match (ck.shape, ck.doc) {
-        (Line, Some(Inner)) => "//!",
-        (Line, Some(Outer)) => "///",
-        (Line, None) => "//",
-        (Block, Some(Inner)) => "/*!",
-        (Block, Some(Outer)) => "/**",
-        (Block, None) => "/*",
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use crate::tests::{check_assist, check_assist_not_applicable};
diff --git a/crates/ide_assists/src/handlers/reorder_fields.rs b/crates/ide_assists/src/handlers/reorder_fields.rs
index 794c89323..383ca6c47 100644
--- a/crates/ide_assists/src/handlers/reorder_fields.rs
+++ b/crates/ide_assists/src/handlers/reorder_fields.rs
@@ -1,9 +1,6 @@
-use itertools::Itertools;
 use rustc_hash::FxHashMap;
 
-use hir::{Adt, ModuleDef, PathResolution, Semantics, Struct};
-use ide_db::RootDatabase;
-use syntax::{algo, ast, match_ast, AstNode, SyntaxKind, SyntaxKind::*, SyntaxNode};
+use syntax::{algo, ast, match_ast, AstNode, SyntaxKind::*, SyntaxNode};
 
 use crate::{AssistContext, AssistId, AssistKind, Assists};
 
@@ -23,26 +20,39 @@ use crate::{AssistContext, AssistId, AssistKind, Assists};
 // ```
 //
 pub(crate) fn reorder_fields(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
-    reorder::<ast::RecordExpr>(acc, ctx).or_else(|| reorder::<ast::RecordPat>(acc, ctx))
-}
+    let record = ctx
+        .find_node_at_offset::<ast::RecordExpr>()
+        .map(|it| it.syntax().clone())
+        .or_else(|| ctx.find_node_at_offset::<ast::RecordPat>().map(|it| it.syntax().clone()))?;
 
-fn reorder<R: AstNode>(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
-    let record = ctx.find_node_at_offset::<R>()?;
-    let path = record.syntax().children().find_map(ast::Path::cast)?;
+    let path = record.children().find_map(ast::Path::cast)?;
 
     let ranks = compute_fields_ranks(&path, &ctx)?;
 
-    let fields = get_fields(&record.syntax());
-    let sorted_fields = sorted_by_rank(&fields, |node| {
-        *ranks.get(&get_field_name(node)).unwrap_or(&usize::max_value())
-    });
+    let fields: Vec<SyntaxNode> = {
+        let field_kind = match record.kind() {
+            RECORD_EXPR => RECORD_EXPR_FIELD,
+            RECORD_PAT => RECORD_PAT_FIELD,
+            _ => {
+                stdx::never!();
+                return None;
+            }
+        };
+        record.children().flat_map(|n| n.children()).filter(|n| n.kind() == field_kind).collect()
+    };
+
+    let sorted_fields = {
+        let mut fields = fields.clone();
+        fields.sort_by_key(|node| *ranks.get(&get_field_name(node)).unwrap_or(&usize::max_value()));
+        fields
+    };
 
     if sorted_fields == fields {
         cov_mark::hit!(reorder_sorted_fields);
         return None;
     }
 
-    let target = record.syntax().text_range();
+    let target = record.text_range();
     acc.add(
         AssistId("reorder_fields", AssistKind::RefactorRewrite),
         "Reorder record fields",
@@ -57,14 +67,6 @@ fn reorder<R: AstNode>(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
     )
 }
 
-fn get_fields_kind(node: &SyntaxNode) -> Vec<SyntaxKind> {
-    match node.kind() {
-        RECORD_EXPR => vec![RECORD_EXPR_FIELD],
-        RECORD_PAT => vec![RECORD_PAT_FIELD, IDENT_PAT],
-        _ => vec![],
-    }
-}
-
 fn get_field_name(node: &SyntaxNode) -> String {
     let res = match_ast! {
         match node {
@@ -76,34 +78,20 @@ fn get_field_name(node: &SyntaxNode) -> String {
     res.unwrap_or_default()
 }
 
-fn get_fields(record: &SyntaxNode) -> Vec<SyntaxNode> {
-    let kinds = get_fields_kind(record);
-    record.children().flat_map(|n| n.children()).filter(|n| kinds.contains(&n.kind())).collect()
-}
-
-fn sorted_by_rank(
-    fields: &[SyntaxNode],
-    get_rank: impl Fn(&SyntaxNode) -> usize,
-) -> Vec<SyntaxNode> {
-    fields.iter().cloned().sorted_by_key(get_rank).collect()
-}
+fn compute_fields_ranks(path: &ast::Path, ctx: &AssistContext) -> Option<FxHashMap<String, usize>> {
+    let strukt = match ctx.sema.resolve_path(path) {
+        Some(hir::PathResolution::Def(hir::ModuleDef::Adt(hir::Adt::Struct(it)))) => it,
+        _ => return None,
+    };
 
-fn struct_definition(path: &ast::Path, sema: &Semantics<RootDatabase>) -> Option<Struct> {
-    match sema.resolve_path(path) {
-        Some(PathResolution::Def(ModuleDef::Adt(Adt::Struct(s)))) => Some(s),
-        _ => None,
-    }
-}
+    let res = strukt
+        .fields(ctx.db())
+        .iter()
+        .enumerate()
+        .map(|(idx, field)| (field.name(ctx.db()).to_string(), idx))
+        .collect();
 
-fn compute_fields_ranks(path: &ast::Path, ctx: &AssistContext) -> Option<FxHashMap<String, usize>> {
-    Some(
-        struct_definition(path, &ctx.sema)?
-            .fields(ctx.db())
-            .iter()
-            .enumerate()
-            .map(|(idx, field)| (field.name(ctx.db()).to_string(), idx))
-            .collect(),
-    )
+    Some(res)
 }
 
 #[cfg(test)]
@@ -118,11 +106,7 @@ mod tests {
         check_assist_not_applicable(
             reorder_fields,
             r#"
-struct Foo {
-    foo: i32,
-    bar: i32,
-}
-
+struct Foo { foo: i32, bar: i32 }
 const test: Foo = $0Foo { foo: 0, bar: 0 };
 "#,
         )
@@ -133,8 +117,8 @@ const test: Foo = $0Foo { foo: 0, bar: 0 };
         check_assist_not_applicable(
             reorder_fields,
             r#"
-struct Foo {};
-const test: Foo = $0Foo {}
+struct Foo {}
+const test: Foo = $0Foo {};
 "#,
         )
     }
@@ -144,12 +128,12 @@ const test: Foo = $0Foo {}
         check_assist(
             reorder_fields,
             r#"
-struct Foo {foo: i32, bar: i32};
-const test: Foo = $0Foo {bar: 0, foo: 1}
+struct Foo { foo: i32, bar: i32 }
+const test: Foo = $0Foo { bar: 0, foo: 1 };
 "#,
             r#"
-struct Foo {foo: i32, bar: i32};
-const test: Foo = Foo {foo: 1, bar: 0}
+struct Foo { foo: i32, bar: i32 }
+const test: Foo = Foo { foo: 1, bar: 0 };
 "#,
         )
     }
@@ -186,10 +170,7 @@ fn f(f: Foo) -> {
         check_assist(
             reorder_fields,
             r#"
-struct Foo {
-    foo: String,
-    bar: String,
-}
+struct Foo { foo: String, bar: String }
 
 impl Foo {
     fn new() -> Foo {
@@ -203,10 +184,7 @@ impl Foo {
 }
 "#,
             r#"
-struct Foo {
-    foo: String,
-    bar: String,
-}
+struct Foo { foo: String, bar: String }
 
 impl Foo {
     fn new() -> Foo {
diff --git a/crates/syntax/src/ast/token_ext.rs b/crates/syntax/src/ast/token_ext.rs
index 090282d28..29d25a58a 100644
--- a/crates/syntax/src/ast/token_ext.rs
+++ b/crates/syntax/src/ast/token_ext.rs
@@ -102,8 +102,9 @@ impl CommentKind {
         kind
     }
 
-    fn prefix(&self) -> &'static str {
-        let &(prefix, _) = CommentKind::BY_PREFIX.iter().find(|(_, kind)| kind == self).unwrap();
+    pub fn prefix(&self) -> &'static str {
+        let &(prefix, _) =
+            CommentKind::BY_PREFIX.iter().rev().find(|(_, kind)| kind == self).unwrap();
         prefix
     }
 }