Merge #3045

3045: Cleanup early return assist r=matklad a=matklad



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

crates/ra_assists/src/assists/apply_demorgan.rs

@@ -31,12 +31,14 @@ pub(crate) fn apply_demorgan(ctx: AssistCtx) -> Option<Assist> {
     if !cursor_in_range {
         return None;
     }
+
     let lhs = expr.lhs()?;
     let lhs_range = lhs.syntax().text_range();
+    let not_lhs = invert_boolean_expression(lhs);
+
     let rhs = expr.rhs()?;
     let rhs_range = rhs.syntax().text_range();
-    let not_lhs = invert_boolean_expression(&lhs)?;
-    let not_rhs = invert_boolean_expression(&rhs)?;
+    let not_rhs = invert_boolean_expression(rhs);
 
     ctx.add_assist(AssistId("apply_demorgan"), "Apply De Morgan's law", |edit| {
         edit.target(op_range);
@@ -77,12 +79,12 @@ mod tests {
     }
 
     #[test]
-    fn demorgan_doesnt_apply_with_cursor_not_on_op() {
-        check_assist_not_applicable(apply_demorgan, "fn f() { <|> !x || !x }")
+    fn demorgan_general_case() {
+        check_assist(apply_demorgan, "fn f() { x ||<|> x }", "fn f() { !(!x &&<|> !x) }")
     }
 
     #[test]
-    fn demorgan_doesnt_apply_when_operands_arent_negated_already() {
-        check_assist_not_applicable(apply_demorgan, "fn f() { x ||<|> x }")
+    fn demorgan_doesnt_apply_with_cursor_not_on_op() {
+        check_assist_not_applicable(apply_demorgan, "fn f() { <|> !x || !x }")
     }
 }

crates/ra_assists/src/assists/early_return.rs

@@ -10,6 +10,7 @@ use ra_syntax::{
 
 use crate::{
     assist_ctx::{Assist, AssistCtx},
+    assists::invert_if::invert_boolean_expression,
     AssistId,
 };
 
@@ -99,9 +100,13 @@ pub(crate) fn convert_to_guarded_return(ctx: AssistCtx) -> Option<Assist> {
         let new_block = match if_let_pat {
             None => {
                 // If.
-                let early_expression = &(early_expression.syntax().to_string() + ";");
-                let new_expr = if_indent_level
-                    .increase_indent(make::if_expression(cond_expr, early_expression));
+                let new_expr = {
+                    let then_branch =
+                        make::block_expr(once(make::expr_stmt(early_expression).into()), None);
+                    let cond = invert_boolean_expression(cond_expr);
+                    let e = make::expr_if(cond, then_branch);
+                    if_indent_level.increase_indent(e)
+                };
                 replace(new_expr.syntax(), &then_block, &parent_block, &if_expr)
             }
             Some((path, bound_ident)) => {

crates/ra_assists/src/assists/invert_if.rs

@@ -1,4 +1,4 @@
-use ra_syntax::ast::{self, AstNode};
+use ra_syntax::ast::{self, make, AstNode};
 use ra_syntax::T;
 
 use crate::{Assist, AssistCtx, AssistId};
@@ -35,8 +35,8 @@ pub(crate) fn invert_if(ctx: AssistCtx) -> Option<Assist> {
     let then_node = expr.then_branch()?.syntax().clone();
 
     if let ast::ElseBranch::Block(else_block) = expr.else_branch()? {
-        let flip_cond = invert_boolean_expression(&cond)?;
         let cond_range = cond.syntax().text_range();
+        let flip_cond = invert_boolean_expression(cond);
         let else_node = else_block.syntax();
         let else_range = else_node.text_range();
         let then_range = then_node.text_range();
@@ -51,16 +51,23 @@ pub(crate) fn invert_if(ctx: AssistCtx) -> Option<Assist> {
     None
 }
 
-pub(crate) fn invert_boolean_expression(expr: &ast::Expr) -> Option<ast::Expr> {
+pub(crate) fn invert_boolean_expression(expr: ast::Expr) -> ast::Expr {
+    if let Some(expr) = invert_special_case(&expr) {
+        return expr;
+    }
+    make::expr_prefix(T![!], expr)
+}
+
+pub(crate) fn invert_special_case(expr: &ast::Expr) -> Option<ast::Expr> {
     match expr {
         ast::Expr::BinExpr(bin) => match bin.op_kind()? {
             ast::BinOp::NegatedEqualityTest => bin.replace_op(T![==]).map(|it| it.into()),
+            ast::BinOp::EqualityTest => bin.replace_op(T![!=]).map(|it| it.into()),
             _ => None,
         },
-        ast::Expr::PrefixExpr(pe) => match pe.op_kind()? {
-            ast::PrefixOp::Not => pe.expr(),
-            _ => None,
-        },
+        ast::Expr::PrefixExpr(pe) if pe.op_kind()? == ast::PrefixOp::Not => pe.expr(),
+        // FIXME:
+        // ast::Expr::Literal(true | false )
         _ => None,
     }
 }
@@ -90,12 +97,16 @@ mod tests {
     }
 
     #[test]
-    fn invert_if_doesnt_apply_with_cursor_not_on_if() {
-        check_assist_not_applicable(invert_if, "fn f() { if !<|>cond { 3 * 2 } else { 1 } }")
+    fn invert_if_general_case() {
+        check_assist(
+            invert_if,
+            "fn f() { i<|>f cond { 3 * 2 } else { 1 } }",
+            "fn f() { i<|>f !cond { 1 } else { 3 * 2 } }",
+        )
     }
 
     #[test]
-    fn invert_if_doesnt_apply_without_negated() {
-        check_assist_not_applicable(invert_if, "fn f() { i<|>f cond { 3 * 2 } else { 1 } }")
+    fn invert_if_doesnt_apply_with_cursor_not_on_if() {
+        check_assist_not_applicable(invert_if, "fn f() { if !<|>cond { 3 * 2 } else { 1 } }")
     }
 }

crates/ra_syntax/src/ast/make.rs

@@ -33,6 +33,21 @@ pub fn record_field(name: ast::NameRef, expr: Option<ast::Expr>) -> ast::RecordF
     }
 }
 
+pub fn block_expr(
+    stmts: impl IntoIterator<Item = ast::Stmt>,
+    tail_expr: Option<ast::Expr>,
+) -> ast::BlockExpr {
+    let mut text = "{\n".to_string();
+    for stmt in stmts.into_iter() {
+        text += &format!("    {}\n", stmt.syntax());
+    }
+    if let Some(tail_expr) = tail_expr {
+        text += &format!("    {}\n", tail_expr.syntax())
+    }
+    text += "}";
+    ast_from_text(&format!("fn f() {}", text))
+}
+
 pub fn block_from_expr(e: ast::Expr) -> ast::Block {
     return from_text(&format!("{{ {} }}", e.syntax()));
 
@@ -62,6 +77,13 @@ pub fn expr_return() -> ast::Expr {
 pub fn expr_match(expr: ast::Expr, match_arm_list: ast::MatchArmList) -> ast::Expr {
     expr_from_text(&format!("match {} {}", expr.syntax(), match_arm_list.syntax()))
 }
+pub fn expr_if(condition: ast::Expr, then_branch: ast::BlockExpr) -> ast::Expr {
+    expr_from_text(&format!("if {} {}", condition.syntax(), then_branch.syntax()))
+}
+pub fn expr_prefix(op: SyntaxKind, expr: ast::Expr) -> ast::Expr {
+    let token = token(op);
+    expr_from_text(&format!("{}{}", token, expr.syntax()))
+}
 fn expr_from_text(text: &str) -> ast::Expr {
     ast_from_text(&format!("const C: () = {};", text))
 }
@@ -158,14 +180,6 @@ pub fn where_clause(preds: impl IntoIterator<Item = ast::WherePred>) -> ast::Whe
     }
 }
 
-pub fn if_expression(condition: ast::Expr, statement: &str) -> ast::IfExpr {
-    ast_from_text(&format!(
-        "fn f() {{ if !{} {{\n    {}\n}}\n}}",
-        condition.syntax().text(),
-        statement
-    ))
-}
-
 pub fn let_stmt(pattern: ast::Pat, initializer: Option<ast::Expr>) -> ast::LetStmt {
     let text = match initializer {
         Some(it) => format!("let {} = {};", pattern.syntax(), it.syntax()),
@@ -173,6 +187,9 @@ pub fn let_stmt(pattern: ast::Pat, initializer: Option<ast::Expr>) -> ast::LetSt
     };
     ast_from_text(&format!("fn f() {{ {} }}", text))
 }
+pub fn expr_stmt(expr: ast::Expr) -> ast::ExprStmt {
+    ast_from_text(&format!("fn f() {{ {}; }}", expr.syntax()))
+}
 
 pub fn token(kind: SyntaxKind) -> SyntaxToken {
     tokens::SOURCE_FILE
@@ -203,7 +220,7 @@ pub mod tokens {
     use once_cell::sync::Lazy;
 
     pub(super) static SOURCE_FILE: Lazy<Parse<SourceFile>> =
-        Lazy::new(|| SourceFile::parse("const C: <()>::Item = (1 != 1, 2 == 2)\n;"));
+        Lazy::new(|| SourceFile::parse("const C: <()>::Item = (1 != 1, 2 == 2, !true)\n;"));
 
     pub fn comma() -> SyntaxToken {
         SOURCE_FILE