borrowck diagnostics: suggest borrowing function inputs in generic positions #132172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
rustbot has assigned @matthewjasper. Use |
rustbot
added
S-waiting-on-review
dianne
force-pushed
the
suggest-borrow-generic
branch
from
3c4d321 to
f4e6d28
Compare
| }) | ||
| }), | ||
| // an opaque type is fn-like if its trait is fn-like | ||
| ty::Alias(ty::Opaque, opaque) => self.infcx.tcx.is_fn_trait(opaque.def_id), |
There was a problem hiding this comment.
This isn't going to ever return true, the DefId here is for the opaque type, not the trait it implements. You'll need to use something like explicit_item_bounds to get the list of bounds. There also doesn't appear to be a test for this.
There was a problem hiding this comment.
Oh, true. Looking at it more closely, I don't think that case is ever relevant, nor the ty::Closure one. suggest_adding_bounds won't do anything unless ty is a parameter. That case I think is already tested by tests/ui/moves/borrow_closure_instead_of_move.rs; removing the check produces suggestions to add + Copy to the impl Fn() and impl FnMut() arguments. I've removed the unnecessary checks. Thanks!
… from other crates This also downgrades its applicability to MaybeIncorrect. Its suggestion can result in ill-typed code when the type parameter it suggests providing a different generic argument for appears elsewhere in the callee's signature or predicates.
This is setup for unifing the logic for suggestions to borrow arguments in generic positions. As of this commit, it's still special cases for `AsRef`/`Borrow`-like traits and `Fn`-like traits.
dianne
force-pushed
the
suggest-borrow-generic
branch
from
f4e6d28 to
5cf64cc
Compare
…e satisfied This subsumes the suggestions to borrow arguments with `AsRef`/`Borrow` bounds and those to borrow arguments with `Fn` and `FnMut` bounds. It works for other traits implemented on references as well, such as `std::io::Read`, `std::io::Write`, and `core::fmt::Write`. Incidentally, by making the logic for suggesting borrowing closures general, this removes some spurious suggestions to mutably borrow `FnMut` closures in assignments, as well as an unhelpful suggestion to add a `Clone` constraint to an `impl Fn` argument.
dianne
force-pushed
the
suggest-borrow-generic
branch
from
5cf64cc to
2ab8480
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
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
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 14, 2024
…llaumeGomez Rollup of 5 pull requests Successful merges: - rust-lang#132172 (borrowck diagnostics: suggest borrowing function inputs in generic positions) - rust-lang#132649 (add ./x clippy ci) - rust-lang#133005 (rustdoc: use a trie for name-based search) - rust-lang#133034 (update download-rustc comments and default) - rust-lang#133036 (add myself into `users_on_vacation` on triagebot) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 andFn-like traits. This PR changes it to test if, for a moved place with typeT, that the callee's signature and clauses don't break if you substitute in&Tor&mut T. For instance, it now works withRead/Write-like traits.Fixes #131413
Incidental changes
Fn, since they can be borrowed (see e.g. tests/ui/moves/borrow-closures-instead-of-move.stderr).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
Iterator::sum), borrowing the receiver isn't useful since it's consumed.Rng::sample_iteris effectively the same (modulo regions) whether you borrow or consume the receiverRng, 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.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.