From 34348f72f40b070a1a5fe5eac829e41cbd5f98e1 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Mon, 3 Jul 2023 01:12:08 +0200 Subject: [PATCH 1/3] [`useless_conversion`]: make sure path points to fn-like item --- clippy_lints/src/useless_conversion.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs index 92b694d307600..3be4fb7cc32c7 100644 --- a/clippy_lints/src/useless_conversion.rs +++ b/clippy_lints/src/useless_conversion.rs @@ -150,9 +150,14 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { { if let Some(parent) = get_parent_expr(cx, e) { let parent_fn = match parent.kind { - ExprKind::Call(recv, args) if let ExprKind::Path(ref qpath) = recv.kind => { - cx.qpath_res(qpath, recv.hir_id).opt_def_id() - .map(|did| (did, args, MethodOrFunction::Function)) + ExprKind::Call(recv, args) + if let ExprKind::Path(ref qpath) = recv.kind + && let Some(did) = cx.qpath_res(qpath, recv.hir_id).opt_def_id() + // make sure that the path indeed points to a fn-like item, so that + // `fn_sig` does not ICE. (see #11065) + && cx.tcx.opt_def_kind(did).is_some_and(|k| k.is_fn_like()) => + { + Some((did, args, MethodOrFunction::Function)) } ExprKind::MethodCall(.., args, _) => { cx.typeck_results().type_dependent_def_id(parent.hir_id) From 2820d980cb3495768babdaeeabd8aa44c3f1aaa8 Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Mon, 3 Jul 2023 01:19:09 +0200 Subject: [PATCH 2/3] [`useless_conversion`]: fix FP in macro and add test --- clippy_lints/src/useless_conversion.rs | 15 ++++++++++++++- tests/ui/crashes/ice-11065.rs | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 tests/ui/crashes/ice-11065.rs diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs index 3be4fb7cc32c7..ec8af2b83626e 100644 --- a/clippy_lints/src/useless_conversion.rs +++ b/clippy_lints/src/useless_conversion.rs @@ -5,6 +5,7 @@ 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 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}; @@ -101,6 +102,17 @@ fn into_iter_deep_call<'hir>(cx: &LateContext<'_>, mut expr: &'hir Expr<'hir>) - (expr, depth) } +/// Checks if the given `expr` is an argument of a macro invocation. +/// This is a slow-ish operation, so consider calling this late +/// to avoid slowing down the lint in the happy path when not emitting a warning +fn is_macro_argument(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + if let Some(parent) = get_parent_expr(cx, expr) { + parent.span.from_expansion() || is_macro_argument(cx, parent) + } else { + false + } +} + #[expect(clippy::too_many_lines)] impl<'tcx> LateLintPass<'tcx> for UselessConversion { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { @@ -155,7 +167,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { && let Some(did) = cx.qpath_res(qpath, recv.hir_id).opt_def_id() // make sure that the path indeed points to a fn-like item, so that // `fn_sig` does not ICE. (see #11065) - && cx.tcx.opt_def_kind(did).is_some_and(|k| k.is_fn_like()) => + && cx.tcx.opt_def_kind(did).is_some_and(DefKind::is_fn_like) => { Some((did, args, MethodOrFunction::Function)) } @@ -173,6 +185,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { && 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) + && !is_macro_argument(cx, e) { // Get the "innermost" `.into_iter()` call, e.g. given this expression: // `foo.into_iter().into_iter()` diff --git a/tests/ui/crashes/ice-11065.rs b/tests/ui/crashes/ice-11065.rs new file mode 100644 index 0000000000000..f5cf6b1cd77a2 --- /dev/null +++ b/tests/ui/crashes/ice-11065.rs @@ -0,0 +1,19 @@ +#![warn(clippy::useless_conversion)] + +use std::iter::FromIterator; +use std::option::IntoIter as OptionIter; + +fn eq(a: T, b: T) -> bool { + a == b +} + +macro_rules! tests { + ($($expr:expr, $ty:ty, ($($test:expr),*);)+) => (pub fn main() {$({ + const C: $ty = $expr; + assert!(eq(C($($test),*), $expr($($test),*))); + })+}) +} + +tests! { + FromIterator::from_iter, fn(OptionIter) -> Vec, (Some(5).into_iter()); +} From f47165c703c1f2819bb0f0aef1be1136457b2f9a Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 9 Jul 2023 22:32:58 +0200 Subject: [PATCH 3/3] find expansions more efficiently --- clippy_lints/src/useless_conversion.rs | 18 ++++++------------ tests/ui/useless_conversion.fixed | 14 ++++++++++++++ tests/ui/useless_conversion.rs | 14 ++++++++++++++ tests/ui/useless_conversion.stderr | 20 ++++++++++---------- 4 files changed, 44 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/useless_conversion.rs b/clippy_lints/src/useless_conversion.rs index ec8af2b83626e..64dfe51b059ac 100644 --- a/clippy_lints/src/useless_conversion.rs +++ b/clippy_lints/src/useless_conversion.rs @@ -40,6 +40,7 @@ declare_clippy_lint! { #[derive(Default)] pub struct UselessConversion { try_desugar_arm: Vec, + expn_depth: u32, } impl_lint_pass!(UselessConversion => [USELESS_CONVERSION]); @@ -102,21 +103,11 @@ fn into_iter_deep_call<'hir>(cx: &LateContext<'_>, mut expr: &'hir Expr<'hir>) - (expr, depth) } -/// Checks if the given `expr` is an argument of a macro invocation. -/// This is a slow-ish operation, so consider calling this late -/// to avoid slowing down the lint in the happy path when not emitting a warning -fn is_macro_argument(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if let Some(parent) = get_parent_expr(cx, expr) { - parent.span.from_expansion() || is_macro_argument(cx, parent) - } else { - false - } -} - #[expect(clippy::too_many_lines)] impl<'tcx> LateLintPass<'tcx> for UselessConversion { fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { if e.span.from_expansion() { + self.expn_depth += 1; return; } @@ -185,7 +176,7 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { && 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) - && !is_macro_argument(cx, e) + && self.expn_depth == 0 { // Get the "innermost" `.into_iter()` call, e.g. given this expression: // `foo.into_iter().into_iter()` @@ -321,5 +312,8 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion { if Some(&e.hir_id) == self.try_desugar_arm.last() { self.try_desugar_arm.pop(); } + if e.span.from_expansion() { + self.expn_depth -= 1; + } } } diff --git a/tests/ui/useless_conversion.fixed b/tests/ui/useless_conversion.fixed index d5a57ec61f7d5..5259195990592 100644 --- a/tests/ui/useless_conversion.fixed +++ b/tests/ui/useless_conversion.fixed @@ -153,6 +153,20 @@ fn main() { let _ = vec![s4, s4, s4].into_iter(); } +#[allow(dead_code)] +fn issue11065_fp() { + use std::option::IntoIter; + fn takes_into_iter(_: impl IntoIterator) {} + + macro_rules! x { + ($e:expr) => { + takes_into_iter($e); + let _: IntoIter = $e; // removing `.into_iter()` leads to a type error here + }; + } + x!(Some(5).into_iter()); +} + #[allow(dead_code)] fn explicit_into_iter_fn_arg() { fn a(_: T) {} diff --git a/tests/ui/useless_conversion.rs b/tests/ui/useless_conversion.rs index 6730e6909c5a0..befb2f9a5c32c 100644 --- a/tests/ui/useless_conversion.rs +++ b/tests/ui/useless_conversion.rs @@ -153,6 +153,20 @@ fn main() { let _ = vec![s4, s4, s4].into_iter().into_iter(); } +#[allow(dead_code)] +fn issue11065_fp() { + use std::option::IntoIter; + fn takes_into_iter(_: impl IntoIterator) {} + + macro_rules! x { + ($e:expr) => { + takes_into_iter($e); + let _: IntoIter = $e; // removing `.into_iter()` leads to a type error here + }; + } + x!(Some(5).into_iter()); +} + #[allow(dead_code)] fn explicit_into_iter_fn_arg() { fn a(_: T) {} diff --git a/tests/ui/useless_conversion.stderr b/tests/ui/useless_conversion.stderr index bf701b1e81ec7..28e7bb61098f7 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:169:7 + --> $DIR/useless_conversion.rs:183: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:159:13 + --> $DIR/useless_conversion.rs:173:13 | LL | fn b>(_: T) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/useless_conversion.rs:170:7 + --> $DIR/useless_conversion.rs:184: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:160:18 + --> $DIR/useless_conversion.rs:174:18 | LL | fn c(_: impl IntoIterator) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/useless_conversion.rs:171:7 + --> $DIR/useless_conversion.rs:185: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:163:12 + --> $DIR/useless_conversion.rs:177:12 | LL | T: IntoIterator, | ^^^^^^^^^^^^^^^^^^^^^^^^ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/useless_conversion.rs:174:7 + --> $DIR/useless_conversion.rs:188: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:159:13 + --> $DIR/useless_conversion.rs:173:13 | LL | fn b>(_: T) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ error: explicit call to `.into_iter()` in function argument accepting `IntoIterator` - --> $DIR/useless_conversion.rs:175:7 + --> $DIR/useless_conversion.rs:189: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:159:13 + --> $DIR/useless_conversion.rs:173:13 | LL | fn b>(_: T) {} | ^^^^^^^^^^^^^^^^^^^^^^^^