Rollup merge of #132172 - dianne:suggest-borrow-generic, r=matthewjasper

borrowck diagnostics: suggest borrowing function inputs in generic positions # Summary This generalizes borrowck's existing suggestions to borrow instead of moving when passing by-value to a function that's generic in that input. Previously, this was special-cased to `AsRef`/`Borrow`-like traits and `Fn`-like traits. This PR changes it to test if, for a moved place with type `T`, that the callee's signature and clauses don't break if you substitute in `&T` or `&mut T`. For instance, it now works with `Read`/`Write`-like traits. Fixes #131413 # Incidental changes - No longer spuriously suggests mutable borrows of closures in some situations (see e.g. the tests in [tests/ui/closures/2229_closure_analysis/](https://github.com/rust-lang/rust/compare/master...dianne:rust:suggest-borrow-generic?expand=1#diff-8dfb200c559f0995d0f2ffa2f23bc6f8041b263e264e5c329a1f4171769787c0)). - No longer suggests cloning closures that implement `Fn`, since they can be borrowed (see e.g. [tests/ui/moves/borrow-closures-instead-of-move.stderr](https://github.com/rust-lang/rust/compare/master...dianne:rust:suggest-borrow-generic?expand=1#diff-5db268aac405eec56d099a72d8b58ac46dab523cf013e29008104840168577fb)). This keeps the behavior to suppress suggestions of `fn_once.clone()()`. I think it might make sense to suggest it with a "but this might not be your desired behavior" caveat, as is done when something is used after being consumed as the receiver for a method call. That's probably out of the scope of this PR though. # Limitations and possible improvements - This doesn't work for receivers of method calls. This is a small change, and I have it implemented locally, but I'm not sure it's useful on its own. In most cases I've found borrowing the receiver would change the call's output type (see below). In other cases (e.g. `Iterator::sum`), borrowing the receiver isn't useful since it's consumed. - This doesn't work when it would change the call's output type. In general, I figure inserting references into the output type is an unwanted change. However, this also means it doesn't work in cases where the new output type would be effectively the same as the old one. For example, from the rand crate, the iterator returned by [`Rng::sample_iter`](https://docs.rs/rand/latest/rand/trait.Rng.html#method.sample_iter) is effectively the same (modulo regions) whether you borrow or consume the receiver `Rng`, so common usage involves borrowing it. I'm not sure whether the best approach is to add a more complex check of approximate equivalence, to forego checking the call's output type and give spurious suggestions, or to leave it as-is. - This doesn't work when it would change the call's other input types. Instead, it could suggest borrowing any others that have the same parameter type (but only when suggesting shared borrows). I think this would be a pretty easy change, but I don't think it's very useful so long as the normalized output type can't change. I'm happy to implement any of these (or other potential improvements to this), but I'm not sure which are common enough patterns to justify the added complexity. Please let me know if any sound worthwhile.
rust-lang · Nov 14, 2024 · 5ee347e · 5ee347e
2 parents a4cedec + 2ab8480
commit 5ee347e
Show file tree

Hide file tree

