Skip to content

Commit 3b1b58c

Browse files
committed
Auto merge of #15662 - rmehri01:fix_panic_with_return_in_match, r=Veykril
fix: panic with wrapping/unwrapping result return type assists With the `wrap_return_type_in_result` assist, the following code results in a panic (note the lack of a semicolon): ```rust fn foo(num: i32) -> $0i32 { return num } => thread 'handlers::wrap_return_type_in_result::tests::wrap_return_in_tail_position' panicked at crates/syntax/src/ted.rs:137:41: called `Option::unwrap()` on a `None` value ``` I think this is because it first walks the body expression to change any `return` expressions and then walks all tail expressions, resulting in the `return num` being changed twice since it is both a `return` and in tail position. This can also happen when a `match` is in tail position and `return` is used in a branch for example. Not really sure how big of an issue this is in practice though since this seems to be the only case that is impacted and can be reduced to just `num` instead of `return num`. This also occurs with the `unwrap_result_return_type` assist but panics with the following instead: ``` thread 'handlers::unwrap_result_return_type::tests::wrap_return_in_tail_position' panicked at /rustc/3223b0b5e8dadda3f76c3fd1a8d6c5addc09599e/library/alloc/src/string.rs:1766:29: assertion failed: self.is_char_boundary(n) ```
2 parents c945f90 + 7306504 commit 3b1b58c

File tree

2 files changed

+40
-8
lines changed

2 files changed

+40
-8
lines changed

crates/ide-assists/src/handlers/unwrap_result_return_type.rs

+20-4
Original file line numberDiff line numberDiff line change
@@ -123,10 +123,8 @@ fn tail_cb_impl(acc: &mut Vec<ast::Expr>, e: &ast::Expr) {
123123
for_each_tail_expr(&break_expr_arg, &mut |e| tail_cb_impl(acc, e))
124124
}
125125
}
126-
Expr::ReturnExpr(ret_expr) => {
127-
if let Some(ret_expr_arg) = &ret_expr.expr() {
128-
for_each_tail_expr(ret_expr_arg, &mut |e| tail_cb_impl(acc, e));
129-
}
126+
Expr::ReturnExpr(_) => {
127+
// all return expressions have already been handled by the walk loop
130128
}
131129
e => acc.push(e.clone()),
132130
}
@@ -800,6 +798,24 @@ fn foo() -> i32 {
800798
);
801799
}
802800

801+
#[test]
802+
fn wrap_return_in_tail_position() {
803+
check_assist(
804+
unwrap_result_return_type,
805+
r#"
806+
//- minicore: result
807+
fn foo(num: i32) -> $0Result<i32, String> {
808+
return Ok(num)
809+
}
810+
"#,
811+
r#"
812+
fn foo(num: i32) -> i32 {
813+
return num
814+
}
815+
"#,
816+
);
817+
}
818+
803819
#[test]
804820
fn unwrap_result_return_type_simple_with_closure() {
805821
check_assist(

crates/ide-assists/src/handlers/wrap_return_type_in_result.rs

+20-4
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,8 @@ fn tail_cb_impl(acc: &mut Vec<ast::Expr>, e: &ast::Expr) {
9898
for_each_tail_expr(&break_expr_arg, &mut |e| tail_cb_impl(acc, e))
9999
}
100100
}
101-
Expr::ReturnExpr(ret_expr) => {
102-
if let Some(ret_expr_arg) = &ret_expr.expr() {
103-
for_each_tail_expr(ret_expr_arg, &mut |e| tail_cb_impl(acc, e));
104-
}
101+
Expr::ReturnExpr(_) => {
102+
// all return expressions have already been handled by the walk loop
105103
}
106104
e => acc.push(e.clone()),
107105
}
@@ -732,6 +730,24 @@ fn foo() -> Result<i32, ${0:_}> {
732730
);
733731
}
734732

733+
#[test]
734+
fn wrap_return_in_tail_position() {
735+
check_assist(
736+
wrap_return_type_in_result,
737+
r#"
738+
//- minicore: result
739+
fn foo(num: i32) -> $0i32 {
740+
return num
741+
}
742+
"#,
743+
r#"
744+
fn foo(num: i32) -> Result<i32, ${0:_}> {
745+
return Ok(num)
746+
}
747+
"#,
748+
);
749+
}
750+
735751
#[test]
736752
fn wrap_return_type_in_result_simple_with_closure() {
737753
check_assist(

0 commit comments

Comments
 (0)