4 files changed, 207 insertions, 46 deletions

diff --git a/crates/ide_assists/src/handlers/extract_function.rs b/crates/ide_assists/src/handlers/extract_function.rs
index 719f22053..059414274 100644
--- a/crates/ide_assists/src/handlers/extract_function.rs
+++ b/crates/ide_assists/src/handlers/extract_function.rs
@@ -75,7 +75,8 @@ pub(crate) fn extract_function(acc: &mut Assists, ctx: &AssistContext) -> Option
     let insert_after = scope_for_fn_insertion(&body, anchor)?;
     let module = ctx.sema.scope(&insert_after).module()?;
 
-    let vars_defined_in_body_and_outlive = vars_defined_in_body_and_outlive(ctx, &body);
+    let vars_defined_in_body_and_outlive =
+        vars_defined_in_body_and_outlive(ctx, &body, &node.parent().as_ref().unwrap_or(&node));
     let ret_ty = body_return_ty(ctx, &body)?;
 
     // FIXME: we compute variables that outlive here just to check `never!` condition
@@ -257,7 +258,7 @@ struct Function {
     control_flow: ControlFlow,
     ret_ty: RetType,
     body: FunctionBody,
-    vars_defined_in_body_and_outlive: Vec<Local>,
+    vars_defined_in_body_and_outlive: Vec<OutlivedLocal>,
 }
 
 #[derive(Debug)]
@@ -296,9 +297,9 @@ impl Function {
             RetType::Expr(ty) => FunType::Single(ty.clone()),
             RetType::Stmt => match self.vars_defined_in_body_and_outlive.as_slice() {
                 [] => FunType::Unit,
-                [var] => FunType::Single(var.ty(ctx.db())),
+                [var] => FunType::Single(var.local.ty(ctx.db())),
                 vars => {
-                    let types = vars.iter().map(|v| v.ty(ctx.db())).collect();
+                    let types = vars.iter().map(|v| v.local.ty(ctx.db())).collect();
                     FunType::Tuple(types)
                 }
             },
@@ -562,6 +563,12 @@ impl HasTokenAtOffset for FunctionBody {
     }
 }
 
+#[derive(Debug)]
+struct OutlivedLocal {
+    local: Local,
+    mut_usage_outside_body: bool,
+}
+
 /// Try to guess what user wants to extract
 ///
 /// We have basically have two cases:
@@ -707,10 +714,10 @@ fn has_exclusive_usages(ctx: &AssistContext, usages: &LocalUsages, body: &Functi
         .any(|reference| reference_is_exclusive(reference, body, ctx))
 }
 
