From dbaf2ce76e8f24ea5e59df1622f77f62cb02c719 Mon Sep 17 00:00:00 2001 From: austaras Date: Fri, 26 Aug 2022 16:52:45 +0800 Subject: [PATCH 1/5] turn `unwrap_or` into `unwrap_or_else` and vice versa --- .../src/handlers/replace_or_with_or_else.rs | 230 ++++++++++++++++++ crates/ide-assists/src/lib.rs | 3 + 2 files changed, 233 insertions(+) create mode 100644 crates/ide-assists/src/handlers/replace_or_with_or_else.rs diff --git a/crates/ide-assists/src/handlers/replace_or_with_or_else.rs b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs new file mode 100644 index 00000000000..b5b5798b8bc --- /dev/null +++ b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs @@ -0,0 +1,230 @@ +use ide_db::assists::{AssistId, AssistKind}; +use syntax::{ + ast::{self, make, HasArgList}, + AstNode, +}; + +use crate::{AssistContext, Assists}; + +// Assist: replace_or_with_or_else +// +// Replace `unwrap_or` with `unwrap_or_else` and `ok_or` with `ok_or_else`. +// +// ``` +// let a = Some(1); +// a.unwra$0p_or(2); +// ``` +// -> +// ``` +// let a = Some(1); +// a.unwrap_or_else(|| 2); +// ``` +pub(crate) fn replace_or_with_or_else(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { + let call: ast::MethodCallExpr = ctx.find_node_at_offset()?; + let (name, arg_list) = (call.name_ref()?, call.arg_list()?); + + let replace = match &*name.text() { + "unwrap_or" => "unwrap_or_else".to_string(), + "ok_or" => "ok_or_else".to_string(), + _ => return None, + }; + + let arg = match arg_list.args().collect::>().as_slice() { + [] => make::arg_list(Vec::new()), + [first] => { + let param = (|| { + if let ast::Expr::CallExpr(call) = first { + if call.arg_list()?.args().count() == 0 { + Some(call.expr()?.clone()) + } else { + None + } + } else { + None + } + })() + .unwrap_or_else(|| make::expr_closure(None, first.clone())); + make::arg_list(vec![param]) + } + _ => return None, + }; + + acc.add( + AssistId("replace_or_with_or_else", AssistKind::RefactorRewrite), + "Replace unwrap_or or ok_or with lazy version", + call.syntax().text_range(), + |builder| { + builder.replace(name.syntax().text_range(), replace); + builder.replace_ast(arg_list, arg) + }, + ) +} + +// Assist: replace_or_else_with_or +// +// Replace `unwrap_or_else` with `unwrap_or` and `ok_or_else` with `ok_or`. +// +// ``` +// let a = Some(1); +// a.unwra$0p_or_else(|| 2); +// ``` +// -> +// ``` +// let a = Some(1); +// a.unwrap_or(2); +// ``` +pub(crate) fn replace_or_else_with_or(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { + let call: ast::MethodCallExpr = ctx.find_node_at_offset()?; + + let (name, arg_list) = (call.name_ref()?, call.arg_list()?); + + let replace = match &*name.text() { + "unwrap_or_else" => "unwrap_or".to_string(), + "ok_or_else" => "ok_or".to_string(), + _ => return None, + }; + + let arg = match arg_list.args().collect::>().as_slice() { + [] => make::arg_list(Vec::new()), + [first] => { + let param = (|| { + if let ast::Expr::ClosureExpr(closure) = first { + if closure.param_list()?.params().count() == 0 { + Some(closure.body()?.clone()) + } else { + None + } + } else { + None + } + })() + .unwrap_or_else(|| make::expr_call(first.clone(), make::arg_list(Vec::new()))); + make::arg_list(vec![param]) + } + _ => return None, + }; + + acc.add( + AssistId("replace_or_else_with_or", AssistKind::RefactorRewrite), + "Replace unwrap_or_else or ok_or_else with eager version", + call.syntax().text_range(), + |builder| { + builder.replace(name.syntax().text_range(), replace); + builder.replace_ast(arg_list, arg) + }, + ) +} + +#[cfg(test)] +mod tests { + use crate::tests::check_assist; + + use super::*; + + #[test] + fn replace_or_with_or_else_simple() { + check_assist( + replace_or_with_or_else, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_$0or(2); +} +"#, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_or_else(|| 2); +} +"#, + ) + } + + #[test] + fn replace_or_with_or_else_call() { + check_assist( + replace_or_with_or_else, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_$0or(x()); +} +"#, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_or_else(x); +} +"#, + ) + } + + #[test] + fn replace_or_with_or_else_block() { + check_assist( + replace_or_with_or_else, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_$0or({ + let mut x = bar(); + for i in 0..10 { + x += i; + } + x + }); +} +"#, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_or_else(|| { + let mut x = bar(); + for i in 0..10 { + x += i; + } + x + }); +} +"#, + ) + } + + #[test] + fn replace_or_else_with_or_simple() { + check_assist( + replace_or_else_with_or, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_$0or_else(|| 2); +} +"#, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_or(2); +} +"#, + ) + } + + #[test] + fn replace_or_else_with_or_call() { + check_assist( + replace_or_else_with_or, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_$0or_else(x); +} +"#, + r#" +fn foo() { + let foo = Some(1); + return foo.unwrap_or(x()); +} +"#, + ) + } +} diff --git a/crates/ide-assists/src/lib.rs b/crates/ide-assists/src/lib.rs index 7fb35143fa2..94fe387efe9 100644 --- a/crates/ide-assists/src/lib.rs +++ b/crates/ide-assists/src/lib.rs @@ -179,6 +179,7 @@ mod handlers { mod replace_try_expr_with_match; mod replace_derive_with_manual_impl; mod replace_if_let_with_match; + mod replace_or_with_or_else; mod introduce_named_generic; mod replace_let_with_if_let; mod replace_qualified_name_with_use; @@ -273,6 +274,8 @@ mod handlers { replace_if_let_with_match::replace_if_let_with_match, replace_if_let_with_match::replace_match_with_if_let, replace_let_with_if_let::replace_let_with_if_let, + replace_or_with_or_else::replace_or_else_with_or, + replace_or_with_or_else::replace_or_with_or_else, replace_turbofish_with_explicit_type::replace_turbofish_with_explicit_type, replace_qualified_name_with_use::replace_qualified_name_with_use, sort_items::sort_items, From 0dd9eef1b9bce0b5b6260c620f5e37f50b618ae5 Mon Sep 17 00:00:00 2001 From: austaras Date: Fri, 26 Aug 2022 18:09:34 +0800 Subject: [PATCH 2/5] add type check --- .../src/handlers/replace_or_with_or_else.rs | 75 ++++++++++++++++++- 1 file changed, 72 insertions(+), 3 deletions(-) diff --git a/crates/ide-assists/src/handlers/replace_or_with_or_else.rs b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs index b5b5798b8bc..96314263c97 100644 --- a/crates/ide-assists/src/handlers/replace_or_with_or_else.rs +++ b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs @@ -1,6 +1,9 @@ -use ide_db::assists::{AssistId, AssistKind}; +use ide_db::{ + assists::{AssistId, AssistKind}, + famous_defs::FamousDefs, +}; use syntax::{ - ast::{self, make, HasArgList}, + ast::{self, make, Expr, HasArgList}, AstNode, }; @@ -21,6 +24,9 @@ use crate::{AssistContext, Assists}; // ``` pub(crate) fn replace_or_with_or_else(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { let call: ast::MethodCallExpr = ctx.find_node_at_offset()?; + + is_option_or_result(call.receiver()?, ctx)?; + let (name, arg_list) = (call.name_ref()?, call.arg_list()?); let replace = match &*name.text() { @@ -76,6 +82,8 @@ pub(crate) fn replace_or_with_or_else(acc: &mut Assists, ctx: &AssistContext<'_> pub(crate) fn replace_or_else_with_or(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { let call: ast::MethodCallExpr = ctx.find_node_at_offset()?; + is_option_or_result(call.receiver()?, ctx)?; + let (name, arg_list) = (call.name_ref()?, call.arg_list()?); let replace = match &*name.text() { @@ -115,9 +123,32 @@ pub(crate) fn replace_or_else_with_or(acc: &mut Assists, ctx: &AssistContext<'_> ) } +fn is_option_or_result(receiver: Expr, ctx: &AssistContext<'_>) -> Option<()> { + let ty = ctx.sema.type_of_expr(&receiver)?.adjusted().as_adt()?.as_enum()?; + let option_enum = + FamousDefs(&ctx.sema, ctx.sema.scope(receiver.syntax())?.krate()).core_option_Option(); + + if let Some(option_enum) = option_enum { + if ty == option_enum { + return Some(()); + } + } + + let result_enum = + FamousDefs(&ctx.sema, ctx.sema.scope(receiver.syntax())?.krate()).core_result_Result(); + + if let Some(result_enum) = result_enum { + if ty == result_enum { + return Some(()); + } + } + + None +} + #[cfg(test)] mod tests { - use crate::tests::check_assist; + use crate::tests::{check_assist, check_assist_not_applicable}; use super::*; @@ -126,6 +157,7 @@ mod tests { check_assist( replace_or_with_or_else, r#" +//- minicore: option fn foo() { let foo = Some(1); return foo.unwrap_$0or(2); @@ -145,6 +177,7 @@ fn foo() { check_assist( replace_or_with_or_else, r#" +//- minicore: option fn foo() { let foo = Some(1); return foo.unwrap_$0or(x()); @@ -164,6 +197,7 @@ fn foo() { check_assist( replace_or_with_or_else, r#" +//- minicore: option fn foo() { let foo = Some(1); return foo.unwrap_$0or({ @@ -195,6 +229,7 @@ fn foo() { check_assist( replace_or_else_with_or, r#" +//- minicore: option fn foo() { let foo = Some(1); return foo.unwrap_$0or_else(|| 2); @@ -214,6 +249,7 @@ fn foo() { check_assist( replace_or_else_with_or, r#" +//- minicore: option fn foo() { let foo = Some(1); return foo.unwrap_$0or_else(x); @@ -224,6 +260,39 @@ fn foo() { let foo = Some(1); return foo.unwrap_or(x()); } +"#, + ) + } + + #[test] + fn replace_or_else_with_or_result() { + check_assist( + replace_or_else_with_or, + r#" +//- minicore: result +fn foo() { + let foo = Ok(1); + return foo.unwrap_$0or_else(x); +} +"#, + r#" +fn foo() { + let foo = Ok(1); + return foo.unwrap_or(x()); +} +"#, + ) + } + + #[test] + fn replace_or_else_with_or_not_applicable() { + check_assist_not_applicable( + replace_or_else_with_or, + r#" +fn foo() { + let foo = Ok(1); + return foo.unwrap_$0or_else(x); +} "#, ) } From f9c180ffd1702dfded1e0cd6c6d3e12a40e14d0a Mon Sep 17 00:00:00 2001 From: austaras Date: Fri, 26 Aug 2022 18:12:38 +0800 Subject: [PATCH 3/5] update tests --- .../src/handlers/replace_or_with_or_else.rs | 26 ++++++++---- crates/ide-assists/src/tests/generated.rs | 40 +++++++++++++++++++ 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/crates/ide-assists/src/handlers/replace_or_with_or_else.rs b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs index 96314263c97..bc1122a9d23 100644 --- a/crates/ide-assists/src/handlers/replace_or_with_or_else.rs +++ b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs @@ -14,13 +14,18 @@ use crate::{AssistContext, Assists}; // Replace `unwrap_or` with `unwrap_or_else` and `ok_or` with `ok_or_else`. // // ``` -// let a = Some(1); -// a.unwra$0p_or(2); +// # //- minicore:option +// fn foo() { +// let a = Some(1); +// a.unwra$0p_or(2); +// } // ``` // -> // ``` -// let a = Some(1); -// a.unwrap_or_else(|| 2); +// fn foo() { +// let a = Some(1); +// a.unwrap_or_else(|| 2); +// } // ``` pub(crate) fn replace_or_with_or_else(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { let call: ast::MethodCallExpr = ctx.find_node_at_offset()?; @@ -71,13 +76,18 @@ pub(crate) fn replace_or_with_or_else(acc: &mut Assists, ctx: &AssistContext<'_> // Replace `unwrap_or_else` with `unwrap_or` and `ok_or_else` with `ok_or`. // // ``` -// let a = Some(1); -// a.unwra$0p_or_else(|| 2); +// # //- minicore:option +// fn foo() { +// let a = Some(1); +// a.unwra$0p_or_else(|| 2); +// } // ``` // -> // ``` -// let a = Some(1); -// a.unwrap_or(2); +// fn foo() { +// let a = Some(1); +// a.unwrap_or(2); +// } // ``` pub(crate) fn replace_or_else_with_or(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { let call: ast::MethodCallExpr = ctx.find_node_at_offset()?; diff --git a/crates/ide-assists/src/tests/generated.rs b/crates/ide-assists/src/tests/generated.rs index 22319f36134..15992722b14 100644 --- a/crates/ide-assists/src/tests/generated.rs +++ b/crates/ide-assists/src/tests/generated.rs @@ -2009,6 +2009,46 @@ fn handle(action: Action) { ) } +#[test] +fn doctest_replace_or_else_with_or() { + check_doc_test( + "replace_or_else_with_or", + r#####" +//- minicore:option +fn foo() { + let a = Some(1); + a.unwra$0p_or_else(|| 2); +} +"#####, + r#####" +fn foo() { + let a = Some(1); + a.unwrap_or(2); +} +"#####, + ) +} + +#[test] +fn doctest_replace_or_with_or_else() { + check_doc_test( + "replace_or_with_or_else", + r#####" +//- minicore:option +fn foo() { + let a = Some(1); + a.unwra$0p_or(2); +} +"#####, + r#####" +fn foo() { + let a = Some(1); + a.unwrap_or_else(|| 2); +} +"#####, + ) +} + #[test] fn doctest_replace_qualified_name_with_use() { check_doc_test( From 42486b6e94a572b31acaf8246f1defb6f72437bb Mon Sep 17 00:00:00 2001 From: austaras Date: Sat, 27 Aug 2022 09:12:19 +0800 Subject: [PATCH 4/5] change as requested --- .../src/handlers/replace_or_with_or_else.rs | 117 +++++++++++++----- 1 file changed, 86 insertions(+), 31 deletions(-) diff --git a/crates/ide-assists/src/handlers/replace_or_with_or_else.rs b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs index bc1122a9d23..bee52a0d7fa 100644 --- a/crates/ide-assists/src/handlers/replace_or_with_or_else.rs +++ b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs @@ -30,33 +30,33 @@ use crate::{AssistContext, Assists}; pub(crate) fn replace_or_with_or_else(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { let call: ast::MethodCallExpr = ctx.find_node_at_offset()?; - is_option_or_result(call.receiver()?, ctx)?; + let kind = is_option_or_result(call.receiver()?, ctx)?; let (name, arg_list) = (call.name_ref()?, call.arg_list()?); + let mut map_or = false; + let replace = match &*name.text() { "unwrap_or" => "unwrap_or_else".to_string(), - "ok_or" => "ok_or_else".to_string(), + "or" => "or_else".to_string(), + "ok_or" if kind == Kind::Option => "ok_or_else".to_string(), + "map_or" => { + map_or = true; + "map_or_else".to_string() + } _ => return None, }; let arg = match arg_list.args().collect::>().as_slice() { [] => make::arg_list(Vec::new()), [first] => { - let param = (|| { - if let ast::Expr::CallExpr(call) = first { - if call.arg_list()?.args().count() == 0 { - Some(call.expr()?.clone()) - } else { - None - } - } else { - None - } - })() - .unwrap_or_else(|| make::expr_closure(None, first.clone())); + let param = into_closure(first); make::arg_list(vec![param]) } + [first, second] if map_or => { + let param = into_closure(first); + make::arg_list(vec![param, second.clone()]) + } _ => return None, }; @@ -71,6 +71,21 @@ pub(crate) fn replace_or_with_or_else(acc: &mut Assists, ctx: &AssistContext<'_> ) } +fn into_closure(param: &Expr) -> Expr { + (|| { + if let ast::Expr::CallExpr(call) = param { + if call.arg_list()?.args().count() == 0 { + Some(call.expr()?.clone()) + } else { + None + } + } else { + None + } + })() + .unwrap_or_else(|| make::expr_closure(None, param.clone())) +} + // Assist: replace_or_else_with_or // // Replace `unwrap_or_else` with `unwrap_or` and `ok_or_else` with `ok_or`. @@ -92,33 +107,32 @@ pub(crate) fn replace_or_with_or_else(acc: &mut Assists, ctx: &AssistContext<'_> pub(crate) fn replace_or_else_with_or(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> { let call: ast::MethodCallExpr = ctx.find_node_at_offset()?; - is_option_or_result(call.receiver()?, ctx)?; + let kind = is_option_or_result(call.receiver()?, ctx)?; let (name, arg_list) = (call.name_ref()?, call.arg_list()?); + let mut map_or = false; let replace = match &*name.text() { "unwrap_or_else" => "unwrap_or".to_string(), - "ok_or_else" => "ok_or".to_string(), + "or_else" => "or".to_string(), + "ok_or_else" if kind == Kind::Option => "ok_or".to_string(), + "map_or_else" => { + map_or = true; + "map_or".to_string() + } _ => return None, }; let arg = match arg_list.args().collect::>().as_slice() { [] => make::arg_list(Vec::new()), [first] => { - let param = (|| { - if let ast::Expr::ClosureExpr(closure) = first { - if closure.param_list()?.params().count() == 0 { - Some(closure.body()?.clone()) - } else { - None - } - } else { - None - } - })() - .unwrap_or_else(|| make::expr_call(first.clone(), make::arg_list(Vec::new()))); + let param = into_call(first); make::arg_list(vec![param]) } + [first, second] if map_or => { + let param = into_call(first); + make::arg_list(vec![param, second.clone()]) + } _ => return None, }; @@ -133,14 +147,35 @@ pub(crate) fn replace_or_else_with_or(acc: &mut Assists, ctx: &AssistContext<'_> ) } -fn is_option_or_result(receiver: Expr, ctx: &AssistContext<'_>) -> Option<()> { +fn into_call(param: &Expr) -> Expr { + (|| { + if let ast::Expr::ClosureExpr(closure) = param { + if closure.param_list()?.params().count() == 0 { + Some(closure.body()?.clone()) + } else { + None + } + } else { + None + } + })() + .unwrap_or_else(|| make::expr_call(param.clone(), make::arg_list(Vec::new()))) +} + +#[derive(PartialEq, Eq)] +enum Kind { + Option, + Result, +} + +fn is_option_or_result(receiver: Expr, ctx: &AssistContext<'_>) -> Option { let ty = ctx.sema.type_of_expr(&receiver)?.adjusted().as_adt()?.as_enum()?; let option_enum = FamousDefs(&ctx.sema, ctx.sema.scope(receiver.syntax())?.krate()).core_option_Option(); if let Some(option_enum) = option_enum { if ty == option_enum { - return Some(()); + return Some(Kind::Option); } } @@ -149,7 +184,7 @@ fn is_option_or_result(receiver: Expr, ctx: &AssistContext<'_>) -> Option<()> { if let Some(result_enum) = result_enum { if ty == result_enum { - return Some(()); + return Some(Kind::Result); } } @@ -294,6 +329,26 @@ fn foo() { ) } + #[test] + fn replace_or_else_with_or_map() { + check_assist( + replace_or_else_with_or, + r#" +//- minicore: result +fn foo() { + let foo = Ok("foo"); + return foo.map$0_or_else(|| 42, |v| v.len()); +} +"#, + r#" +fn foo() { + let foo = Ok("foo"); + return foo.map_or(42, |v| v.len()); +} +"#, + ) + } + #[test] fn replace_or_else_with_or_not_applicable() { check_assist_not_applicable( From 43e8d9644f9e55b677e226dbfa0255d9a8af1303 Mon Sep 17 00:00:00 2001 From: austaras Date: Wed, 31 Aug 2022 17:31:23 +0800 Subject: [PATCH 5/5] change title --- crates/ide-assists/src/handlers/replace_or_with_or_else.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ide-assists/src/handlers/replace_or_with_or_else.rs b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs index bee52a0d7fa..7d91be62101 100644 --- a/crates/ide-assists/src/handlers/replace_or_with_or_else.rs +++ b/crates/ide-assists/src/handlers/replace_or_with_or_else.rs @@ -62,7 +62,7 @@ pub(crate) fn replace_or_with_or_else(acc: &mut Assists, ctx: &AssistContext<'_> acc.add( AssistId("replace_or_with_or_else", AssistKind::RefactorRewrite), - "Replace unwrap_or or ok_or with lazy version", + format!("Replace {} with {}", name.text(), replace), call.syntax().text_range(), |builder| { builder.replace(name.syntax().text_range(), replace); @@ -138,7 +138,7 @@ pub(crate) fn replace_or_else_with_or(acc: &mut Assists, ctx: &AssistContext<'_> acc.add( AssistId("replace_or_else_with_or", AssistKind::RefactorRewrite), - "Replace unwrap_or_else or ok_or_else with eager version", + format!("Replace {} with {}", name.text(), replace), call.syntax().text_range(), |builder| { builder.replace(name.syntax().text_range(), replace);