Repository: rust-analyzer/rust-analyzer
Pull request #8975 (2021-05-24 19:02:47)

Use todo!() as placeholder body for generated match arms
`todo!()` seems to be a better fit for this than `{}`. Seeing that this assist predates stabilization of `todo` my guess is that simply no one bothered to change it yet.

Also fixed the issue where if the last arm was not block-like, rust-analyzer would not add a comma after it and would generate invalid code.

matklad: Using `{}` was intentional -- most of the time you want to just replace this code, and deleting `{}` is fewer keystrokes than deleting `{}`. No idea if that's good motivation though.

I guess, unless anyone objects, lets' merge this and see if people complain(/
jDomantas: My personal gripe with generating `{}` was that it usually gives you non-compiling code. My usual process is that I fill in the arm bodies one by one and I want to check if my code compiles when I finish an arm. To be able to do that I immediately replace `{}` with `todo!()` so that I would be able to have compiling code until I finish the whole match.

Patch:
diff --git a/crates/ide_assists/src/handlers/fill_match_arms.rs b/crates/ide_assists/src/handlers/fill_match_arms.rs
index 97435f0211..3d2cd739ae 100644
--- a/crates/ide_assists/src/handlers/fill_match_arms.rs
+++ b/crates/ide_assists/src/handlers/fill_match_arms.rs
@@ -31,8 +31,8 @@ use crate::{
 //
 // fn handle(action: Action) {
 //     match action {
-//         $0Action::Move { distance } => {}
-//         Action::Stop => {}
+//         $0Action::Move { distance } => todo!(),
+//         Action::Stop => todo!(),
 //     }
 // }
 // ```
@@ -129,7 +129,7 @@ pub(crate) fn fill_match_arms(acc: &mut Assists, ctx: &AssistContext) -> Option<
         |builder| {
             let new_match_arm_list = match_arm_list.clone_for_update();
             let missing_arms = missing_pats
-                .map(|pat| make::match_arm(iter::once(pat), make::expr_empty_block()))
+                .map(|pat| make::match_arm(iter::once(pat), make::ext::expr_todo()))
                 .map(|it| it.clone_for_update());
 
             let catch_all_arm = new_match_arm_list
@@ -350,8 +350,8 @@ fn foo(a: bool) {
             r#"
 fn foo(a: bool) {
     match a {
-        $0true => {}
-        false => {}
+        $0true => todo!(),
+        false => todo!(),
     }
 }
 "#,
@@ -373,7 +373,7 @@ fn foo(a: bool) {
 fn foo(a: bool) {
     match a {
         true => {}
-        $0false => {}
+        $0false => todo!(),
     }
 }
 "#,
@@ -410,10 +410,10 @@ fn foo(a: bool) {
             r#"
 fn foo(a: bool) {
     match (a, a) {
-        $0(true, true) => {}
-        (true, false) => {}
-        (false, true) => {}
-        (false, false) => {}
+        $0(true, true) => todo!(),
+        (true, false) => todo!(),
+        (false, true) => todo!(),
+        (false, false) => todo!(),
     }
 }
 "#,
@@ -435,9 +435,9 @@ fn foo(a: bool) {
 fn foo(a: bool) {
     match (a, a) {
         (false, true) => {}
-        $0(true, true) => {}
-        (true, false) => {}
-        (false, false) => {}
+        $0(true, true) => todo!(),
+        (true, false) => todo!(),
+        (false, false) => todo!(),
     }
 }
 "#,
@@ -471,7 +471,7 @@ fn main() {
     match A::As {
         A::Bs { x, y: Some(_) } => {}
         A::Cs(_, Some(_)) => {}
-        $0A::As => {}
+        $0A::As => todo!(),
     }
 }
 "#,
@@ -499,7 +499,7 @@ use Option::*;
 fn main() {
     match None {
         None => {}
-        Some(${0:_}) => {}
+        Some(${0:_}) => todo!(),
     }
 }
             "#,
@@ -523,7 +523,7 @@ enum A { As, Bs, Cs(Option<i32>) }
 fn main() {
     match A::As {
         A::Cs(_) | A::Bs => {}
-        $0A::As => {}
+        $0A::As => todo!(),
     }
 }
 "#,
@@ -553,8 +553,8 @@ fn main() {
         A::Bs if 0 < 1 => {}
         A::Ds(_value) => { let x = 1; }
         A::Es(B::Xs) => (),
-        $0A::As => {}
-        A::Cs => {}
+        $0A::As => todo!(),
+        A::Cs => todo!(),
     }
 }
 "#,
@@ -580,7 +580,7 @@ fn main() {
     match A::As {
         A::As(_) => {}
         a @ A::Bs(_) => {}
-        A::Cs(${0:_}) => {}
+        A::Cs(${0:_}) => todo!(),
     }
 }
 "#,
@@ -605,11 +605,11 @@ enum A { As, Bs, Cs(String), Ds(String, String), Es { x: usize, y: usize } }
 fn main() {
     let a = A::As;
     match a {
-        $0A::As => {}
-        A::Bs => {}
-        A::Cs(_) => {}
-        A::Ds(_, _) => {}
-        A::Es { x, y } => {}
+        $0A::As => todo!(),
+        A::Bs => todo!(),
+        A::Cs(_) => todo!(),
+        A::Ds(_, _) => todo!(),
+        A::Es { x, y } => todo!(),
     }
 }
 "#,
@@ -638,10 +638,10 @@ fn main() {
     let a = A::One;
     let b = B::One;
     match (a, b) {
-        $0(A::One, B::One) => {}
-        (A::One, B::Two) => {}
-        (A::Two, B::One) => {}
-        (A::Two, B::Two) => {}
+        $0(A::One, B::One) => todo!(),
+        (A::One, B::Two) => todo!(),
+        (A::Two, B::One) => todo!(),
+        (A::Two, B::Two) => todo!(),
     }
 }
 "#,
@@ -670,10 +670,10 @@ fn main() {
     let a = A::One;
     let b = B::One;
     match (&a, &b) {
-        $0(A::One, B::One) => {}
-        (A::One, B::Two) => {}
-        (A::Two, B::One) => {}
-        (A::Two, B::Two) => {}
+        $0(A::One, B::One) => todo!(),
+        (A::One, B::Two) => todo!(),
+        (A::Two, B::One) => todo!(),
+        (A::Two, B::Two) => todo!(),
     }
 }
 "#,
@@ -705,9 +705,9 @@ fn main() {
     let b = B::One;
     match (a, b) {
         (A::Two, B::One) => {}
-        $0(A::One, B::One) => {}
-        (A::One, B::Two) => {}
-        (A::Two, B::Two) => {}
+        $0(A::One, B::One) => todo!(),
+        (A::One, B::Two) => todo!(),
+        (A::Two, B::Two) => todo!(),
     }
 }
 "#,
@@ -736,7 +736,7 @@ fn main() {
     match (a, b) {
         (Some(_), _) => {}
         (None, Some(_)) => {}
-        $0(None, None) => {}
+        $0(None, None) => todo!(),
     }
 }
 "#,
@@ -801,8 +801,8 @@ enum A { One, Two }
 fn main() {
     let a = A::One;
     match (a, ) {
-        $0(A::One,) => {}
-        (A::Two,) => {}
+        $0(A::One,) => todo!(),
+        (A::Two,) => todo!(),
     }
 }
 "#,
@@ -826,7 +826,7 @@ enum A { As }
 
 fn foo(a: &A) {
     match a {
-        $0A::As => {}
+        $0A::As => todo!(),
     }
 }
 "#,
@@ -851,7 +851,7 @@ enum A {
 
 fn foo(a: &mut A) {
     match a {
-        $0A::Es { x, y } => {}
+        $0A::Es { x, y } => todo!(),
     }
 }
 "#,
@@ -891,8 +891,8 @@ enum E { X, Y }
 
 fn main() {
     match E::X {
-        $0E::X => {}
-        E::Y => {}
+        $0E::X => todo!(),
+        E::Y => todo!(),
     }
 }
 "#,
@@ -919,8 +919,8 @@ use foo::E::X;
 
 fn main() {
     match X {
-        $0X => {}
-        foo::E::Y => {}
+        $0X => todo!(),
+        foo::E::Y => todo!(),
     }
 }
 "#,
@@ -947,7 +947,7 @@ fn foo(a: A) {
     match a {
         // foo bar baz
         A::One => {}
-        $0A::Two => {}
+        $0A::Two => todo!(),
         // This is where the rest should be
     }
 }
@@ -971,8 +971,8 @@ fn foo(a: A) {
 enum A { One, Two }
 fn foo(a: A) {
     match a {
-        $0A::One => {}
-        A::Two => {}
+        $0A::One => todo!(),
+        A::Two => todo!(),
         // foo bar baz
     }
 }
@@ -996,8 +996,8 @@ fn foo(a: A) {
 enum A { One, Two, }
 fn foo(a: A) {
     match a {
-        $0A::One => {}
-        A::Two => {}
+        $0A::One => todo!(),
+        A::Two => todo!(),
     }
 }
 "#,
@@ -1021,8 +1021,8 @@ fn foo(opt: Option<i32>) {
             r#"
 fn foo(opt: Option<i32>) {
     match opt {
-        Some(${0:_}) => {}
-        None => {}
+        Some(${0:_}) => todo!(),
+        None => todo!(),
     }
 }
 "#,
@@ -1054,9 +1054,9 @@ enum Test {
 
 fn foo(t: Test) {
     m!(match t {
-    $0Test::A => {}
-    Test::B => {}
-    Test::C => {}
+    $0Test::A => todo!(),
+    Test::B => todo!(),
+    Test::C => todo!(),
 });
 }"#,
         );
@@ -1076,4 +1076,44 @@ fn foo(tuple: (A, A)) {
 "#,
         );
     }
+
+    #[test]
+    fn adds_comma_before_new_arms() {
+        check_assist(
+            fill_match_arms,
+            r#"
+fn foo(t: bool) {
+    match $0t {
+        true => 1 + 2
+    }
+}"#,
+            r#"
+fn foo(t: bool) {
+    match t {
+        true => 1 + 2,
+        $0false => todo!(),
+    }
+}"#,
+        );
+    }
+
+    #[test]
+    fn does_not_add_extra_comma() {
+        check_assist(
+            fill_match_arms,
+            r#"
+fn foo(t: bool) {
+    match $0t {
+        true => 1 + 2,
+    }
+}"#,
+            r#"
+fn foo(t: bool) {
+    match t {
+        true => 1 + 2,
+        $0false => todo!(),
+    }
+}"#,
+        );
+    }
 }
diff --git a/crates/project_model/src/cargo_workspace.rs b/crates/project_model/src/cargo_workspace.rs
index a8fee4f08d..ad705c7527 100644
--- a/crates/project_model/src/cargo_workspace.rs
+++ b/crates/project_model/src/cargo_workspace.rs
@@ -121,7 +121,7 @@ pub struct PackageDependency {
     pub kind: DepKind,
 }
 
-#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord)]
+#[derive(Debug, Clone, Eq, PartialEq)]
 pub enum DepKind {
     /// Available to the library, binary, and dev targets in the package (but not the build script).
     Normal,
@@ -132,23 +132,17 @@ pub enum DepKind {
 }
 
 impl DepKind {
-    fn iter(list: &[cargo_metadata::DepKindInfo]) -> impl Iterator<Item = Self> + '_ {
-        let mut dep_kinds = Vec::new();
-        if list.is_empty() {
-            dep_kinds.push(Self::Normal);
-        }
+    fn new(list: &[cargo_metadata::DepKindInfo]) -> Self {
         for info in list {
-            let kind = match info.kind {
-                cargo_metadata::DependencyKind::Normal => Self::Normal,
-                cargo_metadata::DependencyKind::Development => Self::Dev,
-                cargo_metadata::DependencyKind::Build => Self::Build,
+            match info.kind {
+                cargo_metadata::DependencyKind::Normal => return Self::Normal,
+                cargo_metadata::DependencyKind::Development => return Self::Dev,
+                cargo_metadata::DependencyKind::Build => return Self::Build,
                 cargo_metadata::DependencyKind::Unknown => continue,
-            };
-            dep_kinds.push(kind);
+            }
         }
-        dep_kinds.sort_unstable();
-        dep_kinds.dedup();
-        dep_kinds.into_iter()
+
+        Self::Normal
     }
 }
 
@@ -323,11 +317,7 @@ impl CargoWorkspace {
                 }
             };
             node.deps.sort_by(|a, b| a.pkg.cmp(&b.pkg));
-            for (dep_node, kind) in node
-                .deps
-                .iter()
-                .flat_map(|dep| DepKind::iter(&dep.dep_kinds).map(move |kind| (dep, kind)))
-            {
+            for dep_node in node.deps {
                 let pkg = match pkg_by_id.get(&dep_node.pkg) {
                     Some(&pkg) => pkg,
                     None => {
@@ -338,7 +328,11 @@ impl CargoWorkspace {
                         continue;
                     }
                 };
-                let dep = PackageDependency { name: dep_node.name.clone(), pkg, kind };
+                let dep = PackageDependency {
+                    name: dep_node.name,
+                    pkg,
+                    kind: DepKind::new(&dep_node.dep_kinds),
+                };
                 packages[source].dependencies.push(dep);
             }
             packages[source].active_features.extend(node.features);
diff --git a/crates/syntax/src/ast/edit_in_place.rs b/crates/syntax/src/ast/edit_in_place.rs
index ca8103668b..f7ee29d14c 100644
--- a/crates/syntax/src/ast/edit_in_place.rs
+++ b/crates/syntax/src/ast/edit_in_place.rs
@@ -356,13 +356,17 @@ impl ast::MatchArm {
 impl ast::MatchArmList {
     pub fn add_arm(&self, arm: ast::MatchArm) {
         normalize_ws_between_braces(self.syntax());
+        let mut elements = Vec::new();
         let position = match self.arms().last() {
             Some(last_arm) => {
-                let curly = last_arm
+                let comma = last_arm
                     .syntax()
                     .siblings_with_tokens(Direction::Next)
                     .find(|it| it.kind() == T![,]);
-                Position::after(curly.unwrap_or_else(|| last_arm.syntax().clone().into()))
+                if needs_comma(&last_arm) && comma.is_none() {
+                    elements.push(make::token(SyntaxKind::COMMA).into());
+                }
+                Position::after(comma.unwrap_or_else(|| last_arm.syntax().clone().into()))
             }
             None => match self.l_curly_token() {
                 Some(it) => Position::after(it),
@@ -370,11 +374,16 @@ impl ast::MatchArmList {
             },
         };
         let indent = IndentLevel::from_node(self.syntax()) + 1;
-        let elements = vec![
-            make::tokens::whitespace(&format!("\n{}", indent)).into(),
-            arm.syntax().clone().into(),
-        ];
+        elements.push(make::tokens::whitespace(&format!("\n{}", indent)).into());
+        elements.push(arm.syntax().clone().into());
+        if needs_comma(&arm) {
+            elements.push(make::token(SyntaxKind::COMMA).into());
+        }
         ted::insert_all(position, elements);
+
+        fn needs_comma(arm: &ast::MatchArm) -> bool {
+            arm.expr().map_or(false, |e| !e.is_block_like())
+        }
     }
 }
 
diff --git a/editors/code/src/snippets.ts b/editors/code/src/snippets.ts
index a409e52963..58f7aa128f 100644
--- a/editors/code/src/snippets.ts
+++ b/editors/code/src/snippets.ts
@@ -52,7 +52,7 @@ export async function applySnippetTextEdits(editor: vscode.TextEditor, edits: vs
             } else {
                 builder.replace(indel.range, indel.newText);
             }
-            lineDelta += countLines(indel.newText) - (indel.range.end.line - indel.range.start.line);
+            lineDelta = countLines(indel.newText) - (indel.range.end.line - indel.range.start.line);
         }
     });
     if (selections.length > 0) editor.selections = selections;
diff --git a/xtask/src/tidy.rs b/xtask/src/tidy.rs
index 82b33a7a09..6f687a7883 100644
--- a/xtask/src/tidy.rs
+++ b/xtask/src/tidy.rs
@@ -275,6 +275,7 @@ fn check_todo(path: &Path, text: &str) {
         // Some of our assists generate `todo!()`.
         "handlers/add_turbo_fish.rs",
         "handlers/generate_function.rs",
+        "handlers/fill_match_arms.rs",
         // To support generating `todo!()` in assists, we have `expr_todo()` in
         // `ast::make`.
         "ast/make.rs",


Test patch:
diff --git a/crates/ide_assists/src/tests/generated.rs b/crates/ide_assists/src/tests/generated.rs
index 8a9b0777c0..de5d9e55a1 100644
--- a/crates/ide_assists/src/tests/generated.rs
+++ b/crates/ide_assists/src/tests/generated.rs
@@ -455,8 +455,8 @@ enum Action { Move { distance: u32 }, Stop }
 
 fn handle(action: Action) {
     match action {
-        $0Action::Move { distance } => {}
-        Action::Stop => {}
+        $0Action::Move { distance } => todo!(),
+        Action::Stop => todo!(),
     }
 }
 "#####,