Conversation
Detects owned, non-`Copy` values passed by value to a function-level
generic type parameter when a shared reference to the value would also
satisfy the complete instantiated predicate set of the callee, e.g.
fn read(path: impl AsRef<Path>) {}
let path = PathBuf::from("/tmp");
read(path); // suggest `read(&path)`
Also detects explicit `Clone::clone` calls in such argument positions,
suggesting to borrow the original value instead.
The analysis is type-aware: the lint builds `&A`, substitutes it for
exactly the one function-level generic argument in the callee's
`GenericArgs`, re-instantiates *all* caller bounds of the callee and
proves each one with `predicate_must_hold_modulo_regions`. It
conservatively skips:
* formal parameters that are not directly a type parameter (nested
occurrences like `Option<T>`),
* impl/trait-level or `Self` parameters,
* parameters occurring more than once in the signature (shared by
another argument or the return type),
* explicit turbofish on the callee,
* `Copy` values, references, temporaries and non-place expressions,
* types marked `#[clippy::has_significant_drop]` (e.g. `MutexGuard`),
* `.clone()` calls not resolving to `Clone::clone`.
Because trait implementations for `T` and `&T` can differ and moving
changes drop location, the lint is in the `restriction` group and the
suggestion is `MaybeIncorrect`.
It is complementary to `needless_borrows_for_generic_args` (`&T -> T`):
that lint only suggests removing a borrow for `Copy` values,
temporaries and uniquely-used mutable references, so the two cannot
produce a fix-loop. The duplicated `path_has_args` helper is replaced
by `clippy_utils::last_path_segment`.
Candidate arguments are filtered with cheap HIR checks before any
signature/predicate queries run. Measured overhead is below wall-clock
noise: ~25us per emitted lint (2000 hits in ~47ms), and no measurable
difference on `clippy_lints` itself.
Collaborator
|
Thanks for the pull request, and welcome! You should hear from one of our reviewers after this PR gets at least 2 reviews from the community. Please see the contribution instructions for more information. |
|
Lintcheck changes for b5217fc
This comment will be updated if you push new changes |
This branch has not been deployed
This file contains hidden or 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
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.
Adds a new
restrictionlintneedless_owned_generic_argsthat detects owned,non-
Copyvalues passed by value to a function-level generic type parameterwhen a shared reference to the value would also satisfy the callee's complete
instantiated predicate set:
Use instead:
Explicit
Clone::clonecalls in such argument positions are detected too(
read(path.clone())->read(&path)).How it works
The lint builds
&A, substitutes it for exactly the one function-level genericargument in the callee's
GenericArgs, re-instantiates all caller bounds ofthe callee, and proves each with
predicate_must_hold_modulo_regions(sameidiom as
useless_conversion/needless_borrows_for_generic_args). It isdeliberately conservative and skips:
Option<T>)Selfparametersargument, or appearing in the return type)
Copyvalues, references, temporaries, and non-place expressions#[clippy::has_significant_drop](e.g.MutexGuard).clone()calls that do not resolve toClone::cloneIt is the ownership-policy counterpart of
needless_borrows_for_generic_args(
&T -> T, see #12454 / #12706): that lint only removes borrows ofCopyvalues, temporaries, and uniquely-used mutable references, so the two lints
cannot produce a fix-loop (covered by a
.fixedUI test that recompileswarning-free with both lint groups active). The duplicated
path_has_argshelper was replaced by
clippy_utils::last_path_segment.Since
impl Trait for Tandimpl Trait for &Tmay behave differently andmoving changes drop location, the lint is in the
restrictiongroup and thesuggestion is
MaybeIncorrect.Performance: candidate arguments are filtered with cheap HIR checks before any
signature/predicate queries run. Measured overhead of the lint pass is below
wall-clock noise on
clippy_lintsitself; the trait-solver cost is roughly25us per emitted lint (2000 hits in ~47ms).
changelog: new lint: [
needless_owned_generic_args]LLM usage disclosure
This PR (implementation, tests, and this description) was drafted with LLM
assistance. The changes were built and verified locally before submission:
cargo test(including the new UI tests and dogfood),cargo dev fmt, andcargo dev update_lints --checkall pass..stderrfile)cargo testpasses locallycargo dev update_lintscargo dev fmt