-/// checks if this reference requires `&mut` access inside body
+/// checks if this reference requires `&mut` access inside node
 fn reference_is_exclusive(
     reference: &FileReference,
-    body: &FunctionBody,
+    node: &dyn HasTokenAtOffset,
     ctx: &AssistContext,
 ) -> bool {
     // we directly modify variable with set: `n = 0`, `n += 1`
@@ -719,7 +726,7 @@ fn reference_is_exclusive(
     }
 
     // we take `&mut` reference to variable: `&mut v`
-    let path = match path_element_of_reference(body, reference) {
+    let path = match path_element_of_reference(node, reference) {
         Some(path) => path,
         None => return false,
     };
@@ -832,10 +839,16 @@ fn vars_defined_in_body(body: &FunctionBody, ctx: &AssistContext) -> Vec<Local>
 }
 
 /// list local variables defined inside `body` that should be returned from extracted function
-fn vars_defined_in_body_and_outlive(ctx: &AssistContext, body: &FunctionBody) -> Vec<Local> {
-    let mut vars_defined_in_body = vars_defined_in_body(&body, ctx);
-    vars_defined_in_body.retain(|var| var_outlives_body(ctx, body, var));
+fn vars_defined_in_body_and_outlive(
+    ctx: &AssistContext,
+    body: &FunctionBody,
+    parent: &SyntaxNode,
+) -> Vec<OutlivedLocal> {
+    let vars_defined_in_body = vars_defined_in_body(&body, ctx);
     vars_defined_in_body
+        .into_iter()
+        .filter_map(|var| var_outlives_body(ctx, body, var, parent))
+        .collect()
 }
 
 /// checks if the relevant local was defined before(outside of) body
@@ -855,11 +868,23 @@ fn either_syntax(value: &Either<ast::IdentPat, ast::SelfParam>) -> &SyntaxNode {
     }
 }
 
-/// checks if local variable is used after(outside of) body
-fn var_outlives_body(ctx: &AssistContext, body: &FunctionBody, var: &Local) -> bool {
-    let usages = LocalUsages::find(ctx, *var);
+/// returns usage details if local variable is used after(outside of) body
+fn var_outlives_body(
+    ctx: &AssistContext,
+    body: &FunctionBody,
+    var: Local,
+    parent: &SyntaxNode,
+) -> Option<OutlivedLocal> {
+    let usages = LocalUsages::find(ctx, var);
     let has_usages = usages.iter().any(|reference| body.preceedes_range(reference.range));
-    has_usages
+    if !has_usages {
+        return None;
+    }
+    let has_mut_usages = usages
+        .iter()
+        .filter(|reference| body.preceedes_range(reference.range))
+        .any(|reference| reference_is_exclusive(reference, parent, ctx));
+    Some(OutlivedLocal { local: var, mut_usage_outside_body: has_mut_usages })
 }
 
 fn body_return_ty(ctx: &AssistContext, body: &FunctionBody) -> Option<RetType> {
@@ -939,16 +964,25 @@ fn format_replacement(ctx: &AssistContext, fun: &Function, indent: IndentLevel)
     let mut buf = String::new();
     match fun.vars_defined_in_body_and_outlive.as_slice() {
         [] => {}
-        [var] => format_to!(buf, "let {} = ", var.name(ctx.db()).unwrap()),
+        [var] => {
+            format_to!(buf, "let {}{} = ", mut_modifier(var), var.local.name(ctx.db()).unwrap())
+        }
         [v0, vs @ ..] => {
             buf.push_str("let (");
-            format_to!(buf, "{}", v0.name(ctx.db()).unwrap());
+            format_to!(buf, "{}{}", mut_modifier(v0), v0.local.name(ctx.db()).unwrap());
             for var in vs {
-                format_to!(buf, ", {}", var.name(ctx.db()).unwrap());
+                format_to!(buf, ", {}{}", mut_modifier(var), var.local.name(ctx.db()).unwrap());
             }
             buf.push_str(") = ");
         }
     }
+    fn mut_modifier(var: &OutlivedLocal) -> &'static str {
+        if var.mut_usage_outside_body {
+            "mut "
+        } else {
+            ""
+        }
+    }
     format_to!(buf, "{}", expr);
     if fun.ret_ty.is_unit()
         && (!fun.vars_defined_in_body_and_outlive.is_empty() || !expr.is_block_like())
@@ -1211,10 +1245,10 @@ fn make_body(
                 match fun.vars_defined_in_body_and_outlive.as_slice() {
                     [] => {}
                     [var] => {
-                        tail_expr = Some(path_expr_from_local(ctx, *var));
+                        tail_expr = Some(path_expr_from_local(ctx, var.local));
                     }
                     vars => {
-                        let exprs = vars.iter().map(|var| path_expr_from_local(ctx, *var));
+                        let exprs = vars.iter().map(|var| path_expr_from_local(ctx, var.local));
                         let expr = make::expr_tuple(exprs);
                         tail_expr = Some(expr);
                     }
@@ -2123,6 +2157,30 @@ fn $0fun_name(n: i32) -> i32 {
     }
 
     #[test]
+    fn variable_defined_inside_and_used_after_mutably_no_ret() {
+        check_assist(
+            extract_function,
+            r"
+fn foo() {
+    let n = 1;
+    $0let mut k = n * n;$0
+    k += 1;
+}",
+            r"
+fn foo() {
+    let n = 1;
+    let mut k = fun_name(n);
+    k += 1;
+}
+
+fn $0fun_name(n: i32) -> i32 {
+    let mut k = n * n;
+    k
+}",
+        );
+    }
+
+    #[test]
     fn two_variables_defined_inside_and_used_after_no_ret() {
         check_assist(
             extract_function,
@@ -2149,6 +2207,38 @@ fn $0fun_name(n: i32) -> (i32, i32) {
     }
 
     #[test]
+    fn multi_variables_defined_inside_and_used_after_mutably_no_ret() {
+        check_assist(
+            extract_function,
+            r"
+fn foo() {
+    let n = 1;
+    $0let mut k = n * n;
+    let mut m = k + 2;
+    let mut o = m + 3;
+    o += 1;$0
+    k += o;
+    m = 1;
+}",
+            r"
+fn foo() {
+    let n = 1;
+    let (mut k, mut m, o) = fun_name(n);
+    k += o;
+    m = 1;
+}
+
+fn $0fun_name(n: i32) -> (i32, i32, i32) {
+    let mut k = n * n;
+    let mut m = k + 2;
+    let mut o = m + 3;
+    o += 1;
+    (k, m, o)
+}",
+        );
+    }
+
+    #[test]
     fn nontrivial_patterns_define_variables() {
         check_assist(
             extract_function,
diff --git a/crates/ide_assists/src/handlers/flip_comma.rs b/crates/ide_assists/src/handlers/flip_comma.rs
index 7f5e75d34..99be5e090 100644
--- a/crates/ide_assists/src/handlers/flip_comma.rs
+++ b/crates/ide_assists/src/handlers/flip_comma.rs
@@ -66,26 +66,12 @@ mod tests {
     }
 
     #[test]
-    #[should_panic]
     fn flip_comma_before_punct() {
         // See https://github.com/rust-analyzer/rust-analyzer/issues/1619
         // "Flip comma" assist shouldn't be applicable to the last comma in enum or struct
         // declaration body.
-        check_assist_target(
-            flip_comma,
-            "pub enum Test { \
-             A,$0 \
-             }",
-            ",",
-        );
-
-        check_assist_target(
-            flip_comma,
-            "pub struct Test { \
-             foo: usize,$0 \
-             }",
-            ",",
-        );
+        check_assist_not_applicable(flip_comma, "pub enum Test { A,$0 }");
+        check_assist_not_applicable(flip_comma, "pub struct Test { foo: usize,$0 }");
     }
 
     #[test]
diff --git a/crates/ide_assists/src/handlers/remove_dbg.rs b/crates/ide_assists/src/handlers/remove_dbg.rs
index 6114091f2..c8226550f 100644
--- a/crates/ide_assists/src/handlers/remove_dbg.rs
+++ b/crates/ide_assists/src/handlers/remove_dbg.rs
@@ -1,5 +1,5 @@
 use syntax::{
-    ast::{self, AstNode},
+    ast::{self, AstNode, AstToken},
     match_ast, SyntaxElement, TextRange, TextSize, T,
 };
 
@@ -24,7 +24,39 @@ pub(crate) fn remove_dbg(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
     let macro_call = ctx.find_node_at_offset::<ast::MacroCall>()?;
     let new_contents = adjusted_macro_contents(&macro_call)?;
 
-    let macro_text_range = macro_call.syntax().text_range();
+    let parent = macro_call.syntax().parent();
+
+    let macro_text_range = if let Some(it) = parent.as_ref() {
+        if new_contents.is_empty() {
+            match_ast! {
+                match it {
+                    ast::BlockExpr(_it) => {
+                        macro_call.syntax()
+                            .prev_sibling_or_token()
+                            .and_then(whitespace_start)
+                            .map(|start| TextRange::new(start, macro_call.syntax().text_range().end()))
+                            .unwrap_or(macro_call.syntax().text_range())
+                    },
+                    ast::ExprStmt(it) => {
+                        let start = it
+                            .syntax()
+                            .prev_sibling_or_token()
+                            .and_then(whitespace_start)
+                            .unwrap_or(it.syntax().text_range().start());
+                        let end = it.syntax().text_range().end();
+
+                        TextRange::new(start, end)
+                    },
+                    _ => macro_call.syntax().text_range()
+                }
+            }
+        } else {
+            macro_call.syntax().text_range()
+        }
+    } else {
+        macro_call.syntax().text_range()
+    };
+
     let macro_end = if macro_call.semicolon_token().is_some() {
         macro_text_range.end() - TextSize::of(';')
     } else {
@@ -36,11 +68,22 @@ pub(crate) fn remove_dbg(acc: &mut Assists, ctx: &AssistContext) -> Option<()> {
         "Remove dbg!()",
         macro_text_range,
         |builder| {
-            builder.replace(TextRange::new(macro_text_range.start(), macro_end), new_contents);
+            builder.replace(
+                TextRange::new(macro_text_range.start(), macro_end),
+                if new_contents.is_empty() && parent.and_then(ast::LetStmt::cast).is_some() {
+                    ast::make::expr_unit().to_string()
+                } else {
+                    new_contents
+                },
+            );
         },
     )
 }
 
+fn whitespace_start(it: SyntaxElement) -> Option<TextSize> {
+    Some(it.into_token().and_then(ast::Whitespace::cast)?.syntax().text_range().start())
+}
+
 fn adjusted_macro_contents(macro_call: &ast::MacroCall) -> Option<String> {
     let contents = get_valid_macrocall_contents(&macro_call, "dbg")?;
     let macro_text_with_brackets = macro_call.token_tree()?.syntax().text();
@@ -94,15 +137,11 @@ fn get_valid_macrocall_contents(
     let mut contents_between_brackets = children_with_tokens.collect::<Vec<_>>();
     let last_child = contents_between_brackets.pop()?;
 
-    if contents_between_brackets.is_empty() {
-        None
-    } else {
-        match (first_child.kind(), last_child.kind()) {
-            (T!['('], T![')']) | (T!['['], T![']']) | (T!['{'], T!['}']) => {
-                Some(contents_between_brackets)
-            }
-            _ => None,
+    match (first_child.kind(), last_child.kind()) {
+        (T!['('], T![')']) | (T!['['], T![']']) | (T!['{'], T!['}']) => {
+            Some(contents_between_brackets)
         }
+        _ => None,
     }
 }
 
@@ -418,4 +457,48 @@ fn main() {
 }"#,
         );
     }
+
+    #[test]
+    fn test_remove_empty_dbg() {
+        check_assist(remove_dbg, r#"fn foo() { $0dbg!(); }"#, r#"fn foo() { }"#);
+        check_assist(
+            remove_dbg,
+            r#"
+fn foo() {
+    $0dbg!();
+}
+"#,
+            r#"
+fn foo() {
+}
+"#,
+        );
+        check_assist(
+            remove_dbg,
+            r#"
+fn foo() {
+    let test = $0dbg!();
+}"#,
+            r#"
+fn foo() {
+    let test = ();
+}"#,
+        );
+        check_assist(
+            remove_dbg,
+            r#"
+fn foo() {
+    let t = {
+        println!("Hello, world");
+        $0dbg!()
+    };
+}"#,
+            r#"
+fn foo() {
+    let t = {
+        println!("Hello, world");
+    };
+}"#,
+        );
+    }
 }
diff --git a/crates/ide_assists/src/lib.rs b/crates/ide_assists/src/lib.rs
index 3e2c82dac..3694f468f 100644
--- a/crates/ide_assists/src/lib.rs
+++ b/crates/ide_assists/src/lib.rs
@@ -28,7 +28,9 @@ pub use assist_config::AssistConfig;
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum AssistKind {
+    // FIXME: does the None variant make sense? Probably not.
     None,
+
     QuickFix,
     Generate,
     Refactor,