Enhance needless_borrow to consider trait implementations
#9136
Conversation
rust-highfive
added
the
S-waiting-on-review
|
Probably won't have time to look at this for a few days. Does this handle something like: struct S<T>(Vec<T>);
impl<T> S<T> {
fn push(&mut self, item: T) where T: Eq { .. }
} |
|
No problem at all. Something like |
|
However, you caused me to realize I was incorrectly assuming the argument type was a type parameter. I have corrected the error and added a relevant test. |
There was a problem hiding this comment.
This should probably ignore cases where the trait has a &mut self method, but the unreferenced type is not a mutable reference. e.g.
#[derive(Clone, Copy)]
struct Iter(..);
impl Iterator for Iter { .. }
fn takes_iter(_: impl Iterator) {}
fn dont_warn(mut x: Iter) -> {
takes_iter(&mut x);
}
fn warn(mut x: &mut Iter) -> {
takes_iter(&mut x)
}Iterators shouldn't implement Copy for exactly this reason, but that doesn't stop it from happening.
|
☔ The latest upstream changes (presumably #9169) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I got a crash with this branch on the crossbeam-epoch-0.9.9 crate. Backtrace |
|
Thanks, @mikerite! |
smoelius
force-pushed
the
enhance-needless-borrow
branch
from
3b2d61d
to
3ae202d
Compare
|
I think the most recent push addresses all of the review comments up to this point, and the crash. Please let me know if I misunderstood anything. Regarding the "is copyable" condition, I imposed a hard limit of 256 bytes. Please let me know if you would prefer a different approach. |
|
A couple of comments now that I've had time to think through the implementation.
|
You're saying this should be done for all predicates, not just trait predicates, correct? If so, are there any tools you know of to make iii easier? I.e., something easier than performing a match and handling each |
|
The |
|
☔ The latest upstream changes (presumably #9210) made this pull request unmergeable. Please resolve the merge conflicts. |
smoelius
force-pushed
the
enhance-needless-borrow
branch
3 times, most recently
from
43ea74d
to
cd70a3d
Compare
|
I think the second round of comments are addressed. |
|
Please let me know if I misunderstood any of your suggestions, @Jarcho. |
|
@Jarcho How's this? |
clippy_lints/src/dereference.rs
Outdated
| let new_generic_arg = ty::GenericArg::from(new_ty); | ||
|
|
||
| // If `replaced.is_empty()`, then `param_ty` and `new_ty` are those initially passed in. | ||
| if !(substs[param_ty.index as usize] == new_generic_arg |
There was a problem hiding this comment.
This check could be done right before pushing the types to the queue thereby removing the need to check for the initial argument.
|
Please consider this paused. I have to fix this. |
smoelius
force-pushed
the
enhance-needless-borrow
branch
from
7d9ec81
to
6dd09e1
Compare
|
OK. I think this is fixed now. The new test ICEs without |
|
That should be the last thing. I want to run one more check on more crates, but that will take a couple of days as I have other tests running right now. |
|
Looks like it mostly catches You can squash down some of the commits and then it's good to go. |
smoelius
force-pushed
the
enhance-needless-borrow
branch
from
6dd09e1
to
0bb9b1a
Compare
|
Done. I left in the change that moved |
|
It would be better to remove it. This would get removed as an unused util function if we ever clean up useless/overlapping util functions. I'm going to put a hold on merging this as it conflicts #9126 and the lint fixed there is about to beta. I can't guarantee I'll have time to deal with the merge conflict in the next week. |
smoelius
force-pushed
the
enhance-needless-borrow
branch
3 times, most recently
from
903274b
to
ad778ad
Compare
I moved
No problem at all. |
|
☔ The latest upstream changes (presumably #9126) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping @smoelius just in case github didn't. This is good to merge once the conflicts are resolved. Sorry for dumping that on you. |
smoelius
force-pushed
the
enhance-needless-borrow
branch
from
ad778ad
to
032f112
Compare
|
Sorry, I misunderstood your previous message. If it helps to verify the rebase, here is the diff between the old patch (ad778ad) and the new (032f112): 15c15
< index d513a229b..47bdc41a2 100644
---
> index 3b27f061e..357cf6fc4 100644
25c25
< return Err(CliError::RaSetupActive);
---
> return Err(CliError::IntellijSetupActive);
50c50
< index c089f4d8c..c831fb831 100644
---
> index 05e79a241..c503142e5 100644
72c72
< index a90f894a7..7d2b526bc 100644
---
> index e0b94f719..e38704701 100644
79c79
< -use clippy_utils::ty::{expr_sig, peel_mid_ty_refs, variant_of_res};
---
> -use clippy_utils::ty::{expr_sig, peel_mid_ty_refs, ty_sig, variant_of_res};
81c81
< +use clippy_utils::ty::{contains_ty, expr_sig, is_copy, peel_mid_ty_refs, variant_of_res};
---
> +use clippy_utils::ty::{contains_ty, expr_sig, is_copy, peel_mid_ty_refs, ty_sig, variant_of_res};
99c99
< -use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitable, TypeckResults};
---
> -use rustc_middle::ty::{self, Binder, BoundVariableKind, List, Ty, TyCtxt, TypeVisitable, TypeckResults};
101,102c101,102
< + self, subst::Subst, EarlyBinder, FnSig, GenericArgKind, ParamTy, PredicateKind, ProjectionPredicate, Ty, TyCtxt,
< + TypeVisitable, TypeckResults,
---
> + self, subst::Subst, Binder, BoundVariableKind, EarlyBinder, FnSig, GenericArgKind, List, ParamTy, PredicateKind,
> + ProjectionPredicate, Ty, TyCtxt, TypeVisitable, TypeckResults,
106c106
< use rustc_span::{symbol::sym, Span, Symbol};
---
> use rustc_span::{symbol::sym, Span, Symbol, DUMMY_SP};
196c196
< @@ -521,7 +547,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
---
> @@ -510,7 +536,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
205c205
< @@ -553,6 +579,8 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
---
> @@ -542,6 +568,8 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing {
214c214
< @@ -605,6 +633,7 @@ enum Position {
---
> @@ -594,6 +622,7 @@ enum Position {
222c222
< @@ -638,7 +667,7 @@ impl Position {
---
> @@ -630,7 +659,7 @@ impl Position {
228c228
< Self::DerefStable(p) | Self::ReborrowStable(p) | Self::Other(p) => p,
---
> Self::DerefStable(p, _) | Self::ReborrowStable(p) | Self::Other(p) => p,
231c231
< @@ -647,8 +676,12 @@ impl Position {
---
> @@ -639,8 +668,12 @@ impl Position {
246c246
< @@ -751,12 +784,19 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &
---
> @@ -732,13 +765,20 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &
254,255c254,256
< - Some(ty) => binding_ty_auto_deref_stability(ty, precedence),
< - None => param_auto_deref_stability(ty.skip_binder(), precedence),
---
> - Some(hir_ty) => binding_ty_auto_deref_stability(cx, hir_ty, precedence, ty.bound_vars()),
> - None => ty_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence)
> - .position_for_arg(),
260c261
< + Some(ty) => binding_ty_auto_deref_stability(ty, precedence),
---
> + Some(hir_ty) => binding_ty_auto_deref_stability(cx, hir_ty, precedence, ty.bound_vars()),
265c266,267
< + param_auto_deref_stability(ty.skip_binder(), precedence)
---
> + ty_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence)
> + .position_for_arg()
272c274
< @@ -797,7 +837,12 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &
---
> @@ -779,12 +819,17 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &
276c278,283
< - param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i], precedence)
---
> - ty_auto_deref_stability(
> - cx,
> - cx.tcx.erase_late_bound_regions(cx.tcx.fn_sig(id).input(i)),
> - precedence,
> - )
> - .position_for_arg()
281c288,293
< + param_auto_deref_stability(ty, precedence)
---
> + ty_auto_deref_stability(
> + cx,
> + cx.tcx.erase_late_bound_regions(cx.tcx.fn_sig(id).input(i)),
> + precedence,
> + )
> + .position_for_arg()
286c298
< @@ -920,6 +965,205 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool {
---
> @@ -946,6 +991,205 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool {
489,492c501,504
< // Checks whether a type is stable when switching to auto dereferencing,
< fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position {
< let ty::Ref(_, mut ty, _) = *ty.kind() else {
< @@ -1026,7 +1270,8 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data
---
> struct TyPosition<'tcx> {
> position: Position,
> ty: Option<Ty<'tcx>>,
> @@ -1084,7 +1328,8 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data
503c515
< index 8444532f6..ef8c33a03 100644
---
> index e6a405f81..521739c28 100644
506c518
< @@ -807,7 +807,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
---
> @@ -821,7 +821,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
516c528
< index b4c6bfb31..ad55ee63c 100644
---
> index 99b56da7a..e4624167a 100644
519,521c531,533
< @@ -5,9 +5,8 @@ use clippy_utils::source::snippet_opt;
< use clippy_utils::ty::{
< contains_ty, get_associated_type, get_iterator_item_ty, implements_trait, is_copy, peel_mid_ty_refs,
---
> @@ -6,9 +6,8 @@ use clippy_utils::ty::{
> contains_ty, get_associated_type, get_iterator_item_ty, implements_trait, is_copy, is_type_diagnostic_item,
> peel_mid_ty_refs,
530c542
< @@ -360,25 +359,15 @@ fn get_input_traits_and_projections<'tcx>(
---
> @@ -373,25 +372,15 @@ fn get_input_traits_and_projections<'tcx>( |
|
Thank you for pushing through and implementing all this. @bors r+ |
|
Thanks for the detailed and thorough review! |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Further enhance `needless_borrow`, mildly refactor `redundant_clone` This PR does the following: * Moves some code from `redundant_clone` into a new `clippy_utils` module called `mir`, and wraps that code in a function called `dropped_without_further_use`. * Relaxes the "is copyable" condition condition from #9136 by also suggesting to remove borrows from values dropped without further use. The changes involve the just mentioned function. * Separates `redundant_clone` into modules. Strictly speaking, the last bullet is independent of the others. `redundant_clone` is somewhat hairy, IMO. Separating it into modules makes it slightly less so, by helping to delineate what depends upon what. I've tried to break everything up into digestible commits. r? `@Jarcho` (`@Jarcho` I hope you don't mind.) changelog: continuation of #9136
The proposed enhancement causes
needless_borrowto suggest removing&from&ewhen&eis an argument position requiring trait implementations, andeimplements the required traits. Example:r? @Jarcho
changelog: Enhance
needless_borrowto consider trait implementations