diff options
author | bors[bot] <26634292+bors[bot]@users.noreply.github.com> | 2021-04-10 22:35:05 +0100 |
---|---|---|
committer | GitHub <[email protected]> | 2021-04-10 22:35:05 +0100 |
commit | a8a25863f6e1e6da94b60813b2daee73b55132f7 (patch) | |
tree | 1e2b90d4196867eb1326d19bac70be8a5847a26e /crates/ide_assists/src/handlers/extract_function.rs | |
parent | bd675c8a8bdd3fda239bee3d3f31acd8679655b9 (diff) | |
parent | c989287a34b8b27fa4880d27e698bd880631a794 (diff) |
Merge #8436
8436: Fix extract function's mutability of variables outliving the body r=matklad a=brandondong
**Reproduction:**
```rust
fn main() {
let mut k = 1;
let mut j = 2;
j += 1;
k += j;
}
```
1. Select the first to third lines of the main function. Use the "Extract into function" code assist.
2. The output is the following which does not compile because the outlived variable `k` is declared as immutable:
```rust
fn main() {
let (k, j) = fun_name();
k += j;
}
fn fun_name() -> (i32, i32) {
let mut k = 1;
let mut j = 2;
j += 1;
(k, j)
}
```
3. We would instead expect the output to be:
```rust
fn main() {
let (mut k, j) = fun_name();
k += j;
}
```
**Fix:**
- Instead of declaring outlived variables as immutable unconditionally, check for any mutable usages outside of the extracted function.
Co-authored-by: Brandon <[email protected]>
Diffstat (limited to 'crates/ide_assists/src/handlers/extract_function.rs')
-rw-r--r-- | crates/ide_assists/src/handlers/extract_function.rs | 128 |
1 files changed, 109 insertions, 19 deletions
diff --git a/crates/ide_assists/src/handlers/extract_function.rs b/crates/ide_assists/src/handlers/extract_function.rs index 5fdc8bf38..af9543766 100644 --- a/crates/ide_assists/src/handlers/extract_function.rs +++ b/crates/ide_assists/src/handlers/extract_function.rs | |||
@@ -75,7 +75,8 @@ pub(crate) fn extract_function(acc: &mut Assists, ctx: &AssistContext) -> Option | |||
75 | let insert_after = scope_for_fn_insertion(&body, anchor)?; | 75 | let insert_after = scope_for_fn_insertion(&body, anchor)?; |
76 | let module = ctx.sema.scope(&insert_after).module()?; | 76 | let module = ctx.sema.scope(&insert_after).module()?; |
77 | 77 | ||
78 | let vars_defined_in_body_and_outlive = vars_defined_in_body_and_outlive(ctx, &body); | 78 | let vars_defined_in_body_and_outlive = |
79 | vars_defined_in_body_and_outlive(ctx, &body, &node.parent().as_ref().unwrap_or(&node)); | ||
79 | let ret_ty = body_return_ty(ctx, &body)?; | 80 | let ret_ty = body_return_ty(ctx, &body)?; |
80 | 81 | ||
81 | // FIXME: we compute variables that outlive here just to check `never!` condition | 82 | // FIXME: we compute variables that outlive here just to check `never!` condition |
@@ -257,7 +258,7 @@ struct Function { | |||
257 | control_flow: ControlFlow, | 258 | control_flow: ControlFlow, |
258 | ret_ty: RetType, | 259 | ret_ty: RetType, |
259 | body: FunctionBody, | 260 | body: FunctionBody, |
260 | vars_defined_in_body_and_outlive: Vec<Local>, | 261 | vars_defined_in_body_and_outlive: Vec<OutlivedLocal>, |
261 | } | 262 | } |
262 | 263 | ||
263 | #[derive(Debug)] | 264 | #[derive(Debug)] |
@@ -296,9 +297,9 @@ impl Function { | |||
296 | RetType::Expr(ty) => FunType::Single(ty.clone()), | 297 | RetType::Expr(ty) => FunType::Single(ty.clone()), |
297 | RetType::Stmt => match self.vars_defined_in_body_and_outlive.as_slice() { | 298 | RetType::Stmt => match self.vars_defined_in_body_and_outlive.as_slice() { |
298 | [] => FunType::Unit, | 299 | [] => FunType::Unit, |
299 | [var] => FunType::Single(var.ty(ctx.db())), | 300 | [var] => FunType::Single(var.local.ty(ctx.db())), |
300 | vars => { | 301 | vars => { |
301 | let types = vars.iter().map(|v| v.ty(ctx.db())).collect(); | 302 | let types = vars.iter().map(|v| v.local.ty(ctx.db())).collect(); |
302 | FunType::Tuple(types) | 303 | FunType::Tuple(types) |
303 | } | 304 | } |
304 | }, | 305 | }, |
@@ -562,6 +563,12 @@ impl HasTokenAtOffset for FunctionBody { | |||
562 | } | 563 | } |
563 | } | 564 | } |
564 | 565 | ||
566 | #[derive(Debug)] | ||
567 | struct OutlivedLocal { | ||
568 | local: Local, | ||
569 | mut_usage_outside_body: bool, | ||
570 | } | ||
571 | |||
565 | /// Try to guess what user wants to extract | 572 | /// Try to guess what user wants to extract |
566 | /// | 573 | /// |
567 | /// We have basically have two cases: | 574 | /// We have basically have two cases: |
@@ -707,10 +714,10 @@ fn has_exclusive_usages(ctx: &AssistContext, usages: &LocalUsages, body: &Functi | |||
707 | .any(|reference| reference_is_exclusive(reference, body, ctx)) | 714 | .any(|reference| reference_is_exclusive(reference, body, ctx)) |
708 | } | 715 | } |
709 | 716 | ||
710 | /// checks if this reference requires `&mut` access inside body | 717 | /// checks if this reference requires `&mut` access inside node |
711 | fn reference_is_exclusive( | 718 | fn reference_is_exclusive( |
712 | reference: &FileReference, | 719 | reference: &FileReference, |
713 | body: &FunctionBody, | 720 | node: &dyn HasTokenAtOffset, |
714 | ctx: &AssistContext, | 721 | ctx: &AssistContext, |
715 | ) -> bool { | 722 | ) -> bool { |
716 | // we directly modify variable with set: `n = 0`, `n += 1` | 723 | // we directly modify variable with set: `n = 0`, `n += 1` |
@@ -719,7 +726,7 @@ fn reference_is_exclusive( | |||
719 | } | 726 | } |
720 | 727 | ||
721 | // we take `&mut` reference to variable: `&mut v` | 728 | // we take `&mut` reference to variable: `&mut v` |
722 | let path = match path_element_of_reference(body, reference) { | 729 | let path = match path_element_of_reference(node, reference) { |
723 | Some(path) => path, | 730 | Some(path) => path, |
724 | None => return false, | 731 | None => return false, |
725 | }; | 732 | }; |
@@ -820,10 +827,16 @@ fn vars_defined_in_body(body: &FunctionBody, ctx: &AssistContext) -> Vec<Local> | |||
820 | } | 827 | } |
821 | 828 | ||
822 | /// list local variables defined inside `body` that should be returned from extracted function | 829 | /// list local variables defined inside `body` that should be returned from extracted function |
823 | fn vars_defined_in_body_and_outlive(ctx: &AssistContext, body: &FunctionBody) -> Vec<Local> { | 830 | fn vars_defined_in_body_and_outlive( |
824 | let mut vars_defined_in_body = vars_defined_in_body(&body, ctx); | 831 | ctx: &AssistContext, |
825 | vars_defined_in_body.retain(|var| var_outlives_body(ctx, body, var)); | 832 | body: &FunctionBody, |
833 | parent: &SyntaxNode, | ||
834 | ) -> Vec<OutlivedLocal> { | ||
835 | let vars_defined_in_body = vars_defined_in_body(&body, ctx); | ||
826 | vars_defined_in_body | 836 | vars_defined_in_body |
837 | .into_iter() | ||
838 | .filter_map(|var| var_outlives_body(ctx, body, var, parent)) | ||
839 | .collect() | ||
827 | } | 840 | } |
828 | 841 | ||
829 | /// checks if the relevant local was defined before(outside of) body | 842 | /// checks if the relevant local was defined before(outside of) body |
@@ -843,11 +856,23 @@ fn either_syntax(value: &Either<ast::IdentPat, ast::SelfParam>) -> &SyntaxNode { | |||
843 | } | 856 | } |
844 | } | 857 | } |
845 | 858 | ||
846 | /// checks if local variable is used after(outside of) body | 859 | /// returns usage details if local variable is used after(outside of) body |
847 | fn var_outlives_body(ctx: &AssistContext, body: &FunctionBody, var: &Local) -> bool { | 860 | fn var_outlives_body( |
848 | let usages = LocalUsages::find(ctx, *var); | 861 | ctx: &AssistContext, |
862 | body: &FunctionBody, | ||
863 | var: Local, | ||
864 | parent: &SyntaxNode, | ||
865 | ) -> Option<OutlivedLocal> { | ||
866 | let usages = LocalUsages::find(ctx, var); | ||
849 | let has_usages = usages.iter().any(|reference| body.preceedes_range(reference.range)); | 867 | let has_usages = usages.iter().any(|reference| body.preceedes_range(reference.range)); |
850 | has_usages | 868 | if !has_usages { |
869 | return None; | ||
870 | } | ||
871 | let has_mut_usages = usages | ||
872 | .iter() | ||
873 | .filter(|reference| body.preceedes_range(reference.range)) | ||
874 | .any(|reference| reference_is_exclusive(reference, parent, ctx)); | ||
875 | Some(OutlivedLocal { local: var, mut_usage_outside_body: has_mut_usages }) | ||
851 | } | 876 | } |
852 | 877 | ||
853 | fn body_return_ty(ctx: &AssistContext, body: &FunctionBody) -> Option<RetType> { | 878 | fn body_return_ty(ctx: &AssistContext, body: &FunctionBody) -> Option<RetType> { |
@@ -927,16 +952,25 @@ fn format_replacement(ctx: &AssistContext, fun: &Function, indent: IndentLevel) | |||
927 | let mut buf = String::new(); | 952 | let mut buf = String::new(); |
928 | match fun.vars_defined_in_body_and_outlive.as_slice() { | 953 | match fun.vars_defined_in_body_and_outlive.as_slice() { |
929 | [] => {} | 954 | [] => {} |
930 | [var] => format_to!(buf, "let {} = ", var.name(ctx.db()).unwrap()), | 955 | [var] => { |
956 | format_to!(buf, "let {}{} = ", mut_modifier(var), var.local.name(ctx.db()).unwrap()) | ||
957 | } | ||
931 | [v0, vs @ ..] => { | 958 | [v0, vs @ ..] => { |
932 | buf.push_str("let ("); | 959 | buf.push_str("let ("); |
933 | format_to!(buf, "{}", v0.name(ctx.db()).unwrap()); | 960 | format_to!(buf, "{}{}", mut_modifier(v0), v0.local.name(ctx.db()).unwrap()); |
934 | for var in vs { | 961 | for var in vs { |
935 | format_to!(buf, ", {}", var.name(ctx.db()).unwrap()); | 962 | format_to!(buf, ", {}{}", mut_modifier(var), var.local.name(ctx.db()).unwrap()); |
936 | } | 963 | } |
937 | buf.push_str(") = "); | 964 | buf.push_str(") = "); |
938 | } | 965 | } |
939 | } | 966 | } |
967 | fn mut_modifier(var: &OutlivedLocal) -> &'static str { | ||
968 | if var.mut_usage_outside_body { | ||
969 | "mut " | ||
970 | } else { | ||
971 | "" | ||
972 | } | ||
973 | } | ||
940 | format_to!(buf, "{}", expr); | 974 | format_to!(buf, "{}", expr); |
941 | if fun.ret_ty.is_unit() | 975 | if fun.ret_ty.is_unit() |
942 | && (!fun.vars_defined_in_body_and_outlive.is_empty() || !expr.is_block_like()) | 976 | && (!fun.vars_defined_in_body_and_outlive.is_empty() || !expr.is_block_like()) |
@@ -1199,10 +1233,10 @@ fn make_body( | |||
1199 | match fun.vars_defined_in_body_and_outlive.as_slice() { | 1233 | match fun.vars_defined_in_body_and_outlive.as_slice() { |
1200 | [] => {} | 1234 | [] => {} |
1201 | [var] => { | 1235 | [var] => { |
1202 | tail_expr = Some(path_expr_from_local(ctx, *var)); | 1236 | tail_expr = Some(path_expr_from_local(ctx, var.local)); |
1203 | } | 1237 | } |
1204 | vars => { | 1238 | vars => { |
1205 | let exprs = vars.iter().map(|var| path_expr_from_local(ctx, *var)); | 1239 | let exprs = vars.iter().map(|var| path_expr_from_local(ctx, var.local)); |
1206 | let expr = make::expr_tuple(exprs); | 1240 | let expr = make::expr_tuple(exprs); |
1207 | tail_expr = Some(expr); | 1241 | tail_expr = Some(expr); |
1208 | } | 1242 | } |
@@ -2111,6 +2145,30 @@ fn $0fun_name(n: i32) -> i32 { | |||
2111 | } | 2145 | } |
2112 | 2146 | ||
2113 | #[test] | 2147 | #[test] |
2148 | fn variable_defined_inside_and_used_after_mutably_no_ret() { | ||
2149 | check_assist( | ||
2150 | extract_function, | ||
2151 | r" | ||
2152 | fn foo() { | ||
2153 | let n = 1; | ||
2154 | $0let mut k = n * n;$0 | ||
2155 | k += 1; | ||
2156 | }", | ||
2157 | r" | ||
2158 | fn foo() { | ||
2159 | let n = 1; | ||
2160 | let mut k = fun_name(n); | ||
2161 | k += 1; | ||
2162 | } | ||
2163 | |||
2164 | fn $0fun_name(n: i32) -> i32 { | ||
2165 | let mut k = n * n; | ||
2166 | k | ||
2167 | }", | ||
2168 | ); | ||
2169 | } | ||
2170 | |||
2171 | #[test] | ||
2114 | fn two_variables_defined_inside_and_used_after_no_ret() { | 2172 | fn two_variables_defined_inside_and_used_after_no_ret() { |
2115 | check_assist( | 2173 | check_assist( |
2116 | extract_function, | 2174 | extract_function, |
@@ -2137,6 +2195,38 @@ fn $0fun_name(n: i32) -> (i32, i32) { | |||
2137 | } | 2195 | } |
2138 | 2196 | ||
2139 | #[test] | 2197 | #[test] |
2198 | fn multi_variables_defined_inside_and_used_after_mutably_no_ret() { | ||
2199 | check_assist( | ||
2200 | extract_function, | ||
2201 | r" | ||
2202 | fn foo() { | ||
2203 | let n = 1; | ||
2204 | $0let mut k = n * n; | ||
2205 | let mut m = k + 2; | ||
2206 | let mut o = m + 3; | ||
2207 | o += 1;$0 | ||
2208 | k += o; | ||
2209 | m = 1; | ||
2210 | }", | ||
2211 | r" | ||
2212 | fn foo() { | ||
2213 | let n = 1; | ||
2214 | let (mut k, mut m, o) = fun_name(n); | ||
2215 | k += o; | ||
2216 | m = 1; | ||
2217 | } | ||
2218 | |||
2219 | fn $0fun_name(n: i32) -> (i32, i32, i32) { | ||
2220 | let mut k = n * n; | ||
2221 | let mut m = k + 2; | ||
2222 | let mut o = m + 3; | ||
2223 | o += 1; | ||
2224 | (k, m, o) | ||
2225 | }", | ||
2226 | ); | ||
2227 | } | ||
2228 | |||
2229 | #[test] | ||
2140 | fn nontrivial_patterns_define_variables() { | 2230 | fn nontrivial_patterns_define_variables() { |
2141 | check_assist( | 2231 | check_assist( |
2142 | extract_function, | 2232 | extract_function, |