diff options
author | bors[bot] <26634292+bors[bot]@users.noreply.github.com> | 2021-04-13 12:39:03 +0100 |
---|---|---|
committer | GitHub <[email protected]> | 2021-04-13 12:39:03 +0100 |
commit | e6728a8cd39f9111dcdd654c7c65e99e5a2f1190 (patch) | |
tree | 2532d24e0c10c7e3284033df4256f7adf96d2e82 | |
parent | 0c02208fd8d9ff3ce8184e2eb5dba915d5b54656 (diff) | |
parent | 09a78caca403f1b4be8711d00519094896928e58 (diff) |
Merge #8415
8415: Fix faulty assertion when extracting function with macro call r=matklad a=brandondong
**Reproduction:**
```rust
fn main() {
let n = 1;
let k = n * n;
dbg!(n);
}
```
1. Select the second and third lines of the main function. Use the "Extract into function" code assist.
2. Panic occurs in debug, error is logged in release: "[ERROR ide_assists::handlers::extract_function] assertion failed: matches!(path, ast :: Expr :: PathExpr(_))".
3. Function generates successfully on release where the panic was bypassed.
```rust
fn fun_name(n: i32) {
let k = n * n;
dbg!(n);
}
```
**Cause:**
- The generated function will take `n` as a parameter. The extraction logic needs to search the usages of `n` to determine whether it is used mutably or not. The helper `path_element_of_reference` is called for each usage but the second usage is a macro call and fails the `Expr::PathExpr(_)` match assertion.
- The caller of `path_element_of_reference` does implicitly assume it to be a `Expr::PathExpr(_)` in how it looks at its parent node for determining whether it is used mutably. This logic will not work for macros.
- I'm not sure if there are any other cases besides macros where it could be something other than a `Expr::PathExpr(_)`. I tried various examples and could not find any.
**Fix:**
- Update assertion to include the macro case.
- Add a FIXME to properly handle checking if a macro usage requires mutable access. For now, return false instead of running the existing logic that is tailored for `Expr::PathExpr(_)`'s.
Co-authored-by: Brandon <[email protected]>
-rw-r--r-- | crates/ide_assists/src/handlers/extract_function.rs | 48 |
1 files changed, 46 insertions, 2 deletions
diff --git a/crates/ide_assists/src/handlers/extract_function.rs b/crates/ide_assists/src/handlers/extract_function.rs index af9543766..059414274 100644 --- a/crates/ide_assists/src/handlers/extract_function.rs +++ b/crates/ide_assists/src/handlers/extract_function.rs | |||
@@ -736,6 +736,14 @@ fn reference_is_exclusive( | |||
736 | 736 | ||
737 | /// checks if this expr requires `&mut` access, recurses on field access | 737 | /// checks if this expr requires `&mut` access, recurses on field access |
738 | fn expr_require_exclusive_access(ctx: &AssistContext, expr: &ast::Expr) -> Option<bool> { | 738 | fn expr_require_exclusive_access(ctx: &AssistContext, expr: &ast::Expr) -> Option<bool> { |
739 | match expr { | ||
740 | ast::Expr::MacroCall(_) => { | ||
741 | // FIXME: expand macro and check output for mutable usages of the variable? | ||
742 | return None; | ||
743 | } | ||
744 | _ => (), | ||
745 | } | ||
746 | |||
739 | let parent = expr.syntax().parent()?; | 747 | let parent = expr.syntax().parent()?; |
740 | 748 | ||
741 | if let Some(bin_expr) = ast::BinExpr::cast(parent.clone()) { | 749 | if let Some(bin_expr) = ast::BinExpr::cast(parent.clone()) { |
@@ -794,7 +802,7 @@ impl HasTokenAtOffset for SyntaxNode { | |||
794 | } | 802 | } |
795 | } | 803 | } |
796 | 804 | ||
797 | /// find relevant `ast::PathExpr` for reference | 805 | /// find relevant `ast::Expr` for reference |
798 | /// | 806 | /// |
799 | /// # Preconditions | 807 | /// # Preconditions |
800 | /// | 808 | /// |
@@ -811,7 +819,11 @@ fn path_element_of_reference( | |||
811 | stdx::never!(false, "cannot find path parent of variable usage: {:?}", token); | 819 | stdx::never!(false, "cannot find path parent of variable usage: {:?}", token); |
812 | None | 820 | None |
813 | })?; | 821 | })?; |
814 | stdx::always!(matches!(path, ast::Expr::PathExpr(_))); | 822 | stdx::always!( |
823 | matches!(path, ast::Expr::PathExpr(_) | ast::Expr::MacroCall(_)), | ||
824 | "unexpected expression type for variable usage: {:?}", | ||
825 | path | ||
826 | ); | ||
815 | Some(path) | 827 | Some(path) |
816 | } | 828 | } |
817 | 829 | ||
@@ -3462,4 +3474,36 @@ fn foo() -> Result<(), i64> { | |||
3462 | }"##, | 3474 | }"##, |
3463 | ); | 3475 | ); |
3464 | } | 3476 | } |
3477 | |||
3478 | #[test] | ||
3479 | fn param_usage_in_macro() { | ||
3480 | check_assist( | ||
3481 | extract_function, | ||
3482 | r" | ||
3483 | macro_rules! m { | ||
3484 | ($val:expr) => { $val }; | ||
3485 | } | ||
3486 | |||
3487 | fn foo() { | ||
3488 | let n = 1; | ||
3489 | $0let k = n * m!(n);$0 | ||
3490 | let m = k + 1; | ||
3491 | }", | ||
3492 | r" | ||
3493 | macro_rules! m { | ||
3494 | ($val:expr) => { $val }; | ||
3495 | } | ||
3496 | |||
3497 | fn foo() { | ||
3498 | let n = 1; | ||
3499 | let k = fun_name(n); | ||
3500 | let m = k + 1; | ||
3501 | } | ||
3502 | |||
3503 | fn $0fun_name(n: i32) -> i32 { | ||
3504 | let k = n * m!(n); | ||
3505 | k | ||
3506 | }", | ||
3507 | ); | ||
3508 | } | ||
3465 | } | 3509 | } |