11184: Correctly pass through mutable parameter references when extracting a function r=Veykril a=Vannevelj
Fixes https://github.com/rust-analyzer/rust-analyzer/issues/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:
7409880a07/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:
7409880a07/crates/ide_assists/src/handlers/extract_function.rs (L374-L381)
and then here to determine what transformation is needed for the calling argument:
7409880a07/crates/ide_assists/src/handlers/extract_function.rs (L383-L390)
which then gets transformed here:
7409880a07/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 <jer_vannevel@outlook.com>
dbb1c1b4b1