Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

E0382 diagnostic could suggest a mutable borrow for Read/Write/Sync type bounds #131413

Closed
iliana opened this issue Oct 8, 2024 · 2 comments · Fixed by #132172
Closed

E0382 diagnostic could suggest a mutable borrow for Read/Write/Sync type bounds #131413

iliana opened this issue Oct 8, 2024 · 2 comments · Fixed by #132172
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@iliana
Copy link

iliana commented Oct 8, 2024

I helped someone out with an example like this, which doesn't compile:

use std::io::{Result, Write};

fn write_stuff<W: Write>(mut writer: W) -> Result<()> {
    writeln!(writer, "stuff")
}

fn main() {
    let mut stdout = std::io::stdout();
    writeln!(stdout, "first line").unwrap();
    write_stuff(stdout);
    writeln!(stdout, "second line").unwrap();
}
   Compiling playground v0.0.1 (/playground)
error[E0382]: borrow of moved value: `stdout`
  --> src/main.rs:11:14
   |
8  |     let mut stdout = std::io::stdout();
   |         ---------- move occurs because `stdout` has type `Stdout`, which does not implement the `Copy` trait
9  |     writeln!(stdout, "first line").unwrap();
10 |     write_stuff(stdout);
   |                 ------ value moved here
11 |     writeln!(stdout, "second line").unwrap();
   |              ^^^^^^ value borrowed here after move
   |
note: consider changing this parameter type in function `write_stuff` to borrow instead if owning the value isn't necessary
  --> src/main.rs:3:38
   |
3  | fn write_stuff<W: Write>(mut writer: W) -> Result<()> {
   |    ----------- in this function      ^ this parameter takes ownership of the value

For more information about this error, try `rustc --explain E0382`.
error: could not compile `playground` (bin "playground") due to 1 previous error

The compiler suggests changing the parameter of write_stuff to take &mut W, but suppose you do not have control over that function. Because Read/Write/Sync are implemented over mutable references of readers/writers/syncers(?), I suggested changing the code to:

    write_stuff(&mut stdout);

Could the compiler detect that the type bound here is some combination of Read/Write/Sync and suggest a mutable borrow like this?

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 8, 2024
@ChayimFriedman2
Copy link
Contributor

Maybe even more generally suggest adding a reference when the trait is implemented for it.

@rustbot label +A-diagnostics

@rustbot rustbot added the A-diagnostics Area: Messages for errors, warnings, and lints label Oct 8, 2024
@lolbinarycat lolbinarycat added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` labels Oct 9, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 19, 2024
@dianne
Copy link
Contributor

dianne commented Oct 19, 2024

@rustbot claim

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 14, 2024
…atthewjasper

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 rust-lang#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.
@bors bors closed this as completed in 5ee347e Nov 14, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 14, 2024
Rollup merge of rust-lang#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 rust-lang#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.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants