From 7a1494ced5d762cdebf590619fc3326c4a876a7b Mon Sep 17 00:00:00 2001
From: Andrea Pretto <eulerdisk@gmail.com>
Date: Mon, 28 Jan 2019 15:12:07 +0100
Subject: [PATCH 1/2] 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 3e4434c2379..9035beba89d 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<Assist> {
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<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,28 +43,44 @@ 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| {
+/// 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 };
+}
+",
+ );
+ }
}
From a5fe4a08fb9b6e5df4f9aa1481fb62f6938897af Mon Sep 17 00:00:00 2001
From: Andrea Pretto <eulerdisk@gmail.com>
Date: Wed, 30 Jan 2019 21:36:49 +0100
Subject: [PATCH 2/2] 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 aea8397c99f..8905b041913 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<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| {
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 9035beba89d..ed13bddc48e 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<Assist> {
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<Assist> {
})
}
+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 dc2470aa30b..22ded243553 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<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,