Merge #6506

6506: Cleanup assists r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/assists/src/handlers/replace_derive_with_manual_impl.rs

@@ -16,24 +16,31 @@ use crate::{
     AssistId, AssistKind,
 };
 
-// Assist: add_custom_impl
+// Assist: replace_derive_with_manual_impl
 //
-// Adds impl block for derived trait.
+// Converts a `derive` impl into a manual one.
 //
 // ```
+// # trait Debug { fn fmt(&self, f: &mut Formatter) -> Result<()>; }
 // #[derive(Deb<|>ug, Display)]
 // struct S;
 // ```
 // ->
 // ```
+// # trait Debug { fn fmt(&self, f: &mut Formatter) -> Result<()>; }
 // #[derive(Display)]
 // struct S;
 //
 // impl Debug for S {
-//     $0
+//     fn fmt(&self, f: &mut Formatter) -> Result<()> {
+//         ${0:todo!()}
+//     }
 // }
 // ```
-pub(crate) fn add_custom_impl(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
+pub(crate) fn replace_derive_with_manual_impl(
+    acc: &mut Assists,
+    ctx: &AssistContext,
+) -> Option<()> {
     let attr = ctx.find_node_at_offset::<ast::Attr>()?;
 
     let attr_name = attr
@@ -90,43 +97,49 @@ fn add_assist(
 ) -> Option<()> {
     let target = attr.syntax().text_range();
     let input = attr.token_tree()?;
-    let label = format!("Add custom impl `{}` for `{}`", trait_path, annotated_name);
+    let label = format!("Convert to manual  `impl {} for {}`", trait_path, annotated_name);
     let trait_name = trait_path.segment().and_then(|seg| seg.name_ref())?;
 
-    acc.add(AssistId("add_custom_impl", AssistKind::Refactor), label, target, |builder| {
-        let impl_def_with_items =
-            impl_def_from_trait(&ctx.sema, annotated_name, trait_, trait_path);
-        update_attribute(builder, &input, &trait_name, &attr);
-        match (ctx.config.snippet_cap, impl_def_with_items) {
-            (None, _) => builder.insert(
-                insert_pos,
-                format!("\n\nimpl {} for {} {{\n\n}}", trait_path, annotated_name),
-            ),
-            (Some(cap), None) => builder.insert_snippet(
-                cap,
-                insert_pos,
-                format!("\n\nimpl {} for {} {{\n    $0\n}}", trait_path, annotated_name),
-            ),
-            (Some(cap), Some((impl_def, first_assoc_item))) => {
-                let mut cursor = Cursor::Before(first_assoc_item.syntax());
-                let placeholder;
-                if let ast::AssocItem::Fn(ref func) = first_assoc_item {
-                    if let Some(m) = func.syntax().descendants().find_map(ast::MacroCall::cast) {
-                        if m.syntax().text() == "todo!()" {
-                            placeholder = m;
-                            cursor = Cursor::Replace(placeholder.syntax());
-                        }
-                    }
-                }
-
-                builder.insert_snippet(
+    acc.add(
+        AssistId("replace_derive_with_manual_impl", AssistKind::Refactor),
+        label,
+        target,
+        |builder| {
+            let impl_def_with_items =
+                impl_def_from_trait(&ctx.sema, annotated_name, trait_, trait_path);
+            update_attribute(builder, &input, &trait_name, &attr);
+            match (ctx.config.snippet_cap, impl_def_with_items) {
+                (None, _) => builder.insert(
+                    insert_pos,
+                    format!("\n\nimpl {} for {} {{\n\n}}", trait_path, annotated_name),
+                ),
+                (Some(cap), None) => builder.insert_snippet(
                     cap,
                     insert_pos,
-                    format!("\n\n{}", render_snippet(cap, impl_def.syntax(), cursor)),
-                )
-            }
-        };
-    })
+                    format!("\n\nimpl {} for {} {{\n    $0\n}}", trait_path, annotated_name),
+                ),
+                (Some(cap), Some((impl_def, first_assoc_item))) => {
+                    let mut cursor = Cursor::Before(first_assoc_item.syntax());
+                    let placeholder;
+                    if let ast::AssocItem::Fn(ref func) = first_assoc_item {
+                        if let Some(m) = func.syntax().descendants().find_map(ast::MacroCall::cast)
+                        {
+                            if m.syntax().text() == "todo!()" {
+                                placeholder = m;
+                                cursor = Cursor::Replace(placeholder.syntax());
+                            }
+                        }
+                    }
+
+                    builder.insert_snippet(
+                        cap,
+                        insert_pos,
+                        format!("\n\n{}", render_snippet(cap, impl_def.syntax(), cursor)),
+                    )
+                }
+            };
+        },
+    )
 }
 
 fn impl_def_from_trait(
@@ -192,7 +205,7 @@ mod tests {
     #[test]
     fn add_custom_impl_debug() {
         check_assist(
-            add_custom_impl,
+            replace_derive_with_manual_impl,
             "
 mod fmt {
     pub struct Error;
@@ -233,7 +246,7 @@ impl fmt::Debug for Foo {
     #[test]
     fn add_custom_impl_all() {
         check_assist(
-            add_custom_impl,
+            replace_derive_with_manual_impl,
             "
 mod foo {
     pub trait Bar {
@@ -282,7 +295,7 @@ impl foo::Bar for Foo {
     #[test]
     fn add_custom_impl_for_unique_input() {
         check_assist(
-            add_custom_impl,
+            replace_derive_with_manual_impl,
             "
 #[derive(Debu<|>g)]
 struct Foo {
@@ -304,7 +317,7 @@ impl Debug for Foo {
     #[test]
     fn add_custom_impl_for_with_visibility_modifier() {
         check_assist(
-            add_custom_impl,
+            replace_derive_with_manual_impl,
             "
 #[derive(Debug<|>)]
 pub struct Foo {
@@ -326,7 +339,7 @@ impl Debug for Foo {
     #[test]
     fn add_custom_impl_when_multiple_inputs() {
         check_assist(
-            add_custom_impl,
+            replace_derive_with_manual_impl,
             "
 #[derive(Display, Debug<|>, Serialize)]
 struct Foo {}
@@ -345,7 +358,7 @@ impl Debug for Foo {
     #[test]
     fn test_ignore_derive_macro_without_input() {
         check_assist_not_applicable(
-            add_custom_impl,
+            replace_derive_with_manual_impl,
             "
 #[derive(<|>)]
 struct Foo {}
@@ -356,7 +369,7 @@ struct Foo {}
     #[test]
     fn test_ignore_if_cursor_on_param() {
         check_assist_not_applicable(
-            add_custom_impl,
+            replace_derive_with_manual_impl,
             "
 #[derive<|>(Debug)]
 struct Foo {}
@@ -364,7 +377,7 @@ struct Foo {}
         );
 
         check_assist_not_applicable(
-            add_custom_impl,
+            replace_derive_with_manual_impl,
             "
 #[derive(Debug)<|>]
 struct Foo {}
@@ -375,7 +388,7 @@ struct Foo {}
     #[test]
     fn test_ignore_if_not_derive() {
         check_assist_not_applicable(
-            add_custom_impl,
+            replace_derive_with_manual_impl,
             "
 #[allow(non_camel_<|>case_types)]
 struct Foo {}

crates/assists/src/lib.rs

@@ -120,13 +120,11 @@ mod handlers {
 
     pub(crate) type Handler = fn(&mut Assists, &AssistContext) -> Option<()>;
 
-    mod add_custom_impl;
     mod add_explicit_type;
     mod add_missing_impl_members;
     mod add_turbo_fish;
     mod apply_demorgan;
     mod auto_import;
-    mod change_return_type_to_result;
     mod change_visibility;
     mod convert_integer_literal;
     mod early_return;
@@ -157,6 +155,7 @@ mod handlers {
     mod remove_mut;
     mod remove_unused_param;
     mod reorder_fields;
+    mod replace_derive_with_manual_impl;
     mod replace_if_let_with_match;
     mod replace_impl_trait_with_generic;
     mod replace_let_with_if_let;
@@ -165,16 +164,15 @@ mod handlers {
     mod replace_unwrap_with_match;
     mod split_import;
     mod unwrap_block;
+    mod wrap_return_type_in_result;
 
     pub(crate) fn all() -> &'static [Handler] {
         &[
             // These are alphabetic for the foolish consistency
-            add_custom_impl::add_custom_impl,
             add_explicit_type::add_explicit_type,
             add_turbo_fish::add_turbo_fish,
             apply_demorgan::apply_demorgan,
             auto_import::auto_import,
-            change_return_type_to_result::change_return_type_to_result,
             change_visibility::change_visibility,
             convert_integer_literal::convert_integer_literal,
             early_return::convert_to_guarded_return,
@@ -208,6 +206,7 @@ mod handlers {
             remove_mut::remove_mut,
             remove_unused_param::remove_unused_param,
             reorder_fields::reorder_fields,
+            replace_derive_with_manual_impl::replace_derive_with_manual_impl,
             replace_if_let_with_match::replace_if_let_with_match,
             replace_impl_trait_with_generic::replace_impl_trait_with_generic,
             replace_let_with_if_let::replace_let_with_if_let,
@@ -215,6 +214,7 @@ mod handlers {
             replace_unwrap_with_match::replace_unwrap_with_match,
             split_import::split_import,
             unwrap_block::unwrap_block,
+            wrap_return_type_in_result::wrap_return_type_in_result,
             // These are manually sorted for better priorities
             add_missing_impl_members::add_missing_impl_members,
             add_missing_impl_members::add_missing_default_members,

crates/assists/src/tests/generated.rs

@@ -2,25 +2,6 @@
 
 use super::check_doc_test;
 
-#[test]
-fn doctest_add_custom_impl() {
-    check_doc_test(
-        "add_custom_impl",
-        r#####"
-#[derive(Deb<|>ug, Display)]
-struct S;
-"#####,
-        r#####"
-#[derive(Display)]
-struct S;
-
-impl Debug for S {
-    $0
-}
-"#####,
-    )
-}
-
 #[test]
 fn doctest_add_explicit_type() {
     check_doc_test(
@@ -177,19 +158,6 @@ pub mod std { pub mod collections { pub struct HashMap { } } }
     )
 }
 
-#[test]
-fn doctest_change_return_type_to_result() {
-    check_doc_test(
-        "change_return_type_to_result",
-        r#####"
-fn foo() -> i32<|> { 42i32 }
-"#####,
-        r#####"
-fn foo() -> Result<i32, ${0:_}> { Ok(42i32) }
-"#####,
-    )
-}
-
 #[test]
 fn doctest_change_visibility() {
     check_doc_test(
@@ -831,6 +799,29 @@ const test: Foo = Foo {foo: 1, bar: 0}
     )
 }
 
+#[test]
+fn doctest_replace_derive_with_manual_impl() {
+    check_doc_test(
+        "replace_derive_with_manual_impl",
+        r#####"
+trait Debug { fn fmt(&self, f: &mut Formatter) -> Result<()>; }
+#[derive(Deb<|>ug, Display)]
+struct S;
+"#####,
+        r#####"
+trait Debug { fn fmt(&self, f: &mut Formatter) -> Result<()>; }
+#[derive(Display)]
+struct S;
+
+impl Debug for S {
+    fn fmt(&self, f: &mut Formatter) -> Result<()> {
+        ${0:todo!()}
+    }
+}
+"#####,
+    )
+}
+
 #[test]
 fn doctest_replace_if_let_with_match() {
     check_doc_test(
@@ -985,3 +976,16 @@ fn foo() {
 "#####,
     )
 }
+
+#[test]
+fn doctest_wrap_return_type_in_result() {
+    check_doc_test(
+        "wrap_return_type_in_result",
+        r#####"
+fn foo() -> i32<|> { 42i32 }
+"#####,
+        r#####"
+fn foo() -> Result<i32, ${0:_}> { Ok(42i32) }
+"#####,
+    )
+}

xtask/tests/tidy.rs

@@ -214,9 +214,6 @@ fn check_todo(path: &Path, text: &str) {
         // This file itself obviously needs to use todo (<- like this!).
         "tests/cli.rs",
         // Some of our assists generate `todo!()`.
-        "tests/generated.rs",
-        "handlers/add_custom_impl.rs",
-        "handlers/add_missing_impl_members.rs",
         "handlers/add_turbo_fish.rs",
         "handlers/generate_function.rs",
         // To support generating `todo!()` in assists, we have `expr_todo()` in
@@ -229,6 +226,11 @@ fn check_todo(path: &Path, text: &str) {
         return;
     }
     if text.contains("TODO") || text.contains("TOOD") || text.contains("todo!") {
+        // Generated by an assist
+        if text.contains("${0:todo!()}") {
+            return;
+        }
+
         panic!(
             "\nTODO markers or todo! macros should not be committed to the master branch,\n\
              use FIXME instead\n\