From e603090961d950b1130950c179361b530c7ad10a Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 8 May 2021 20:40:07 +0300 Subject: minor: add missing test --- .../ide_assists/src/handlers/pull_assignment_up.rs | 52 +++++++++++++++------- 1 file changed, 36 insertions(+), 16 deletions(-) (limited to 'crates') diff --git a/crates/ide_assists/src/handlers/pull_assignment_up.rs b/crates/ide_assists/src/handlers/pull_assignment_up.rs index 04bae4e58..543b1dfe9 100644 --- a/crates/ide_assists/src/handlers/pull_assignment_up.rs +++ b/crates/ide_assists/src/handlers/pull_assignment_up.rs @@ -37,22 +37,24 @@ use crate::{ // ``` pub(crate) fn pull_assignment_up(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { let assign_expr = ctx.find_node_at_offset::()?; - let name_expr = if assign_expr.op_kind()? == ast::BinOp::Assignment { - assign_expr.lhs()? - } else { + + let op_kind = assign_expr.op_kind()?; + if op_kind != ast::BinOp::Assignment { + cov_mark::hit!(test_cant_pull_non_assignments); return None; - }; + } - let (old_stmt, new_stmt) = if let Some(if_expr) = ctx.find_node_at_offset::() { - ( - ast::Expr::cast(if_expr.syntax().to_owned())?, - exprify_if(&if_expr, &ctx.sema, &name_expr)?.indent(if_expr.indent_level()), - ) + let name_expr = assign_expr.lhs()?; + + let old_stmt: ast::Expr; + let new_stmt: ast::Expr; + + if let Some(if_expr) = ctx.find_node_at_offset::() { + new_stmt = exprify_if(&if_expr, &ctx.sema, &name_expr)?.indent(if_expr.indent_level()); + old_stmt = if_expr.into(); } else if let Some(match_expr) = ctx.find_node_at_offset::() { - ( - ast::Expr::cast(match_expr.syntax().to_owned())?, - exprify_match(&match_expr, &ctx.sema, &name_expr)?, - ) + new_stmt = exprify_match(&match_expr, &ctx.sema, &name_expr)?; + old_stmt = match_expr.into() } else { return None; }; @@ -99,9 +101,7 @@ fn exprify_if( ) -> Option { let then_branch = exprify_block(&statement.then_branch()?, sema, name)?; let else_branch = match statement.else_branch()? { - ast::ElseBranch::Block(ref block) => { - ast::ElseBranch::Block(exprify_block(block, sema, name)?) - } + ast::ElseBranch::Block(block) => ast::ElseBranch::Block(exprify_block(&block, sema, name)?), ast::ElseBranch::IfExpr(expr) => { cov_mark::hit!(test_pull_assignment_up_chained_if); ast::ElseBranch::IfExpr(ast::IfExpr::cast( @@ -436,6 +436,26 @@ fn foo() { 3 }; } +"#, + ) + } + + #[test] + fn test_cant_pull_non_assignments() { + cov_mark::check!(test_cant_pull_non_assignments); + check_assist_not_applicable( + pull_assignment_up, + r#" +fn foo() { + let mut a = 1; + let b = &mut a; + + if true { + $0*b + 2; + } else { + *b + 3; + } +} "#, ) } -- cgit v1.2.3 From 1755b57e1a568176bc0bda144889460e7d603cd5 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 8 May 2021 23:09:36 +0300 Subject: internal: pull_assignment_up uses mutable trees --- .../ide_assists/src/handlers/pull_assignment_up.rs | 137 ++++++++++----------- crates/syntax/src/ast/make.rs | 3 + 2 files changed, 71 insertions(+), 69 deletions(-) (limited to 'crates') diff --git a/crates/ide_assists/src/handlers/pull_assignment_up.rs b/crates/ide_assists/src/handlers/pull_assignment_up.rs index 543b1dfe9..602c3813e 100644 --- a/crates/ide_assists/src/handlers/pull_assignment_up.rs +++ b/crates/ide_assists/src/handlers/pull_assignment_up.rs @@ -1,6 +1,6 @@ use syntax::{ - ast::{self, edit::AstNodeEdit, make}, - AstNode, + ast::{self, make}, + ted, AstNode, }; use crate::{ @@ -44,96 +44,95 @@ pub(crate) fn pull_assignment_up(acc: &mut Assists, ctx: &AssistContext) -> Opti return None; } - let name_expr = assign_expr.lhs()?; - - let old_stmt: ast::Expr; - let new_stmt: ast::Expr; + let mut collector = AssignmentsCollector { + sema: &ctx.sema, + common_lhs: assign_expr.lhs()?, + assignments: Vec::new(), + }; - if let Some(if_expr) = ctx.find_node_at_offset::() { - new_stmt = exprify_if(&if_expr, &ctx.sema, &name_expr)?.indent(if_expr.indent_level()); - old_stmt = if_expr.into(); + let tgt: ast::Expr = if let Some(if_expr) = ctx.find_node_at_offset::() { + collector.collect_if(&if_expr)?; + if_expr.into() } else if let Some(match_expr) = ctx.find_node_at_offset::() { - new_stmt = exprify_match(&match_expr, &ctx.sema, &name_expr)?; - old_stmt = match_expr.into() + collector.collect_match(&match_expr)?; + match_expr.into() } else { return None; }; - let expr_stmt = make::expr_stmt(new_stmt); - acc.add( AssistId("pull_assignment_up", AssistKind::RefactorExtract), "Pull assignment up", - old_stmt.syntax().text_range(), + tgt.syntax().text_range(), move |edit| { - edit.replace(old_stmt.syntax().text_range(), format!("{} = {};", name_expr, expr_stmt)); + let assignments: Vec<_> = collector + .assignments + .into_iter() + .map(|(stmt, rhs)| (edit.make_ast_mut(stmt), rhs.clone_for_update())) + .collect(); + + let tgt = edit.make_ast_mut(tgt); + + for (stmt, rhs) in assignments { + ted::replace(stmt.syntax(), rhs.syntax()); + } + let assign_expr = make::expr_assignment(collector.common_lhs, tgt.clone()); + let assign_stmt = make::expr_stmt(assign_expr); + + ted::replace(tgt.syntax(), assign_stmt.syntax().clone_for_update()); }, ) } -fn exprify_match( - match_expr: &ast::MatchExpr, - sema: &hir::Semantics, - name: &ast::Expr, -) -> Option { - let new_arm_list = match_expr - .match_arm_list()? - .arms() - .map(|arm| { - if let ast::Expr::BlockExpr(block) = arm.expr()? { - let new_block = exprify_block(&block, sema, name)?.indent(block.indent_level()); - Some(arm.replace_descendant(block, new_block)) - } else { - None - } - }) - .collect::>>()?; - let new_arm_list = match_expr - .match_arm_list()? - .replace_descendants(match_expr.match_arm_list()?.arms().zip(new_arm_list)); - Some(make::expr_match(match_expr.expr()?, new_arm_list)) +struct AssignmentsCollector<'a> { + sema: &'a hir::Semantics<'a, ide_db::RootDatabase>, + common_lhs: ast::Expr, + assignments: Vec<(ast::ExprStmt, ast::Expr)>, } -fn exprify_if( - statement: &ast::IfExpr, - sema: &hir::Semantics, - name: &ast::Expr, -) -> Option { - let then_branch = exprify_block(&statement.then_branch()?, sema, name)?; - let else_branch = match statement.else_branch()? { - ast::ElseBranch::Block(block) => ast::ElseBranch::Block(exprify_block(&block, sema, name)?), - ast::ElseBranch::IfExpr(expr) => { - cov_mark::hit!(test_pull_assignment_up_chained_if); - ast::ElseBranch::IfExpr(ast::IfExpr::cast( - exprify_if(&expr, sema, name)?.syntax().to_owned(), - )?) +impl<'a> AssignmentsCollector<'a> { + fn collect_match(&mut self, match_expr: &ast::MatchExpr) -> Option<()> { + for arm in match_expr.match_arm_list()?.arms() { + match arm.expr()? { + ast::Expr::BlockExpr(block) => self.collect_block(&block)?, + // TODO: Handle this while we are at it? + _ => return None, + } } - }; - Some(make::expr_if(statement.condition()?, then_branch, Some(else_branch))) -} -fn exprify_block( - block: &ast::BlockExpr, - sema: &hir::Semantics, - name: &ast::Expr, -) -> Option { - if block.tail_expr().is_some() { - return None; + Some(()) } + fn collect_if(&mut self, if_expr: &ast::IfExpr) -> Option<()> { + let then_branch = if_expr.then_branch()?; + self.collect_block(&then_branch)?; + + match if_expr.else_branch()? { + ast::ElseBranch::Block(block) => self.collect_block(&block), + ast::ElseBranch::IfExpr(expr) => { + cov_mark::hit!(test_pull_assignment_up_chained_if); + self.collect_if(&expr) + } + } + } + fn collect_block(&mut self, block: &ast::BlockExpr) -> Option<()> { + if block.tail_expr().is_some() { + return None; + } - let mut stmts: Vec<_> = block.statements().collect(); - let stmt = stmts.pop()?; - - if let ast::Stmt::ExprStmt(stmt) = stmt { - if let ast::Expr::BinExpr(expr) = stmt.expr()? { - if expr.op_kind()? == ast::BinOp::Assignment && is_equivalent(sema, &expr.lhs()?, name) - { - // The last statement in the block is an assignment to the name we want - return Some(make::block_expr(stmts, Some(expr.rhs()?))); + let last_stmt = block.statements().last()?; + if let ast::Stmt::ExprStmt(stmt) = last_stmt { + if let ast::Expr::BinExpr(expr) = stmt.expr()? { + if expr.op_kind()? == ast::BinOp::Assignment + && is_equivalent(self.sema, &expr.lhs()?, &self.common_lhs) + { + self.assignments.push((stmt, expr.rhs()?)); + return Some(()); + } } } + + None } - None } fn is_equivalent( diff --git a/crates/syntax/src/ast/make.rs b/crates/syntax/src/ast/make.rs index f8b508a90..5a6687397 100644 --- a/crates/syntax/src/ast/make.rs +++ b/crates/syntax/src/ast/make.rs @@ -275,6 +275,9 @@ pub fn expr_tuple(elements: impl IntoIterator) -> ast::Expr { let expr = elements.into_iter().format(", "); expr_from_text(&format!("({})", expr)) } +pub fn expr_assignment(lhs: ast::Expr, rhs: ast::Expr) -> ast::Expr { + expr_from_text(&format!("{} = {}", lhs, rhs)) +} fn expr_from_text(text: &str) -> ast::Expr { ast_from_text(&format!("const C: () = {};", text)) } -- cgit v1.2.3 From 1ee12b5db17f6f4396af6bfd6ba2528de7f86c78 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 8 May 2021 23:19:08 +0300 Subject: feat: add "mentoring instructions" test for pull up assist --- .../ide_assists/src/handlers/pull_assignment_up.rs | 33 +++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) (limited to 'crates') diff --git a/crates/ide_assists/src/handlers/pull_assignment_up.rs b/crates/ide_assists/src/handlers/pull_assignment_up.rs index 602c3813e..28d14b9c3 100644 --- a/crates/ide_assists/src/handlers/pull_assignment_up.rs +++ b/crates/ide_assists/src/handlers/pull_assignment_up.rs @@ -95,7 +95,6 @@ impl<'a> AssignmentsCollector<'a> { for arm in match_expr.match_arm_list()?.arms() { match arm.expr()? { ast::Expr::BlockExpr(block) => self.collect_block(&block)?, - // TODO: Handle this while we are at it? _ => return None, } } @@ -241,6 +240,38 @@ fn foo() { ); } + #[test] + #[ignore] + fn test_pull_assignment_up_assignment_expressions() { + check_assist( + pull_assignment_up, + r#" +fn foo() { + let mut a = 1; + + match 1 { + 1 => { $0a = 2; }, + 2 => a = 3, + 3 => { + a = 4 + } + } +}"#, + r#" +fn foo() { + let mut a = 1; + + a = match 1 { + 1 => { 2 }, + 2 => 3, + 3 => { + 4 + } + }; +}"#, + ); + } + #[test] fn test_pull_assignment_up_not_last_not_applicable() { check_assist_not_applicable( -- cgit v1.2.3