Unnamed repository; edit this file 'description' to name the repository.
Merge pull request #22674 from Albab-Hasan/fix-assists-unit-return-identifier-panics
fix: panics in `unwrap_return_type`, `remove_underscore`, and `promote_local_to_const`
3 files changed, 167 insertions, 15 deletions
diff --git a/crates/ide-assists/src/handlers/promote_local_to_const.rs b/crates/ide-assists/src/handlers/promote_local_to_const.rs index 23c88e65d6..5dcda1deb0 100644 --- a/crates/ide-assists/src/handlers/promote_local_to_const.rs +++ b/crates/ide-assists/src/handlers/promote_local_to_const.rs @@ -63,6 +63,11 @@ pub(crate) fn promote_local_to_const(acc: &mut Assists, ctx: &AssistContext<'_, return None; } + let const_name = to_upper_snake_case(&name.to_string()); + if const_name.is_empty() || const_name.chars().all(|c| c == '_') { + return None; + } + acc.add( AssistId::refactor("promote_local_to_const"), "Promote local to constant", @@ -70,15 +75,14 @@ pub(crate) fn promote_local_to_const(acc: &mut Assists, ctx: &AssistContext<'_, |edit| { let editor = edit.make_editor(let_stmt.syntax()); let make = editor.make(); - let name = to_upper_snake_case(&name.to_string()); let usages = Definition::Local(local).usages(&ctx.sema).all(); if let Some(usages) = usages.references.get(&ctx.file_id()) { - let name_ref = make.name_ref(&name); + let name_ref = make.name_ref(&const_name); for usage in usages { let Some(usage_name) = usage.name.as_name_ref().cloned() else { continue }; if let Some(record_field) = ast::RecordExprField::for_name_ref(&usage_name) { - let path = make.ident_path(&name); + let path = make.ident_path(&const_name); let name_expr = make.expr_path(path); utils::replace_record_field_expr(ctx, edit, record_field, name_expr); } else { @@ -88,7 +92,8 @@ pub(crate) fn promote_local_to_const(acc: &mut Assists, ctx: &AssistContext<'_, } } - let item = make.item_const(None, None, make.name(&name), make.ty(&ty), initializer); + let item = + make.item_const(None, None, make.name(&const_name), make.ty(&ty), initializer); if let Some((cap, name)) = ctx.config.snippet_cap.zip(item.name()) { let tabstop = edit.make_tabstop_before(cap); @@ -293,6 +298,19 @@ fn foo() { } #[test] + fn not_applicable_when_name_converts_to_all_underscores() { + check_assist_not_applicable( + promote_local_to_const, + r" +fn foo() { + let _$0_ = 0; + __; +} +", + ); + } + + #[test] fn not_applicable_non_simple_ident() { cov_mark::check!(promote_local_non_simple_ident); check_assist_not_applicable( diff --git a/crates/ide-assists/src/handlers/remove_underscore.rs b/crates/ide-assists/src/handlers/remove_underscore.rs index efe5c945be..1ccdcda52d 100644 --- a/crates/ide-assists/src/handlers/remove_underscore.rs +++ b/crates/ide-assists/src/handlers/remove_underscore.rs @@ -57,14 +57,17 @@ pub(crate) fn remove_underscore(acc: &mut Assists, ctx: &AssistContext<'_, '_>) } let new_name = text.trim_start_matches('_'); + if new_name.is_empty() { + return None; + } + let changes = + def.rename(&ctx.sema, new_name, RenameDefinition::Yes, &ctx.config.rename_config()).ok()?; + acc.add( AssistId::refactor("remove_underscore_from_used_variables"), "Remove underscore from a used variable", text_range, |builder| { - let changes = def - .rename(&ctx.sema, new_name, RenameDefinition::Yes, &ctx.config.rename_config()) - .unwrap(); builder.source_change = changes; }, ) @@ -176,6 +179,32 @@ fn foo(foo: i32) { } #[test] + fn not_applicable_when_result_is_keyword() { + check_assist_not_applicable( + remove_underscore, + r#" +fn main() { + let _$0crate = 1; + _crate; +} +"#, + ); + } + + #[test] + fn not_applicable_when_result_is_empty() { + check_assist_not_applicable( + remove_underscore, + r#" +fn main() { + let _$0_ = 1; + __; +} +"#, + ); + } + + #[test] fn remove_underscore_in_function_parameter() { check_assist( remove_underscore, diff --git a/crates/ide-assists/src/handlers/unwrap_return_type.rs b/crates/ide-assists/src/handlers/unwrap_return_type.rs index 608c68ea85..e53b73bad6 100644 --- a/crates/ide-assists/src/handlers/unwrap_return_type.rs +++ b/crates/ide-assists/src/handlers/unwrap_return_type.rs @@ -113,16 +113,21 @@ pub(crate) fn unwrap_return_type(acc: &mut Assists, ctx: &AssistContext<'_, '_>) let arg_list = call_expr.arg_list().unwrap(); if is_unit_type { - let tail_parent = tail_expr - .syntax() - .parent() - .and_then(Either::<ast::ReturnExpr, ast::StmtList>::cast) - .unwrap(); + let tail_parent = tail_expr.syntax().parent().and_then( + Either::<Either<ast::ReturnExpr, ast::BreakExpr>, ast::StmtList>::cast, + ); match tail_parent { - Either::Left(ret_expr) => { - editor.replace(ret_expr.syntax(), make.expr_return(None).syntax()) + Some(Either::Left(_expr)) => { + if let Some(ws) = tail_expr + .syntax() + .prev_sibling_or_token() + .filter(|e| e.kind() == SyntaxKind::WHITESPACE) + { + editor.delete(ws); + } + editor.delete(tail_expr.syntax()); } - Either::Right(stmt_list) => { + Some(Either::Right(stmt_list)) => { let new_block = if stmt_list.statements().next().is_none() { make.expr_empty_block() } else { @@ -133,6 +138,10 @@ pub(crate) fn unwrap_return_type(acc: &mut Assists, ctx: &AssistContext<'_, '_>) new_block.stmt_list().unwrap().syntax(), ); } + // Parent is a match arm or similar — replace with () + None => { + editor.replace(tail_expr.syntax(), make.expr_unit().syntax()); + } } } else if let Some(first_arg) = arg_list.args().next() { editor.replace(tail_expr.syntax(), first_arg.syntax()); @@ -2246,6 +2255,102 @@ fn foo(the_field: u32) -> u32 { } #[test] + fn unwrap_option_return_type_unit_type_match_arm() { + check_assist_by_label( + unwrap_return_type, + r#" +//- minicore: option +fn foo(flag: bool) -> Option<()$0> { + match flag { + true => Some(()), + false => None, + } +} +"#, + r#" +fn foo(flag: bool) { + match flag { + true => (), + false => ${1:()}$0, + } +} +"#, + "Unwrap Option return type", + ); + } + + #[test] + fn unwrap_option_return_type_unit_type_break() { + check_assist_by_label( + unwrap_return_type, + r#" +//- minicore: option +fn foo() -> Option<()$0> { + loop { + break Some(()); + } +} +"#, + r#" +fn foo() { + loop { + break; + } +} +"#, + "Unwrap Option return type", + ); + } + + #[test] + fn unwrap_option_return_type_unit_type_break_with_label() { + check_assist_by_label( + unwrap_return_type, + r#" +//- minicore: option +fn foo() -> Option<()$0> { + 'outer: loop { + break 'outer Some(()); + } +} +"#, + r#" +fn foo() { + 'outer: loop { + break 'outer; + } +} +"#, + "Unwrap Option return type", + ); + } + + #[test] + fn unwrap_result_return_type_unit_type_match_arm() { + check_assist_by_label( + unwrap_return_type, + r#" +//- minicore: result +fn foo(flag: bool) -> Result<(), ()$0> { + match flag { + true => Ok(()), + false => Err(()), + } +} +"#, + r#" +fn foo(flag: bool) { + match flag { + true => (), + false => (), + } +} +"#, + "Unwrap Result return type", + ); + } + + #[test] fn unwrap_result_return_type_nested_type() { check_assist_by_label( unwrap_return_type, |