Showing 14 changed files with 396 additions and 233 deletions.
diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
diff --git a/...losures/2229_closure_analysis/diagnostics/closure-origin-multi-variant-diagnostics.stderr b/...losures/2229_closure_analysis/diagnostics/closure-origin-multi-variant-diagnostics.stderr
@@ -11,10 +11,6 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         if let MultiVariant::Point(ref mut x, _) = point {
    |                                                    ^^^^^
-help: consider mutably borrowing `c`
-   |
-LL |     let a = &mut c;
-   |             ++++
 
 error: aborting due to 1 previous error
 

diff --git a/...osures/2229_closure_analysis/diagnostics/closure-origin-single-variant-diagnostics.stderr b/...osures/2229_closure_analysis/diagnostics/closure-origin-single-variant-diagnostics.stderr
@@ -11,10 +11,6 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         let SingleVariant::Point(ref mut x, _) = point;
    |                                                  ^^^^^
-help: consider mutably borrowing `c`
-   |
-LL |     let b = &mut c;
-   |             ++++
 
 error: aborting due to 1 previous error
 

diff --git a/tests/ui/closures/2229_closure_analysis/diagnostics/closure-origin-struct-diagnostics.stderr b/tests/ui/closures/2229_closure_analysis/diagnostics/closure-origin-struct-diagnostics.stderr
@@ -11,10 +11,6 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         x.y.a += 1;
    |         ^^^^^
-help: consider mutably borrowing `hello`
-   |
-LL |     let b = &mut hello;
-   |             ++++
 
 error: aborting due to 1 previous error
 

diff --git a/...s/ui/closures/2229_closure_analysis/diagnostics/closure-origin-tuple-diagnostics-1.stderr b/...s/ui/closures/2229_closure_analysis/diagnostics/closure-origin-tuple-diagnostics-1.stderr
@@ -11,10 +11,6 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         x.0 += 1;
    |         ^^^
-help: consider mutably borrowing `hello`
-   |
-LL |     let b = &mut hello;
-   |             ++++
 
 error: aborting due to 1 previous error
 

diff --git a/tests/ui/moves/auxiliary/suggest-borrow-for-generic-arg-aux.rs b/tests/ui/moves/auxiliary/suggest-borrow-for-generic-arg-aux.rs
@@ -0,0 +1,20 @@
+//! auxiliary definitons for suggest-borrow-for-generic-arg.rs, to ensure the suggestion works on
+//! functions defined in other crates.
+
+use std::io::{self, Read, Write};
+use std::iter::Sum;
+
+pub fn write_stuff<W: Write>(mut writer: W) -> io::Result<()> {
+    writeln!(writer, "stuff")
+}
+
+pub fn read_and_discard<R: Read>(mut reader: R) -> io::Result<()> {
+    let mut buf = Vec::new();
+    reader.read_to_end(&mut buf).map(|_| ())
+}
+
+pub fn sum_three<I: IntoIterator>(iter: I) -> <I as IntoIterator>::Item
+    where <I as IntoIterator>::Item: Sum
+{
+    iter.into_iter().take(3).sum()
+}
diff --git a/tests/ui/moves/borrow-closures-instead-of-move.rs b/tests/ui/moves/borrow-closures-instead-of-move.rs
@@ -1,4 +1,4 @@
-fn takes_fn(f: impl Fn()) { //~ HELP if `impl Fn()` implemented `Clone`
+fn takes_fn(f: impl Fn()) {
     loop {
         takes_fnonce(f);
         //~^ ERROR use of moved value

diff --git a/tests/ui/moves/borrow-closures-instead-of-move.stderr b/tests/ui/moves/borrow-closures-instead-of-move.stderr
@@ -8,21 +8,6 @@ LL |     loop {
 LL |         takes_fnonce(f);
    |                      ^ value moved here, in previous iteration of loop
    |
-note: consider changing this parameter type in function `takes_fnonce` to borrow instead if owning the value isn't necessary
-  --> $DIR/borrow-closures-instead-of-move.rs:34:20
-   |
-LL | fn takes_fnonce(_: impl FnOnce()) {}
-   |    ------------    ^^^^^^^^^^^^^ this parameter takes ownership of the value
-   |    |
-   |    in this function
-help: if `impl Fn()` implemented `Clone`, you could clone the value
-  --> $DIR/borrow-closures-instead-of-move.rs:1:16
-   |
-LL | fn takes_fn(f: impl Fn()) {
-   |                ^^^^^^^^^ consider constraining this type parameter with `Clone`
-LL |     loop {
-LL |         takes_fnonce(f);
-   |                      - you could clone this value
 help: consider borrowing `f`
    |
 LL |         takes_fnonce(&f);
@@ -40,13 +25,6 @@ LL |         takes_fnonce(m);
 LL |     takes_fnonce(m);
    |                  ^ value used here after move
    |
-note: consider changing this parameter type in function `takes_fnonce` to borrow instead if owning the value isn't necessary
-  --> $DIR/borrow-closures-instead-of-move.rs:34:20
-   |
-LL | fn takes_fnonce(_: impl FnOnce()) {}
-   |    ------------    ^^^^^^^^^^^^^ this parameter takes ownership of the value
-   |    |
-   |    in this function
 help: if `impl FnMut()` implemented `Clone`, you could clone the value
   --> $DIR/borrow-closures-instead-of-move.rs:9:20
    |

diff --git a/tests/ui/moves/moved-value-on-as-ref-arg.stderr b/tests/ui/moves/moved-value-on-as-ref-arg.stderr
@@ -8,7 +8,7 @@ LL |     foo(bar);
 LL |     let _baa = bar;
    |                ^^^ value used here after move
    |
-help: borrow the value to avoid moving it
+help: consider borrowing `bar`
    |
 LL |     foo(&bar);
    |         +
@@ -31,7 +31,7 @@ LL | struct Bar;
 ...
 LL |     qux(bar);
    |         --- you could clone this value
-help: borrow the value to avoid moving it
+help: consider mutably borrowing `bar`
    |
 LL |     qux(&mut bar);
    |         ++++
@@ -46,7 +46,7 @@ LL |     bat(bar);
 LL |     let _baa = bar;
    |                ^^^ value used here after move
    |
-help: borrow the value to avoid moving it
+help: consider borrowing `bar`
    |
 LL |     bat(&bar);
    |         +
@@ -69,7 +69,7 @@ LL | struct Bar;
 ...
 LL |     baz(bar);
    |         --- you could clone this value
-help: borrow the value to avoid moving it
+help: consider mutably borrowing `bar`
    |
 LL |     baz(&mut bar);
    |         ++++

diff --git a/tests/ui/moves/moves-based-on-type-no-recursive-stack-closure.stderr b/tests/ui/moves/moves-based-on-type-no-recursive-stack-closure.stderr
@@ -24,10 +24,6 @@ LL | fn conspirator<F>(mut f: F) where F: FnMut(&mut R, bool) {
    |                ^ consider constraining this type parameter with `Clone`
 LL |     let mut r = R {c: Box::new(f)};
    |                                - you could clone this value
-help: consider mutably borrowing `f`
-   |
-LL |     let mut r = R {c: Box::new(&mut f)};
-   |                                ++++
 
 error: aborting due to 2 previous errors
 

diff --git a/tests/ui/moves/suggest-borrow-for-generic-arg.fixed b/tests/ui/moves/suggest-borrow-for-generic-arg.fixed
@@ -0,0 +1,46 @@
+//! Test suggetions to borrow generic arguments instead of moving. Tests for other instances of this
+//! can be found in `moved-value-on-as-ref-arg.rs` and `borrow-closures-instead-of-move.rs`
+//@ run-rustfix
+//@ aux-crate:aux=suggest-borrow-for-generic-arg-aux.rs
+//@ edition: 2021
+
+#![allow(unused_mut)]
+use std::io::{self, Write};
+
+// test for `std::io::Write` (#131413)
+fn test_write() -> io::Result<()> {
+    let mut stdout = io::stdout();
+    aux::write_stuff(&stdout)?; //~ HELP consider borrowing `stdout`
+    writeln!(stdout, "second line")?; //~ ERROR borrow of moved value: `stdout`
+
+    let mut buf = Vec::new();
+    aux::write_stuff(&mut buf.clone())?; //~ HELP consider mutably borrowing `buf`
+    //~^ HELP consider cloning the value
+    writeln!(buf, "second_line") //~ ERROR borrow of moved value: `buf`
+}
+
+/// test for `std::io::Read` (#131413)
+fn test_read() -> io::Result<()> {
+    let stdin = io::stdin();
+    aux::read_and_discard(&stdin)?; //~ HELP consider borrowing `stdin`
+    aux::read_and_discard(stdin)?; //~ ERROR use of moved value: `stdin`
+
+    let mut bytes = std::collections::VecDeque::from([1, 2, 3, 4, 5, 6]);
+    aux::read_and_discard(&mut bytes.clone())?; //~ HELP consider mutably borrowing `bytes`
+    //~^ HELP consider cloning the value
+    aux::read_and_discard(bytes) //~ ERROR use of moved value: `bytes`
+}
+
+/// test that suggestions work with projection types in the callee's signature
+fn test_projections() {
+    let mut iter = [1, 2, 3, 4, 5, 6].into_iter();
+    let _six: usize = aux::sum_three(&mut iter.clone()); //~ HELP consider mutably borrowing `iter`
+    //~^ HELP consider cloning the value
+    let _fifteen: usize = aux::sum_three(iter); //~ ERROR use of moved value: `iter`
+}
+
+fn main() {
+    test_write().unwrap();
+    test_read().unwrap();
+    test_projections();
+}
diff --git a/tests/ui/moves/suggest-borrow-for-generic-arg.rs b/tests/ui/moves/suggest-borrow-for-generic-arg.rs
@@ -0,0 +1,46 @@
+//! Test suggetions to borrow generic arguments instead of moving. Tests for other instances of this
+//! can be found in `moved-value-on-as-ref-arg.rs` and `borrow-closures-instead-of-move.rs`
+//@ run-rustfix
+//@ aux-crate:aux=suggest-borrow-for-generic-arg-aux.rs
+//@ edition: 2021
+
+#![allow(unused_mut)]
+use std::io::{self, Write};
+
+// test for `std::io::Write` (#131413)
+fn test_write() -> io::Result<()> {
+    let mut stdout = io::stdout();
+    aux::write_stuff(stdout)?; //~ HELP consider borrowing `stdout`
+    writeln!(stdout, "second line")?; //~ ERROR borrow of moved value: `stdout`
+
+    let mut buf = Vec::new();
+    aux::write_stuff(buf)?; //~ HELP consider mutably borrowing `buf`
+    //~^ HELP consider cloning the value
+    writeln!(buf, "second_line") //~ ERROR borrow of moved value: `buf`
+}
+
+/// test for `std::io::Read` (#131413)
+fn test_read() -> io::Result<()> {
+    let stdin = io::stdin();
+    aux::read_and_discard(stdin)?; //~ HELP consider borrowing `stdin`
+    aux::read_and_discard(stdin)?; //~ ERROR use of moved value: `stdin`
+
+    let mut bytes = std::collections::VecDeque::from([1, 2, 3, 4, 5, 6]);
+    aux::read_and_discard(bytes)?; //~ HELP consider mutably borrowing `bytes`
+    //~^ HELP consider cloning the value
+    aux::read_and_discard(bytes) //~ ERROR use of moved value: `bytes`
+}
+
+/// test that suggestions work with projection types in the callee's signature
+fn test_projections() {
+    let mut iter = [1, 2, 3, 4, 5, 6].into_iter();
+    let _six: usize = aux::sum_three(iter); //~ HELP consider mutably borrowing `iter`
+    //~^ HELP consider cloning the value
+    let _fifteen: usize = aux::sum_three(iter); //~ ERROR use of moved value: `iter`
+}
+
+fn main() {
+    test_write().unwrap();
+    test_read().unwrap();
+    test_projections();
+}
diff --git a/tests/ui/moves/suggest-borrow-for-generic-arg.stderr b/tests/ui/moves/suggest-borrow-for-generic-arg.stderr
@@ -0,0 +1,93 @@
+error[E0382]: borrow of moved value: `stdout`
+  --> $DIR/suggest-borrow-for-generic-arg.rs:14:14
+   |
+LL |     let mut stdout = io::stdout();
+   |         ---------- move occurs because `stdout` has type `Stdout`, which does not implement the `Copy` trait
+LL |     aux::write_stuff(stdout)?;
+   |                      ------ value moved here
+LL |     writeln!(stdout, "second line")?;
+   |              ^^^^^^ value borrowed here after move
+   |
+help: consider borrowing `stdout`
+   |
+LL |     aux::write_stuff(&stdout)?;
+   |                      +
+
+error[E0382]: borrow of moved value: `buf`
+  --> $DIR/suggest-borrow-for-generic-arg.rs:19:14
+   |
+LL |     let mut buf = Vec::new();
+   |         ------- move occurs because `buf` has type `Vec<u8>`, which does not implement the `Copy` trait
+LL |     aux::write_stuff(buf)?;
+   |                      --- value moved here
+LL |
+LL |     writeln!(buf, "second_line")
+   |              ^^^ value borrowed here after move
+   |
+help: consider mutably borrowing `buf`
+   |
+LL |     aux::write_stuff(&mut buf)?;
+   |                      ++++
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |     aux::write_stuff(buf.clone())?;
+   |                         ++++++++
+
+error[E0382]: use of moved value: `stdin`
+  --> $DIR/suggest-borrow-for-generic-arg.rs:26:27
+   |
+LL |     let stdin = io::stdin();
+   |         ----- move occurs because `stdin` has type `Stdin`, which does not implement the `Copy` trait
+LL |     aux::read_and_discard(stdin)?;
+   |                           ----- value moved here
+LL |     aux::read_and_discard(stdin)?;
+   |                           ^^^^^ value used here after move
+   |
+help: consider borrowing `stdin`
+   |
+LL |     aux::read_and_discard(&stdin)?;
+   |                           +
+
+error[E0382]: use of moved value: `bytes`
+  --> $DIR/suggest-borrow-for-generic-arg.rs:31:27
+   |
+LL |     let mut bytes = std::collections::VecDeque::from([1, 2, 3, 4, 5, 6]);
+   |         --------- move occurs because `bytes` has type `VecDeque<u8>`, which does not implement the `Copy` trait
+LL |     aux::read_and_discard(bytes)?;
+   |                           ----- value moved here
+LL |
+LL |     aux::read_and_discard(bytes)
+   |                           ^^^^^ value used here after move
+   |
+help: consider mutably borrowing `bytes`
+   |
+LL |     aux::read_and_discard(&mut bytes)?;
+   |                           ++++
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |     aux::read_and_discard(bytes.clone())?;
+   |                                ++++++++
+
+error[E0382]: use of moved value: `iter`
+  --> $DIR/suggest-borrow-for-generic-arg.rs:39:42
+   |
+LL |     let mut iter = [1, 2, 3, 4, 5, 6].into_iter();
+   |         -------- move occurs because `iter` has type `std::array::IntoIter<usize, 6>`, which does not implement the `Copy` trait
+LL |     let _six: usize = aux::sum_three(iter);
+   |                                      ---- value moved here
+LL |
+LL |     let _fifteen: usize = aux::sum_three(iter);
+   |                                          ^^^^ value used here after move
+   |
+help: consider mutably borrowing `iter`
+   |
+LL |     let _six: usize = aux::sum_three(&mut iter);
+   |                                      ++++
+help: consider cloning the value if the performance cost is acceptable
+   |
+LL |     let _six: usize = aux::sum_three(iter.clone());
+   |                                          ++++++++
+
+error: aborting due to 5 previous errors
+
+For more information about this error, try `rustc --explain E0382`.
diff --git a/tests/ui/not-copy-closure.stderr b/tests/ui/not-copy-closure.stderr
@@ -11,10 +11,6 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         a += 1;
    |         ^
-help: consider mutably borrowing `hello`
-   |
-LL |     let b = &mut hello;
-   |             ++++
 
 error: aborting due to 1 previous error