Skip to content

Commit

Permalink
Auto merge of #15662 - rmehri01:fix_panic_with_return_in_match, r=Vey…
Browse files Browse the repository at this point in the history
…kril

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)
```
  • Loading branch information
bors committed Sep 26, 2023
2 parents c945f90 + 7306504 commit 3b1b58c
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 8 deletions.
24 changes: 20 additions & 4 deletions crates/ide-assists/src/handlers/unwrap_result_return_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,8 @@ fn tail_cb_impl(acc: &mut Vec<ast::Expr>, e: &ast::Expr) {
for_each_tail_expr(&break_expr_arg, &mut |e| tail_cb_impl(acc, e))
}
}
Expr::ReturnExpr(ret_expr) => {
if let Some(ret_expr_arg) = &ret_expr.expr() {
for_each_tail_expr(ret_expr_arg, &mut |e| tail_cb_impl(acc, e));
}
Expr::ReturnExpr(_) => {
// all return expressions have already been handled by the walk loop
}
e => acc.push(e.clone()),
}
Expand Down Expand Up @@ -800,6 +798,24 @@ fn foo() -> i32 {
);
}

#[test]
fn wrap_return_in_tail_position() {
check_assist(
unwrap_result_return_type,
r#"
//- minicore: result
fn foo(num: i32) -> $0Result<i32, String> {
return Ok(num)
}
"#,
r#"
fn foo(num: i32) -> i32 {
return num
}
"#,
);
}

#[test]
fn unwrap_result_return_type_simple_with_closure() {
check_assist(
Expand Down
24 changes: 20 additions & 4 deletions crates/ide-assists/src/handlers/wrap_return_type_in_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,8 @@ fn tail_cb_impl(acc: &mut Vec<ast::Expr>, e: &ast::Expr) {
for_each_tail_expr(&break_expr_arg, &mut |e| tail_cb_impl(acc, e))
}
}
Expr::ReturnExpr(ret_expr) => {
if let Some(ret_expr_arg) = &ret_expr.expr() {
for_each_tail_expr(ret_expr_arg, &mut |e| tail_cb_impl(acc, e));
}
Expr::ReturnExpr(_) => {
// all return expressions have already been handled by the walk loop
}
e => acc.push(e.clone()),
}
Expand Down Expand Up @@ -732,6 +730,24 @@ fn foo() -> Result<i32, ${0:_}> {
);
}

#[test]
fn wrap_return_in_tail_position() {
check_assist(
wrap_return_type_in_result,
r#"
//- minicore: result
fn foo(num: i32) -> $0i32 {
return num
}
"#,
r#"
fn foo(num: i32) -> Result<i32, ${0:_}> {
return Ok(num)
}
"#,
);
}

#[test]
fn wrap_return_type_in_result_simple_with_closure() {
check_assist(
Expand Down

0 comments on commit 3b1b58c

Please sign in to comment.