From e34e49f7ff8a4fee6667c3ec68a76e2a55e36ad8 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 6 Aug 2023 23:45:34 +0200 Subject: [PATCH 1/3] `useless_conversion`: don't lint if ty param has multiple bounds --- clippy_lints/src/useless_conversion.rs | 99 +++++++++++++++++++++----- tests/ui/useless_conversion.fixed | 18 +++++ tests/ui/useless_conversion.rs | 18 +++++ tests/ui/useless_conversion.stderr | 20 +++--- 4 files changed, 128 insertions(+), 27 deletions(-) diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs index 5ac4f0aa46c1..05acf034a95d 100644 --- a/clippy_lints/src/useless_conversion.rs +++ b/clippy_lints/src/useless_conversion.rs @@ -1,15 +1,19 @@ +use std::ops::ControlFlow; + use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then}; use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_context}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::{is_copy, is_type_diagnostic_item, same_type_and_consts}; -use clippy_utils::{get_parent_expr, is_trait_method, is_ty_alias, match_def_path, path_to_local, paths}; +use clippy_utils::{ + get_parent_expr, is_diag_trait_item, is_trait_method, is_ty_alias, match_def_path, path_to_local, paths, +}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::def::DefKind; use rustc_hir::def_id::DefId; use rustc_hir::{BindingAnnotation, Expr, ExprKind, HirId, MatchSource, Node, PatKind}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty; +use rustc_middle::ty::{self, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{sym, Span}; @@ -61,22 +65,83 @@ impl MethodOrFunction { } } -/// Returns the span of the `IntoIterator` trait bound in the function pointed to by `fn_did` -fn into_iter_bound(cx: &LateContext<'_>, fn_did: DefId, into_iter_did: DefId, param_index: u32) -> Option { - cx.tcx - .predicates_of(fn_did) - .predicates - .iter() - .find_map(|&(ref pred, span)| { - if let ty::ClauseKind::Trait(tr) = pred.kind().skip_binder() - && tr.def_id() == into_iter_did - && tr.self_ty().is_param(param_index) +/// Returns the span of the `IntoIterator` trait bound in the function pointed to by `fn_did`, +/// iff the `IntoIterator` bound is the only bound on the type parameter. +/// +/// This last part is important because it might be that the type the user is calling `.into_iter()` +/// on might not satisfy those other bounds and would result in compile errors: +/// ```ignore +/// pub fn foo(i: I) +/// where I: IntoIterator + ExactSizeIterator +/// ^^^^^^^^^^^^^^^^^ this extra bound stops us from suggesting to remove `.into_iter()` ... +/// { +/// assert_eq!(i.len(), 3); +/// } +/// +/// pub fn bar() { +/// foo([1, 2, 3].into_iter()); +/// ^^^^^^^^^^^^ ... here, because `[i32; 3]` is not `ExactSizeIterator` +/// } +/// ``` +fn exclusive_into_iter_bound( + cx: &LateContext<'_>, + fn_did: DefId, + into_iter_did: DefId, + param_index: u32, +) -> Option { + #[derive(Clone)] + struct ExplicitlyUsedTyParam<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + param_index: u32, + } + + impl<'a, 'tcx> TypeVisitor> for ExplicitlyUsedTyParam<'a, 'tcx> { + type BreakTy = (); + fn visit_predicate(&mut self, p: ty::Predicate<'tcx>) -> ControlFlow { + // Ignore implicit `T: Sized` bound + if let ty::PredicateKind::Clause(ty::ClauseKind::Trait(tr)) = p.kind().skip_binder() + && let Some(sized_trait_did) = self.cx.tcx.lang_items().sized_trait() + && sized_trait_did == tr.def_id() { - Some(span) - } else { - None + return ControlFlow::Continue(()); } - }) + + // Ignore `::Item` projection, this use of the ty param specifically is fine + // because it's what we're already looking for + if let ty::PredicateKind::Clause(ty::ClauseKind::Projection(proj)) = p.kind().skip_binder() + && is_diag_trait_item(self.cx,proj.projection_ty.def_id, sym::IntoIterator) + { + return ControlFlow::Continue(()); + } + + p.super_visit_with(self) + } + + fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow { + if t.is_param(self.param_index) { + ControlFlow::Break(()) + } else { + ControlFlow::Continue(()) + } + } + } + + let mut into_iter_span = None; + + for (pred, span) in cx.tcx.explicit_predicates_of(fn_did).predicates { + if let ty::ClauseKind::Trait(tr) = pred.kind().skip_binder() + && tr.def_id() == into_iter_did + && tr.self_ty().is_param(param_index) + { + into_iter_span = Some(*span); + } else if pred.visit_with(&mut ExplicitlyUsedTyParam { cx, param_index }).is_break() { + // Found another reference of the type parameter; conservatively assume + // that we can't remove the bound. + return None; + } + } + + into_iter_span } /// Extracts the receiver of a `.into_iter()` method call. @@ -175,7 +240,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { && let Some(arg_pos) = args.iter().position(|x| x.hir_id == e.hir_id) && let Some(&into_iter_param) = sig.inputs().get(kind.param_pos(arg_pos)) && let ty::Param(param) = into_iter_param.kind() - && let Some(span) = into_iter_bound(cx, parent_fn_did, into_iter_did, param.index) + && let Some(span) = exclusive_into_iter_bound(cx, parent_fn_did, into_iter_did, param.index) && self.expn_depth == 0 { // Get the "innermost" `.into_iter()` call, e.g. given this expression: diff --git a/tests/ui/useless_conversion.fixed b/tests/ui/useless_conversion.fixed index 525919599059..13463e1782dc 100644 --- a/tests/ui/useless_conversion.fixed +++ b/tests/ui/useless_conversion.fixed @@ -151,6 +151,8 @@ fn main() { let _ = s3; let s4: Foo<'a'> = Foo; let _ = vec![s4, s4, s4].into_iter(); + + issue11300::bar(); } #[allow(dead_code)] @@ -196,6 +198,22 @@ fn explicit_into_iter_fn_arg() { b(macro_generated!()); } +mod issue11300 { + pub fn foo(i: I) + where + I: IntoIterator + ExactSizeIterator, + { + assert_eq!(i.len(), 3); + } + + pub fn bar() { + // This should not trigger the lint: + // `[i32, 3]` does not satisfy the `ExactSizeIterator` bound, so the into_iter call cannot be + // removed and is not useless. + foo([1, 2, 3].into_iter()); + } +} + #[derive(Copy, Clone)] struct Foo; diff --git a/tests/ui/useless_conversion.rs b/tests/ui/useless_conversion.rs index befb2f9a5c32..06f3160b0f3c 100644 --- a/tests/ui/useless_conversion.rs +++ b/tests/ui/useless_conversion.rs @@ -151,6 +151,8 @@ fn main() { let _ = Foo::<'a'>::from(s3); let s4: Foo<'a'> = Foo; let _ = vec![s4, s4, s4].into_iter().into_iter(); + + issue11300::bar(); } #[allow(dead_code)] @@ -196,6 +198,22 @@ fn explicit_into_iter_fn_arg() { b(macro_generated!()); } +mod issue11300 { + pub fn foo(i: I) + where + I: IntoIterator + ExactSizeIterator, + { + assert_eq!(i.len(), 3); + } + + pub fn bar() { + // This should not trigger the lint: + // `[i32, 3]` does not satisfy the `ExactSizeIterator` bound, so the into_iter call cannot be + // removed and is not useless. + foo([1, 2, 3].into_iter()); + } +} + #[derive(Copy, Clone)] struct Foo; diff --git a/tests/ui/useless_conversion.stderr b/tests/ui/useless_conversion.stderr index 28e7bb61098f..e7466e2fbdbe 100644 --- a/tests/ui/useless_conversion.stderr +++ b/tests/ui/useless_conversion.stderr @@ -119,61 +119,61 @@ LL | let _ = vec![s4, s4, s4].into_iter().into_iter(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![s4, s4, s4].into_iter()` error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/useless_conversion.rs:183:7 + --> $DIR/useless_conversion.rs:185:7 | LL | b(vec![1, 2].into_iter()); | ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `vec![1, 2]` | note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()` - --> $DIR/useless_conversion.rs:173:13 + --> $DIR/useless_conversion.rs:175:13 | LL | fn b>(_: T) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/useless_conversion.rs:184:7 + --> $DIR/useless_conversion.rs:186:7 | LL | c(vec![1, 2].into_iter()); | ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `vec![1, 2]` | note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()` - --> $DIR/useless_conversion.rs:174:18 + --> $DIR/useless_conversion.rs:176:18 | LL | fn c(_: impl IntoIterator) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/useless_conversion.rs:185:7 + --> $DIR/useless_conversion.rs:187:7 | LL | d(vec![1, 2].into_iter()); | ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `vec![1, 2]` | note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()` - --> $DIR/useless_conversion.rs:177:12 + --> $DIR/useless_conversion.rs:179:12 | LL | T: IntoIterator, | ^^^^^^^^^^^^^^^^^^^^^^^^ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/useless_conversion.rs:188:7 + --> $DIR/useless_conversion.rs:190:7 | LL | b(vec![1, 2].into_iter().into_iter()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`s: `vec![1, 2]` | note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()` - --> $DIR/useless_conversion.rs:173:13 + --> $DIR/useless_conversion.rs:175:13 | LL | fn b>(_: T) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/useless_conversion.rs:189:7 + --> $DIR/useless_conversion.rs:191:7 | LL | b(vec![1, 2].into_iter().into_iter().into_iter()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`s: `vec![1, 2]` | note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()` - --> $DIR/useless_conversion.rs:173:13 + --> $DIR/useless_conversion.rs:175:13 | LL | fn b>(_: T) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ From cf10690ad4e1c4df0794fcf072f65515cf8bbadd Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Mon, 7 Aug 2023 04:04:51 +0200 Subject: [PATCH 2/3] check that the receiver type satisfies bounds --- clippy_lints/src/useless_conversion.rs | 121 ++++++++++++------------- tests/ui/useless_conversion.fixed | 32 +++++++ tests/ui/useless_conversion.rs | 32 +++++++ tests/ui/useless_conversion.stderr | 26 +++++- 4 files changed, 148 insertions(+), 63 deletions(-) diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs index 05acf034a95d..5afb1f76df8c 100644 --- a/clippy_lints/src/useless_conversion.rs +++ b/clippy_lints/src/useless_conversion.rs @@ -1,21 +1,21 @@ -use std::ops::ControlFlow; - use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then}; use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_context}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::{is_copy, is_type_diagnostic_item, same_type_and_consts}; -use clippy_utils::{ - get_parent_expr, is_diag_trait_item, is_trait_method, is_ty_alias, match_def_path, path_to_local, paths, -}; +use clippy_utils::{get_parent_expr, is_trait_method, is_ty_alias, match_def_path, path_to_local, paths}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::def::DefKind; use rustc_hir::def_id::DefId; use rustc_hir::{BindingAnnotation, Expr, ExprKind, HirId, MatchSource, Node, PatKind}; +use rustc_infer::infer::TyCtxtInferExt; +use rustc_infer::traits::Obligation; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::{self, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor}; +use rustc_middle::traits::ObligationCause; +use rustc_middle::ty::{self, EarlyBinder, GenericArg, GenericArgsRef, Ty}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{sym, Span}; +use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt; declare_clippy_lint! { /// ### What it does @@ -66,10 +66,7 @@ impl MethodOrFunction { } /// Returns the span of the `IntoIterator` trait bound in the function pointed to by `fn_did`, -/// iff the `IntoIterator` bound is the only bound on the type parameter. -/// -/// This last part is important because it might be that the type the user is calling `.into_iter()` -/// on might not satisfy those other bounds and would result in compile errors: +/// iff all of the bounds also hold for the type of the `.into_iter()` receiver. /// ```ignore /// pub fn foo(i: I) /// where I: IntoIterator + ExactSizeIterator @@ -83,61 +80,42 @@ impl MethodOrFunction { /// ^^^^^^^^^^^^ ... here, because `[i32; 3]` is not `ExactSizeIterator` /// } /// ``` -fn exclusive_into_iter_bound( - cx: &LateContext<'_>, +fn into_iter_bound<'tcx>( + cx: &LateContext<'tcx>, fn_did: DefId, into_iter_did: DefId, + into_iter_receiver: Ty<'tcx>, param_index: u32, + node_args: GenericArgsRef<'tcx>, ) -> Option { - #[derive(Clone)] - struct ExplicitlyUsedTyParam<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - param_index: u32, - } - - impl<'a, 'tcx> TypeVisitor> for ExplicitlyUsedTyParam<'a, 'tcx> { - type BreakTy = (); - fn visit_predicate(&mut self, p: ty::Predicate<'tcx>) -> ControlFlow { - // Ignore implicit `T: Sized` bound - if let ty::PredicateKind::Clause(ty::ClauseKind::Trait(tr)) = p.kind().skip_binder() - && let Some(sized_trait_did) = self.cx.tcx.lang_items().sized_trait() - && sized_trait_did == tr.def_id() - { - return ControlFlow::Continue(()); - } - - // Ignore `::Item` projection, this use of the ty param specifically is fine - // because it's what we're already looking for - if let ty::PredicateKind::Clause(ty::ClauseKind::Projection(proj)) = p.kind().skip_binder() - && is_diag_trait_item(self.cx,proj.projection_ty.def_id, sym::IntoIterator) - { - return ControlFlow::Continue(()); - } - - p.super_visit_with(self) - } - - fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow { - if t.is_param(self.param_index) { - ControlFlow::Break(()) - } else { - ControlFlow::Continue(()) - } - } - } - + let param_env = cx.tcx.param_env(fn_did); let mut into_iter_span = None; for (pred, span) in cx.tcx.explicit_predicates_of(fn_did).predicates { - if let ty::ClauseKind::Trait(tr) = pred.kind().skip_binder() - && tr.def_id() == into_iter_did - && tr.self_ty().is_param(param_index) - { - into_iter_span = Some(*span); - } else if pred.visit_with(&mut ExplicitlyUsedTyParam { cx, param_index }).is_break() { - // Found another reference of the type parameter; conservatively assume - // that we can't remove the bound. - return None; + if let ty::ClauseKind::Trait(tr) = pred.kind().skip_binder() { + if tr.def_id() == into_iter_did && tr.self_ty().is_param(param_index) { + into_iter_span = Some(*span); + } else { + // Substitute generics in the predicate and replace the IntoIterator type parameter with the + // `.into_iter()` receiver to see if the bound also holds for that type. + let args = cx.tcx.mk_args_from_iter(node_args.iter().enumerate().map(|(i, arg)| { + if i == param_index as usize { + GenericArg::from(into_iter_receiver) + } else { + arg + } + })); + let predicate = EarlyBinder::bind(tr).instantiate(cx.tcx, args); + let obligation = Obligation::new(cx.tcx, ObligationCause::dummy(), param_env, predicate); + if !cx + .tcx + .infer_ctxt() + .build() + .predicate_must_hold_modulo_regions(&obligation) + { + return None; + } + } } } @@ -225,22 +203,41 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { // `fn_sig` does not ICE. (see #11065) && cx.tcx.opt_def_kind(did).is_some_and(DefKind::is_fn_like) => { - Some((did, args, MethodOrFunction::Function)) + Some(( + did, + args, + cx.typeck_results().node_args(recv.hir_id), + MethodOrFunction::Function + )) } ExprKind::MethodCall(.., args, _) => { cx.typeck_results().type_dependent_def_id(parent.hir_id) - .map(|did| (did, args, MethodOrFunction::Method)) + .map(|did| { + return ( + did, + args, + cx.typeck_results().node_args(recv.hir_id), + MethodOrFunction::Method + ); + }) } _ => None, }; - if let Some((parent_fn_did, args, kind)) = parent_fn + if let Some((parent_fn_did, args, node_args, kind)) = parent_fn && let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator) && let sig = cx.tcx.fn_sig(parent_fn_did).skip_binder().skip_binder() && let Some(arg_pos) = args.iter().position(|x| x.hir_id == e.hir_id) && let Some(&into_iter_param) = sig.inputs().get(kind.param_pos(arg_pos)) && let ty::Param(param) = into_iter_param.kind() - && let Some(span) = exclusive_into_iter_bound(cx, parent_fn_did, into_iter_did, param.index) + && let Some(span) = into_iter_bound( + cx, + parent_fn_did, + into_iter_did, + cx.typeck_results().expr_ty(into_iter_recv), + param.index, + node_args + ) && self.expn_depth == 0 { // Get the "innermost" `.into_iter()` call, e.g. given this expression: diff --git a/tests/ui/useless_conversion.fixed b/tests/ui/useless_conversion.fixed index 13463e1782dc..f3504bc62b45 100644 --- a/tests/ui/useless_conversion.fixed +++ b/tests/ui/useless_conversion.fixed @@ -206,11 +206,43 @@ mod issue11300 { assert_eq!(i.len(), 3); } + trait Helper {} + impl Helper for [i32; 3] {} + impl Helper for std::array::IntoIter {} + impl Helper<()> for std::array::IntoIter {} + + fn foo2(_: I) + where + I: IntoIterator + Helper, + { + } + + trait Helper2 {} + impl Helper2> for i32 {} + impl Helper2<[i32; 3]> for i32 {} + fn foo3(_: I) + where + I: IntoIterator, + i32: Helper2, + { + } + pub fn bar() { // This should not trigger the lint: // `[i32, 3]` does not satisfy the `ExactSizeIterator` bound, so the into_iter call cannot be // removed and is not useless. foo([1, 2, 3].into_iter()); + + // This should trigger the lint, receiver type [i32; 3] also implements `Helper` + foo2::([1, 2, 3]); + + // This again should *not* lint, since X = () and I = std::array::IntoIter, + // and `[i32; 3]: Helper<()>` is not true (only `std::array::IntoIter: Helper<()>` is). + foo2::<(), _>([1, 2, 3].into_iter()); + + // This should lint. Removing the `.into_iter()` means that `I` gets substituted with `[i32; 3]`, + // and `i32: Helper2<[i32, 3]>` is true, so this call is indeed unncessary. + foo3([1, 2, 3]); } } diff --git a/tests/ui/useless_conversion.rs b/tests/ui/useless_conversion.rs index 06f3160b0f3c..a4f4722927fe 100644 --- a/tests/ui/useless_conversion.rs +++ b/tests/ui/useless_conversion.rs @@ -206,11 +206,43 @@ mod issue11300 { assert_eq!(i.len(), 3); } + trait Helper {} + impl Helper for [i32; 3] {} + impl Helper for std::array::IntoIter {} + impl Helper<()> for std::array::IntoIter {} + + fn foo2(_: I) + where + I: IntoIterator + Helper, + { + } + + trait Helper2 {} + impl Helper2> for i32 {} + impl Helper2<[i32; 3]> for i32 {} + fn foo3(_: I) + where + I: IntoIterator, + i32: Helper2, + { + } + pub fn bar() { // This should not trigger the lint: // `[i32, 3]` does not satisfy the `ExactSizeIterator` bound, so the into_iter call cannot be // removed and is not useless. foo([1, 2, 3].into_iter()); + + // This should trigger the lint, receiver type [i32; 3] also implements `Helper` + foo2::([1, 2, 3].into_iter()); + + // This again should *not* lint, since X = () and I = std::array::IntoIter, + // and `[i32; 3]: Helper<()>` is not true (only `std::array::IntoIter: Helper<()>` is). + foo2::<(), _>([1, 2, 3].into_iter()); + + // This should lint. Removing the `.into_iter()` means that `I` gets substituted with `[i32; 3]`, + // and `i32: Helper2<[i32, 3]>` is true, so this call is indeed unncessary. + foo3([1, 2, 3].into_iter()); } } diff --git a/tests/ui/useless_conversion.stderr b/tests/ui/useless_conversion.stderr index e7466e2fbdbe..81a0898bdf62 100644 --- a/tests/ui/useless_conversion.stderr +++ b/tests/ui/useless_conversion.stderr @@ -178,5 +178,29 @@ note: this parameter accepts any `IntoIterator`, so you don't need to call `.int LL | fn b>(_: T) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 24 previous errors +error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` + --> $DIR/useless_conversion.rs:237:24 + | +LL | foo2::([1, 2, 3].into_iter()); + | ^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `[1, 2, 3]` + | +note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()` + --> $DIR/useless_conversion.rs:216:12 + | +LL | I: IntoIterator + Helper, + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` + --> $DIR/useless_conversion.rs:245:14 + | +LL | foo3([1, 2, 3].into_iter()); + | ^^^^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `[1, 2, 3]` + | +note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()` + --> $DIR/useless_conversion.rs:225:12 + | +LL | I: IntoIterator, + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 26 previous errors From 18f36897ef0844dc5e5a0d995356247cbee72ff3 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Tue, 5 Sep 2023 19:08:34 +0200 Subject: [PATCH 3/3] use the correct node args for substitution --- clippy_lints/src/useless_conversion.rs | 52 +++++++++++++++----------- tests/ui/useless_conversion.fixed | 41 ++++++++++++++++++++ tests/ui/useless_conversion.rs | 41 ++++++++++++++++++++ tests/ui/useless_conversion.stderr | 26 ++++++++++++- 4 files changed, 137 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs index 5afb1f76df8c..f32e7edad6cb 100644 --- a/clippy_lints/src/useless_conversion.rs +++ b/clippy_lints/src/useless_conversion.rs @@ -12,7 +12,7 @@ use rustc_infer::infer::TyCtxtInferExt; use rustc_infer::traits::Obligation; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::traits::ObligationCause; -use rustc_middle::ty::{self, EarlyBinder, GenericArg, GenericArgsRef, Ty}; +use rustc_middle::ty::{self, EarlyBinder, GenericArg, GenericArgsRef, Ty, TypeVisitableExt}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{sym, Span}; use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt; @@ -93,27 +93,35 @@ fn into_iter_bound<'tcx>( for (pred, span) in cx.tcx.explicit_predicates_of(fn_did).predicates { if let ty::ClauseKind::Trait(tr) = pred.kind().skip_binder() { - if tr.def_id() == into_iter_did && tr.self_ty().is_param(param_index) { - into_iter_span = Some(*span); - } else { - // Substitute generics in the predicate and replace the IntoIterator type parameter with the - // `.into_iter()` receiver to see if the bound also holds for that type. - let args = cx.tcx.mk_args_from_iter(node_args.iter().enumerate().map(|(i, arg)| { - if i == param_index as usize { - GenericArg::from(into_iter_receiver) - } else { - arg + if tr.self_ty().is_param(param_index) { + if tr.def_id() == into_iter_did { + into_iter_span = Some(*span); + } else { + let tr = cx.tcx.erase_regions(tr); + if tr.has_escaping_bound_vars() { + return None; + } + + // Substitute generics in the predicate and replace the IntoIterator type parameter with the + // `.into_iter()` receiver to see if the bound also holds for that type. + let args = cx.tcx.mk_args_from_iter(node_args.iter().enumerate().map(|(i, arg)| { + if i == param_index as usize { + GenericArg::from(into_iter_receiver) + } else { + arg + } + })); + + let predicate = EarlyBinder::bind(tr).instantiate(cx.tcx, args); + let obligation = Obligation::new(cx.tcx, ObligationCause::dummy(), param_env, predicate); + if !cx + .tcx + .infer_ctxt() + .build() + .predicate_must_hold_modulo_regions(&obligation) + { + return None; } - })); - let predicate = EarlyBinder::bind(tr).instantiate(cx.tcx, args); - let obligation = Obligation::new(cx.tcx, ObligationCause::dummy(), param_env, predicate); - if !cx - .tcx - .infer_ctxt() - .build() - .predicate_must_hold_modulo_regions(&obligation) - { - return None; } } } @@ -216,7 +224,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { return ( did, args, - cx.typeck_results().node_args(recv.hir_id), + cx.typeck_results().node_args(parent.hir_id), MethodOrFunction::Method ); }) diff --git a/tests/ui/useless_conversion.fixed b/tests/ui/useless_conversion.fixed index f3504bc62b45..ed8387b7eb2c 100644 --- a/tests/ui/useless_conversion.fixed +++ b/tests/ui/useless_conversion.fixed @@ -244,6 +244,47 @@ mod issue11300 { // and `i32: Helper2<[i32, 3]>` is true, so this call is indeed unncessary. foo3([1, 2, 3]); } + + fn ice() { + struct S1; + impl S1 { + pub fn foo(&self, _: I) {} + } + + S1.foo([1, 2]); + + // ICE that occured in itertools + trait Itertools { + fn interleave_shortest(self, other: J) + where + J: IntoIterator, + Self: Sized; + } + impl Itertools for I { + fn interleave_shortest(self, other: J) + where + J: IntoIterator, + Self: Sized, + { + } + } + let v0: Vec = vec![0, 2, 4]; + let v1: Vec = vec![1, 3, 5, 7]; + v0.into_iter().interleave_shortest(v1); + + trait TraitWithLifetime<'a> {} + impl<'a> TraitWithLifetime<'a> for std::array::IntoIter<&'a i32, 2> {} + + struct Helper; + impl<'a> Helper { + fn with_lt(&self, _: I) + where + I: IntoIterator + TraitWithLifetime<'a>, + { + } + } + Helper.with_lt([&1, &2].into_iter()); + } } #[derive(Copy, Clone)] diff --git a/tests/ui/useless_conversion.rs b/tests/ui/useless_conversion.rs index a4f4722927fe..991a7762fc64 100644 --- a/tests/ui/useless_conversion.rs +++ b/tests/ui/useless_conversion.rs @@ -244,6 +244,47 @@ mod issue11300 { // and `i32: Helper2<[i32, 3]>` is true, so this call is indeed unncessary. foo3([1, 2, 3].into_iter()); } + + fn ice() { + struct S1; + impl S1 { + pub fn foo(&self, _: I) {} + } + + S1.foo([1, 2].into_iter()); + + // ICE that occured in itertools + trait Itertools { + fn interleave_shortest(self, other: J) + where + J: IntoIterator, + Self: Sized; + } + impl Itertools for I { + fn interleave_shortest(self, other: J) + where + J: IntoIterator, + Self: Sized, + { + } + } + let v0: Vec = vec![0, 2, 4]; + let v1: Vec = vec![1, 3, 5, 7]; + v0.into_iter().interleave_shortest(v1.into_iter()); + + trait TraitWithLifetime<'a> {} + impl<'a> TraitWithLifetime<'a> for std::array::IntoIter<&'a i32, 2> {} + + struct Helper; + impl<'a> Helper { + fn with_lt(&self, _: I) + where + I: IntoIterator + TraitWithLifetime<'a>, + { + } + } + Helper.with_lt([&1, &2].into_iter()); + } } #[derive(Copy, Clone)] diff --git a/tests/ui/useless_conversion.stderr b/tests/ui/useless_conversion.stderr index 81a0898bdf62..c1f8b6b4aa96 100644 --- a/tests/ui/useless_conversion.stderr +++ b/tests/ui/useless_conversion.stderr @@ -202,5 +202,29 @@ note: this parameter accepts any `IntoIterator`, so you don't need to call `.int LL | I: IntoIterator, | ^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 26 previous errors +error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` + --> $DIR/useless_conversion.rs:254:16 + | +LL | S1.foo([1, 2].into_iter()); + | ^^^^^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `[1, 2]` + | +note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()` + --> $DIR/useless_conversion.rs:251:27 + | +LL | pub fn foo(&self, _: I) {} + | ^^^^^^^^^^^^ + +error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` + --> $DIR/useless_conversion.rs:273:44 + | +LL | v0.into_iter().interleave_shortest(v1.into_iter()); + | ^^^^^^^^^^^^^^ help: consider removing the `.into_iter()`: `v1` + | +note: this parameter accepts any `IntoIterator`, so you don't need to call `.into_iter()` + --> $DIR/useless_conversion.rs:260:20 + | +LL | J: IntoIterator, + | ^^^^^^^^^^^^ + +error: aborting due to 28 previous errors