Merge #712

712:  Fix #667 and improvements to introduce_variable r=matklad a=eulerdisk

Fix #667 (but not re-indenting currently), plus many other improvements.
@matklad I'm not sure how to handle re-indenting here.

Co-authored-by: Andrea Pretto <eulerdisk@gmail.com>

crates/ra_ide_api_light/src/assists.rs

@@ -196,6 +196,14 @@ fn check_assist(assist: fn(AssistCtx) -> Option<Assist>, before: &str, after: &s
     })
 }
 
+#[cfg(test)]
+fn check_assist_not_applicable(assist: fn(AssistCtx) -> Option<Assist>, text: &str) {
+    crate::test_utils::check_action_not_applicable(text, |file, off| {
+        let range = TextRange::offset_len(off, 0.into());
+        AssistCtx::new(file, range).apply(assist)
+    })
+}
+
 #[cfg(test)]
 fn check_assist_range(assist: fn(AssistCtx) -> Option<Assist>, before: &str, after: &str) {
     crate::test_utils::check_action_range(before, after, |file, range| {

crates/ra_ide_api_light/src/assists/introduce_variable.rs

@@ -1,16 +1,19 @@
 use ra_syntax::{
     ast::{self, AstNode},
-    SyntaxKind::WHITESPACE,
-    SyntaxNode, TextUnit,
+    SyntaxKind::{
+        WHITESPACE, MATCH_ARM, LAMBDA_EXPR, PATH_EXPR, BREAK_EXPR, LOOP_EXPR, RETURN_EXPR, COMMENT
+    }, SyntaxNode, TextUnit,
 };
 
 use crate::assists::{AssistCtx, Assist};
 
 pub fn introduce_variable<'a>(ctx: AssistCtx) -> Option<Assist> {
     let node = ctx.covering_node();
-    let expr = node.ancestors().filter_map(ast::Expr::cast).next()?;
-
-    let anchor_stmt = anchor_stmt(expr)?;
+    if !valid_covering_node(node) {
+        return None;
+    }
+    let expr = node.ancestors().filter_map(valid_target_expr).next()?;
+    let (anchor_stmt, wrap_in_block) = anchor_stmt(expr)?;
     let indent = anchor_stmt.prev_sibling()?;
     if indent.kind() != WHITESPACE {
         return None;
@@ -18,7 +21,14 @@ pub fn introduce_variable<'a>(ctx: AssistCtx) -> Option<Assist> {
     ctx.build("introduce variable", move |edit| {
         let mut buf = String::new();
 
-        buf.push_str("let var_name = ");
+        let cursor_offset = if wrap_in_block {
+            buf.push_str("{ let var_name = ");
+            TextUnit::of_str("{ let ")
+        } else {
+            buf.push_str("let var_name = ");
+            TextUnit::of_str("let ")
+        };
+
         expr.syntax().text().push_to(&mut buf);
         let full_stmt = ast::ExprStmt::cast(anchor_stmt);
         let is_full_stmt = if let Some(expr_stmt) = full_stmt {
@@ -36,35 +46,64 @@ pub fn introduce_variable<'a>(ctx: AssistCtx) -> Option<Assist> {
             indent.text().push_to(&mut buf);
             edit.replace(expr.syntax().range(), "var_name".to_string());
             edit.insert(anchor_stmt.range().start(), buf);
+            if wrap_in_block {
+                edit.insert(anchor_stmt.range().end(), " }");
+            }
         }
-        edit.set_cursor(anchor_stmt.range().start() + TextUnit::of_str("let "));
+        edit.set_cursor(anchor_stmt.range().start() + cursor_offset);
     })
 }
 
-/// Statement or last in the block expression, which will follow
-/// the freshly introduced var.
-fn anchor_stmt(expr: &ast::Expr) -> Option<&SyntaxNode> {
-    expr.syntax().ancestors().find(|&node| {
+fn valid_covering_node(node: &SyntaxNode) -> bool {
+    node.kind() != COMMENT
+}
+/// Check wether the node is a valid expression which can be extracted to a variable.
+/// In general that's true for any expression, but in some cases that would produce invalid code.
+fn valid_target_expr(node: &SyntaxNode) -> Option<&ast::Expr> {
+    return match node.kind() {
+        PATH_EXPR => None,
+        BREAK_EXPR => ast::BreakExpr::cast(node).and_then(|e| e.expr()),
+        RETURN_EXPR => ast::ReturnExpr::cast(node).and_then(|e| e.expr()),
+        LOOP_EXPR => ast::ReturnExpr::cast(node).and_then(|e| e.expr()),
+        _ => ast::Expr::cast(node),
+    };
+}
+
+/// Returns the syntax node which will follow the freshly introduced var
+/// and a boolean indicating whether we have to wrap it within a { } block
+/// to produce correct code.
+/// It can be a statement, the last in a block expression or a wanna be block
+/// expression like a lamba or match arm.
+fn anchor_stmt(expr: &ast::Expr) -> Option<(&SyntaxNode, bool)> {
+    expr.syntax().ancestors().find_map(|node| {
         if ast::Stmt::cast(node).is_some() {
-            return true;
+            return Some((node, false));
         }
+
         if let Some(expr) = node
             .parent()
             .and_then(ast::Block::cast)
             .and_then(|it| it.expr())
         {
             if expr.syntax() == node {
-                return true;
+                return Some((node, false));
             }
         }
-        false
+
+        if let Some(parent) = node.parent() {
+            if parent.kind() == MATCH_ARM || parent.kind() == LAMBDA_EXPR {
+                return Some((node, true));
+            }
+        }
+
+        None
     })
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::assists::check_assist_range;
+    use crate::assists::{ check_assist, check_assist_not_applicable, check_assist_range };
 
     #[test]
     fn test_introduce_var_simple() {
@@ -161,4 +200,232 @@ fn foo() {
 }",
         );
     }
+
+    #[test]
+    fn test_introduce_var_in_match_arm_no_block() {
+        check_assist_range(
+            introduce_variable,
+            "
+fn main() {
+    let x = true;
+    let tuple = match x {
+        true => (<|>2 + 2<|>, true)
+        _ => (0, false)
+    };
+}
+",
+            "
+fn main() {
+    let x = true;
+    let tuple = match x {
+        true => { let <|>var_name = 2 + 2; (var_name, true) }
+        _ => (0, false)
+    };
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_in_match_arm_with_block() {
+        check_assist_range(
+            introduce_variable,
+            "
+fn main() {
+    let x = true;
+    let tuple = match x {
+        true => {
+            let y = 1;
+            (<|>2 + y<|>, true)
+        }
+        _ => (0, false)
+    };
+}
+",
+            "
+fn main() {
+    let x = true;
+    let tuple = match x {
+        true => {
+            let y = 1;
+            let <|>var_name = 2 + y;
+            (var_name, true)
+        }
+        _ => (0, false)
+    };
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_in_closure_no_block() {
+        check_assist_range(
+            introduce_variable,
+            "
+fn main() {
+    let lambda = |x: u32| <|>x * 2<|>;
+}
+",
+            "
+fn main() {
+    let lambda = |x: u32| { let <|>var_name = x * 2; var_name };
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_in_closure_with_block() {
+        check_assist_range(
+            introduce_variable,
+            "
+fn main() {
+    let lambda = |x: u32| { <|>x * 2<|> };
+}
+",
+            "
+fn main() {
+    let lambda = |x: u32| { let <|>var_name = x * 2; var_name };
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_path_simple() {
+        check_assist(
+            introduce_variable,
+            "
+fn main() {
+    let o = S<|>ome(true);
+}
+",
+            "
+fn main() {
+    let <|>var_name = Some(true);
+    let o = var_name;
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_path_method() {
+        check_assist(
+            introduce_variable,
+            "
+fn main() {
+    let v = b<|>ar.foo();
+}
+",
+            "
+fn main() {
+    let <|>var_name = bar.foo();
+    let v = var_name;
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_return() {
+        check_assist(
+            introduce_variable,
+            "
+fn foo() -> u32 {
+    r<|>eturn 2 + 2;
+}
+",
+            "
+fn foo() -> u32 {
+    let <|>var_name = 2 + 2;
+    return var_name;
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_break() {
+        check_assist(
+            introduce_variable,
+            "
+fn main() {
+    let result = loop {
+        b<|>reak 2 + 2;
+    };
+}
+",
+            "
+fn main() {
+    let result = loop {
+        let <|>var_name = 2 + 2;
+        break var_name;
+    };
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_for_cast() {
+        check_assist(
+            introduce_variable,
+            "
+fn main() {
+    let v = 0f32 a<|>s u32;
+}
+",
+            "
+fn main() {
+    let <|>var_name = 0f32 as u32;
+    let v = var_name;
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_for_return_not_applicable() {
+        check_assist_not_applicable(
+            introduce_variable,
+            "
+fn foo() {
+    r<|>eturn;
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_for_break_not_applicable() {
+        check_assist_not_applicable(
+            introduce_variable,
+            "
+fn main() {
+    loop {
+        b<|>reak;
+    };
+}
+",
+        );
+    }
+
+    #[test]
+    fn test_introduce_var_in_comment_not_applicable() {
+        check_assist_not_applicable(
+            introduce_variable,
+            "
+fn main() {
+    let x = true;
+    let tuple = match x {
+        // c<|>omment
+        true => (2 + 2, true)
+        _ => (0, false)
+    };
+}
+",
+        );
+    }
 }

crates/ra_ide_api_light/src/test_utils.rs

@@ -23,6 +23,18 @@ pub fn check_action<F: Fn(&SourceFile, TextUnit) -> Option<LocalEdit>>(
     assert_eq_text!(after, &actual);
 }
 
+pub fn check_action_not_applicable<F: Fn(&SourceFile, TextUnit) -> Option<LocalEdit>>(
+    text: &str,
+    f: F,
+) {
+    let (text_cursor_pos, text) = extract_offset(text);
+    let file = SourceFile::parse(&text);
+    assert!(
+        f(&file, text_cursor_pos).is_none(),
+        "code action is applicable but it shouldn't"
+    );
+}
+
 pub fn check_action_range<F: Fn(&SourceFile, TextRange) -> Option<LocalEdit>>(
     before: &str,
     after: &str,