Merge #11184

11184: Correctly pass through mutable parameter references when extracting a function r=Veykril a=Vannevelj

Fixes #10277

I have based this investigation based on my understanding of [the Borrowing chapter](https://doc.rust-lang.org/book/ch04-02-references-and-borrowing.html) but I wasn't able to debug the test runs or see it in action in an IDE. I'll try to figure out how to do that for future PRs but for now, the tests seem to confirm my understanding. I'll lay out my hypothesis below.

Here we define the parameters for the to-be-generated function: 

https://github.com/rust-analyzer/rust-analyzer/blob/7409880a07803c34590ad162d7854061145c6eae/crates/ide_assists/src/handlers/extract_function.rs#L882

Three values in particular are important here: `requires_mut`, `is_copy` and `move_local`. These will in turn be used here to determine the kind of parameter:

https://github.com/rust-analyzer/rust-analyzer/blob/7409880a07803c34590ad162d7854061145c6eae/crates/ide_assists/src/handlers/extract_function.rs#L374-L381

and then here to determine what transformation is needed for the calling argument:

https://github.com/rust-analyzer/rust-analyzer/blob/7409880a07803c34590ad162d7854061145c6eae/crates/ide_assists/src/handlers/extract_function.rs#L383-L390

which then gets transformed here:

https://github.com/rust-analyzer/rust-analyzer/blob/7409880a07803c34590ad162d7854061145c6eae/crates/syntax/src/ast/make.rs#L381-L383

What I believe is happening is that 
* `requires_mut` is `false` (it already is marked as mutable), 
* `is_copy` is `false` (`Foo` does not implement `Copy`), and 
* `move_local` is `false` (it has further usages)

According to the pattern matching in `fn kind()`, that would lead to `ParamKind::SharedRef` which in turn applies a transformation that prepends `&`.

However if I look at the chapter on borrowing then we only need to mark an argument as a reference if we actually own it. In this case the value is passed through as a reference parameter into the current function which means we never had ownership in the first place. By including the additional check for a reference parameter, `move_local` now becomes `true` and the resulting parameter is now `ParamKind::Value` which will avoid applying any transformations. This was further obscured by the fact that you need further usages of the variable or `move_local` would be considered `true` after all.

I didn't follow it in depth but it appears this idea applies for both the generated argument and the generated parameter.
There are existing tests that account for `&mut` values but they refer to local variables for which we do have ownership and as such they didn't expose this issue.

Co-authored-by: Jeroen Vannevel <[email protected]>

Author: bors[bot]

crates/ide_assists/src/handlers/extract_function.rs

@@ -878,7 +878,7 @@ impl FunctionBody {
                 // We can move the value into the function call if it's not used after the call,
                 // if the var is not used but defined outside a loop we are extracting from we can't move it either
                 // as the function will reuse it in the next iteration.
-                let move_local = !has_usages && defined_outside_parent_loop;
+                let move_local = (!has_usages && defined_outside_parent_loop) || ty.is_reference();
                 Param { var, ty, move_local, requires_mut, is_copy }
             })
             .collect()
@@ -4332,6 +4332,68 @@ fn foo() {
 fn $0fun_name(a: _) -> _ {
     a
 }
+"#,
+        );
+    }
+
+    #[test]
+    fn reference_mutable_param_with_further_usages() {
+        check_assist(
+            extract_function,
+            r#"
+pub struct Foo {
+    field: u32,
+}
+
+pub fn testfn(arg: &mut Foo) {
+    $0arg.field = 8;$0
+    // Simulating access after the extracted portion
+    arg.field = 16;
+}
+"#,
+            r#"
+pub struct Foo {
+    field: u32,
+}
+
+pub fn testfn(arg: &mut Foo) {
+    fun_name(arg);
+    // Simulating access after the extracted portion
+    arg.field = 16;
+}
+
+fn $0fun_name(arg: &mut Foo) {
+    arg.field = 8;
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn reference_mutable_param_without_further_usages() {
+        check_assist(
+            extract_function,
+            r#"
+pub struct Foo {
+    field: u32,
+}
+
+pub fn testfn(arg: &mut Foo) {
+    $0arg.field = 8;$0
+}
+"#,
+            r#"
+pub struct Foo {
+    field: u32,
+}
+
+pub fn testfn(arg: &mut Foo) {
+    fun_name(arg);
+}
+
+fn $0fun_name(arg: &mut Foo) {
+    arg.field = 8;
+}
 "#,
         );
     }