Auto merge of rust-lang#11070 - y21:issue11065, r=flip1995

[`useless_conversion`]: only lint on paths to fn items and fix FP in macro Fixes rust-lang#11065 (which is actually two issues: an ICE and a false positive) It now makes sure that the function call path points to a function-like item (and not e.g. a `const` like in the linked issue), so that calling `TyCtxt::fn_sig` later in the lint does not ICE (fixes rust-lang/rust-clippy#11065 (comment)). It *also* makes sure that the expression is not part of a macro call (fixes rust-lang/rust-clippy#11065 (comment)). ~~I'm not sure if there's a better way to check this other than to walk the parent expr chain and see if any of them are expansions.~~ (edit: it doesn't do this anymore) changelog: [`useless_conversion`]: fix ICE when call receiver is a non-fn item changelog: [`useless_conversion`]: don't lint if argument is a macro argument (fixes a FP) r? `@llogiq` (reviewed rust-lang#10814, which introduced these issues)
flip1995 · Aug 17, 2023 · d068043 · d068043
1 parent 39219a6
commit d068043
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 13 deletions.
diff --git 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};
@@ -39,6 +40,7 @@ declare_clippy_lint! {
 #[derive(Default)]
 pub struct UselessConversion {
     try_desugar_arm: Vec<HirId>,
+    expn_depth: u32,
 }
 
 impl_lint_pass!(UselessConversion => [USELESS_CONVERSION]);
@@ -105,6 +107,7 @@ fn into_iter_deep_call<'hir>(cx: &LateContext<'_>, mut expr: &'hir Expr<'hir>) -
 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;
         }
 
@@ -150,9 +153,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(DefKind::is_fn_like) =>
+                            {
+                                    Some((did, args, MethodOrFunction::Function))
                             }
                             ExprKind::MethodCall(.., args, _) => {
                                 cx.typeck_results().type_dependent_def_id(parent.hir_id)
@@ -168,6 +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)
+                            && self.expn_depth == 0
                         {
                             // Get the "innermost" `.into_iter()` call, e.g. given this expression:
                             // `foo.into_iter().into_iter()`
@@ -303,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/crashes/ice-11065.rs 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<T: 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<i32>) -> Vec<i32>, (Some(5).into_iter());
+}
diff --git a/tests/ui/useless_conversion.fixed b/tests/ui/useless_conversion.fixed
@@ -155,6 +155,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<Item = i32>) {}
+
+    macro_rules! x {
+        ($e:expr) => {
+            takes_into_iter($e);
+            let _: IntoIter<i32> = $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>(_: T) {}

diff --git a/tests/ui/useless_conversion.rs b/tests/ui/useless_conversion.rs
@@ -155,6 +155,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<Item = i32>) {}
+
+    macro_rules! x {
+        ($e:expr) => {
+            takes_into_iter($e);
+            let _: IntoIter<i32> = $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>(_: T) {}

diff --git 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:171: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:161:13
+  --> $DIR/useless_conversion.rs:175:13
    |
 LL |     fn b<T: IntoIterator<Item = i32>>(_: T) {}
    |             ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:172: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:162:18
+  --> $DIR/useless_conversion.rs:176:18
    |
 LL |     fn c(_: impl IntoIterator<Item = i32>) {}
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:173: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:165:12
+  --> $DIR/useless_conversion.rs:179:12
    |
 LL |         T: IntoIterator<Item = i32>,
    |            ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:176: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:161:13
+  --> $DIR/useless_conversion.rs:175:13
    |
 LL |     fn b<T: IntoIterator<Item = i32>>(_: T) {}
    |             ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: explicit call to `.into_iter()` in function argument accepting `IntoIterator`
-  --> $DIR/useless_conversion.rs:177: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:161:13
+  --> $DIR/useless_conversion.rs:175:13
    |
 LL |     fn b<T: IntoIterator<Item = i32>>(_: T) {}
    |             ^^^^^^^^^^^^^^^^^^^^^^^^