From 7a1494ced5d762cdebf590619fc3326c4a876a7b Mon Sep 17 00:00:00 2001 From: Andrea Pretto Date: Mon, 28 Jan 2019 15:12:07 +0100 Subject: Fix #667 --- .../src/assists/introduce_variable.rs | 136 +++++++++++++++++++-- 1 file changed, 125 insertions(+), 11 deletions(-) diff --git a/crates/ra_ide_api_light/src/assists/introduce_variable.rs b/crates/ra_ide_api_light/src/assists/introduce_variable.rs index 3e4434c23..9035beba8 100644 --- a/crates/ra_ide_api_light/src/assists/introduce_variable.rs +++ b/crates/ra_ide_api_light/src/assists/introduce_variable.rs @@ -1,6 +1,6 @@ use ra_syntax::{ ast::{self, AstNode}, - SyntaxKind::WHITESPACE, + SyntaxKind::WHITESPACE, SyntaxKind::MATCH_ARM, SyntaxKind::LAMBDA_EXPR, SyntaxNode, TextUnit, }; @@ -10,7 +10,7 @@ pub fn introduce_variable<'a>(ctx: AssistCtx) -> Option { let node = ctx.covering_node(); let expr = node.ancestors().filter_map(ast::Expr::cast).next()?; - let anchor_stmt = anchor_stmt(expr)?; + let (anchor_stmt, wrap_in_block) = anchor_stmt(expr)?; let indent = anchor_stmt.prev_sibling()?; if indent.kind() != WHITESPACE { return None; @@ -18,7 +18,14 @@ pub fn introduce_variable<'a>(ctx: AssistCtx) -> Option { 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,28 +43,44 @@ pub fn introduce_variable<'a>(ctx: AssistCtx) -> Option { 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| { +/// 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 }) } @@ -161,4 +184,95 @@ 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 }; +} +", + ); + } } -- cgit v1.2.3 From a5fe4a08fb9b6e5df4f9aa1481fb62f6938897af Mon Sep 17 00:00:00 2001 From: Andrea Pretto Date: Wed, 30 Jan 2019 21:36:49 +0100 Subject: Some improvements to introduce_variable. --- crates/ra_ide_api_light/src/assists.rs | 8 + .../src/assists/introduce_variable.rs | 169 ++++++++++++++++++++- crates/ra_ide_api_light/src/test_utils.rs | 12 ++ 3 files changed, 181 insertions(+), 8 deletions(-) diff --git a/crates/ra_ide_api_light/src/assists.rs b/crates/ra_ide_api_light/src/assists.rs index aea8397c9..8905b0419 100644 --- a/crates/ra_ide_api_light/src/assists.rs +++ b/crates/ra_ide_api_light/src/assists.rs @@ -196,6 +196,14 @@ fn check_assist(assist: fn(AssistCtx) -> Option, before: &str, after: &s }) } +#[cfg(test)] +fn check_assist_not_applicable(assist: fn(AssistCtx) -> Option, 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, before: &str, after: &str) { crate::test_utils::check_action_range(before, after, |file, range| { diff --git a/crates/ra_ide_api_light/src/assists/introduce_variable.rs b/crates/ra_ide_api_light/src/assists/introduce_variable.rs index 9035beba8..ed13bddc4 100644 --- a/crates/ra_ide_api_light/src/assists/introduce_variable.rs +++ b/crates/ra_ide_api_light/src/assists/introduce_variable.rs @@ -1,15 +1,18 @@ use ra_syntax::{ ast::{self, AstNode}, - SyntaxKind::WHITESPACE, SyntaxKind::MATCH_ARM, SyntaxKind::LAMBDA_EXPR, - 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 { let node = ctx.covering_node(); - let expr = node.ancestors().filter_map(ast::Expr::cast).next()?; - + 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 { @@ -51,6 +54,21 @@ pub fn introduce_variable<'a>(ctx: AssistCtx) -> Option { }) } +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. @@ -73,9 +91,7 @@ fn anchor_stmt(expr: &ast::Expr) -> Option<(&SyntaxNode, bool)> { } if let Some(parent) = node.parent() { - if parent.kind() == MATCH_ARM - || parent.kind() == LAMBDA_EXPR - { + if parent.kind() == MATCH_ARM || parent.kind() == LAMBDA_EXPR { return Some((node, true)); } } @@ -87,7 +103,7 @@ fn anchor_stmt(expr: &ast::Expr) -> Option<(&SyntaxNode, bool)> { #[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() { @@ -272,6 +288,143 @@ fn main() { 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) + }; +} ", ); } diff --git a/crates/ra_ide_api_light/src/test_utils.rs b/crates/ra_ide_api_light/src/test_utils.rs index dc2470aa3..22ded2435 100644 --- a/crates/ra_ide_api_light/src/test_utils.rs +++ b/crates/ra_ide_api_light/src/test_utils.rs @@ -23,6 +23,18 @@ pub fn check_action Option>( assert_eq_text!(after, &actual); } +pub fn check_action_not_applicable Option>( + 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 Option>( before: &str, after: &str, -- cgit v1.2.3