Auto merge of #10020 - samueltardieu:more-into-iter-removal, r=xFrednet

Identify more cases of useless `into_iter()` calls

changelog: Sugg: [`useless_conversion`]: Now suggests removing calls to `into_iter()` on an expression implementing `Iterator`
[#10020](#10020)
<!-- changelog_checked -->

Author: bors

clippy_lints/src/useless_conversion.rs

@@ -1,11 +1,11 @@
 use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
 use clippy_utils::source::{snippet, snippet_with_macro_callsite};
 use clippy_utils::sugg::Sugg;
-use clippy_utils::ty::{is_type_diagnostic_item, same_type_and_consts};
-use clippy_utils::{get_parent_expr, is_trait_method, match_def_path, paths};
+use clippy_utils::ty::{is_copy, is_type_diagnostic_item, same_type_and_consts};
+use clippy_utils::{get_parent_expr, is_trait_method, match_def_path, path_to_local, paths};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, HirId, MatchSource};
+use rustc_hir::{BindingAnnotation, Expr, ExprKind, HirId, MatchSource, Node, PatKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -81,16 +81,24 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
                     }
                 }
                 if is_trait_method(cx, e, sym::IntoIterator) && name.ident.name == sym::into_iter {
-                    if let Some(parent_expr) = get_parent_expr(cx, e) {
-                        if let ExprKind::MethodCall(parent_name, ..) = parent_expr.kind {
-                            if parent_name.ident.name != sym::into_iter {
-                                return;
-                            }
-                        }
+                    if get_parent_expr(cx, e).is_some() &&
+                       let Some(id) = path_to_local(recv) &&
+                       let Node::Pat(pat) = cx.tcx.hir().get(id) &&
+                       let PatKind::Binding(ann, ..) = pat.kind &&
+                       ann != BindingAnnotation::MUT
+                    {
+                        // Do not remove .into_iter() applied to a non-mutable local variable used in
+                        // a larger expression context as it would differ in mutability.
+                        return;
                     }
+
                     let a = cx.typeck_results().expr_ty(e);
                     let b = cx.typeck_results().expr_ty(recv);
-                    if same_type_and_consts(a, b) {
+
+                    // If the types are identical then .into_iter() can be removed, unless the type
+                    // implements Copy, in which case .into_iter() returns a copy of the receiver and
+                    // cannot be safely omitted.
+                    if same_type_and_consts(a, b) && !is_copy(cx, b) {
                         let sugg = snippet(cx, recv.span, "<expr>").into_owned();
                         span_lint_and_sugg(
                             cx,

tests/ui/useless_conversion.fixed

@@ -33,20 +33,87 @@ fn test_issue_3913() -> Result<(), std::io::Error> {
     Ok(())
 }
 
-fn test_issue_5833() -> Result<(), ()> {
+fn dont_lint_into_iter_on_immutable_local_implementing_iterator_in_expr() {
     let text = "foo\r\nbar\n\nbaz\n";
     let lines = text.lines();
     if Some("ok") == lines.into_iter().next() {}
+}
 
-    Ok(())
+fn lint_into_iter_on_mutable_local_implementing_iterator_in_expr() {
+    let text = "foo\r\nbar\n\nbaz\n";
+    let mut lines = text.lines();
+    if Some("ok") == lines.next() {}
+}
+
+fn lint_into_iter_on_expr_implementing_iterator() {
+    let text = "foo\r\nbar\n\nbaz\n";
+    let mut lines = text.lines();
+    if Some("ok") == lines.next() {}
+}
+
+fn lint_into_iter_on_expr_implementing_iterator_2() {
+    let text = "foo\r\nbar\n\nbaz\n";
+    if Some("ok") == text.lines().next() {}
+}
+
+#[allow(const_item_mutation)]
+fn lint_into_iter_on_const_implementing_iterator() {
+    const NUMBERS: std::ops::Range<i32> = 0..10;
+    let _ = NUMBERS.next();
+}
+
+fn lint_into_iter_on_const_implementing_iterator_2() {
+    const NUMBERS: std::ops::Range<i32> = 0..10;
+    let mut n = NUMBERS;
+    n.next();
+}
+
+#[derive(Clone, Copy)]
+struct CopiableCounter {
+    counter: u32,
+}
+
+impl Iterator for CopiableCounter {
+    type Item = u32;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        self.counter = self.counter.wrapping_add(1);
+        Some(self.counter)
+    }
+}
+
+fn dont_lint_into_iter_on_copy_iter() {
+    let mut c = CopiableCounter { counter: 0 };
+    assert_eq!(c.into_iter().next(), Some(1));
+    assert_eq!(c.into_iter().next(), Some(1));
+    assert_eq!(c.next(), Some(1));
+    assert_eq!(c.next(), Some(2));
+}
+
+fn dont_lint_into_iter_on_static_copy_iter() {
+    static mut C: CopiableCounter = CopiableCounter { counter: 0 };
+    unsafe {
+        assert_eq!(C.into_iter().next(), Some(1));
+        assert_eq!(C.into_iter().next(), Some(1));
+        assert_eq!(C.next(), Some(1));
+        assert_eq!(C.next(), Some(2));
+    }
 }
 
 fn main() {
     test_generic(10i32);
     test_generic2::<i32, i32>(10i32);
     test_questionmark().unwrap();
     test_issue_3913().unwrap();
-    test_issue_5833().unwrap();
+
+    dont_lint_into_iter_on_immutable_local_implementing_iterator_in_expr();
+    lint_into_iter_on_mutable_local_implementing_iterator_in_expr();
+    lint_into_iter_on_expr_implementing_iterator();
+    lint_into_iter_on_expr_implementing_iterator_2();
+    lint_into_iter_on_const_implementing_iterator();
+    lint_into_iter_on_const_implementing_iterator_2();
+    dont_lint_into_iter_on_copy_iter();
+    dont_lint_into_iter_on_static_copy_iter();
 
     let _: String = "foo".into();
     let _: String = From::from("foo");

tests/ui/useless_conversion.rs

@@ -33,20 +33,87 @@ fn test_issue_3913() -> Result<(), std::io::Error> {
     Ok(())
 }
 
-fn test_issue_5833() -> Result<(), ()> {
+fn dont_lint_into_iter_on_immutable_local_implementing_iterator_in_expr() {
     let text = "foo\r\nbar\n\nbaz\n";
     let lines = text.lines();
     if Some("ok") == lines.into_iter().next() {}
+}
 
-    Ok(())
+fn lint_into_iter_on_mutable_local_implementing_iterator_in_expr() {
+    let text = "foo\r\nbar\n\nbaz\n";
+    let mut lines = text.lines();
+    if Some("ok") == lines.into_iter().next() {}
+}
+
+fn lint_into_iter_on_expr_implementing_iterator() {
+    let text = "foo\r\nbar\n\nbaz\n";
+    let mut lines = text.lines().into_iter();
+    if Some("ok") == lines.next() {}
+}
+
+fn lint_into_iter_on_expr_implementing_iterator_2() {
+    let text = "foo\r\nbar\n\nbaz\n";
+    if Some("ok") == text.lines().into_iter().next() {}
+}
+
+#[allow(const_item_mutation)]
+fn lint_into_iter_on_const_implementing_iterator() {
+    const NUMBERS: std::ops::Range<i32> = 0..10;
+    let _ = NUMBERS.into_iter().next();
+}
+
+fn lint_into_iter_on_const_implementing_iterator_2() {
+    const NUMBERS: std::ops::Range<i32> = 0..10;
+    let mut n = NUMBERS.into_iter();
+    n.next();
+}
+
+#[derive(Clone, Copy)]
+struct CopiableCounter {
+    counter: u32,
+}
+
+impl Iterator for CopiableCounter {
+    type Item = u32;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        self.counter = self.counter.wrapping_add(1);
+        Some(self.counter)
+    }
+}
+
+fn dont_lint_into_iter_on_copy_iter() {
+    let mut c = CopiableCounter { counter: 0 };
+    assert_eq!(c.into_iter().next(), Some(1));
+    assert_eq!(c.into_iter().next(), Some(1));
+    assert_eq!(c.next(), Some(1));
+    assert_eq!(c.next(), Some(2));
+}
+
+fn dont_lint_into_iter_on_static_copy_iter() {
+    static mut C: CopiableCounter = CopiableCounter { counter: 0 };
+    unsafe {
+        assert_eq!(C.into_iter().next(), Some(1));
+        assert_eq!(C.into_iter().next(), Some(1));
+        assert_eq!(C.next(), Some(1));
+        assert_eq!(C.next(), Some(2));
+    }
 }
 
 fn main() {
     test_generic(10i32);
     test_generic2::<i32, i32>(10i32);
     test_questionmark().unwrap();
     test_issue_3913().unwrap();
-    test_issue_5833().unwrap();
+
+    dont_lint_into_iter_on_immutable_local_implementing_iterator_in_expr();
+    lint_into_iter_on_mutable_local_implementing_iterator_in_expr();
+    lint_into_iter_on_expr_implementing_iterator();
+    lint_into_iter_on_expr_implementing_iterator_2();
+    lint_into_iter_on_const_implementing_iterator();
+    lint_into_iter_on_const_implementing_iterator_2();
+    dont_lint_into_iter_on_copy_iter();
+    dont_lint_into_iter_on_static_copy_iter();
 
     let _: String = "foo".into();
     let _: String = From::from("foo");

tests/ui/useless_conversion.stderr

@@ -22,71 +22,101 @@ error: useless conversion to the same type: `i32`
 LL |         let _: i32 = 0i32.into();
    |                      ^^^^^^^^^^^ help: consider removing `.into()`: `0i32`
 
+error: useless conversion to the same type: `std::str::Lines<'_>`
+  --> $DIR/useless_conversion.rs:45:22
+   |
+LL |     if Some("ok") == lines.into_iter().next() {}
+   |                      ^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `lines`
+
+error: useless conversion to the same type: `std::str::Lines<'_>`
+  --> $DIR/useless_conversion.rs:50:21
+   |
+LL |     let mut lines = text.lines().into_iter();
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `text.lines()`
+
+error: useless conversion to the same type: `std::str::Lines<'_>`
+  --> $DIR/useless_conversion.rs:56:22
+   |
+LL |     if Some("ok") == text.lines().into_iter().next() {}
+   |                      ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `text.lines()`
+
+error: useless conversion to the same type: `std::ops::Range<i32>`
+  --> $DIR/useless_conversion.rs:62:13
+   |
+LL |     let _ = NUMBERS.into_iter().next();
+   |             ^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `NUMBERS`
+
+error: useless conversion to the same type: `std::ops::Range<i32>`
+  --> $DIR/useless_conversion.rs:67:17
+   |
+LL |     let mut n = NUMBERS.into_iter();
+   |                 ^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `NUMBERS`
+
 error: useless conversion to the same type: `std::string::String`
-  --> $DIR/useless_conversion.rs:61:21
+  --> $DIR/useless_conversion.rs:128:21
    |
 LL |     let _: String = "foo".to_string().into();
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `"foo".to_string()`
 
 error: useless conversion to the same type: `std::string::String`
-  --> $DIR/useless_conversion.rs:62:21
+  --> $DIR/useless_conversion.rs:129:21
    |
 LL |     let _: String = From::from("foo".to_string());
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `From::from()`: `"foo".to_string()`
 
 error: useless conversion to the same type: `std::string::String`
-  --> $DIR/useless_conversion.rs:63:13
+  --> $DIR/useless_conversion.rs:130:13
    |
 LL |     let _ = String::from("foo".to_string());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `String::from()`: `"foo".to_string()`
 
 error: useless conversion to the same type: `std::string::String`
-  --> $DIR/useless_conversion.rs:64:13
+  --> $DIR/useless_conversion.rs:131:13
    |
 LL |     let _ = String::from(format!("A: {:04}", 123));
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `String::from()`: `format!("A: {:04}", 123)`
 
 error: useless conversion to the same type: `std::str::Lines<'_>`
-  --> $DIR/useless_conversion.rs:65:13
+  --> $DIR/useless_conversion.rs:132:13
    |
 LL |     let _ = "".lines().into_iter();
    |             ^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `"".lines()`
 
 error: useless conversion to the same type: `std::vec::IntoIter<i32>`
-  --> $DIR/useless_conversion.rs:66:13
+  --> $DIR/useless_conversion.rs:133:13
    |
 LL |     let _ = vec![1, 2, 3].into_iter().into_iter();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![1, 2, 3].into_iter()`
 
 error: useless conversion to the same type: `std::string::String`
-  --> $DIR/useless_conversion.rs:67:21
+  --> $DIR/useless_conversion.rs:134:21
    |
 LL |     let _: String = format!("Hello {}", "world").into();
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into()`: `format!("Hello {}", "world")`
 
 error: useless conversion to the same type: `i32`
-  --> $DIR/useless_conversion.rs:72:13
+  --> $DIR/useless_conversion.rs:139:13
    |
 LL |     let _ = i32::from(a + b) * 3;
    |             ^^^^^^^^^^^^^^^^ help: consider removing `i32::from()`: `(a + b)`
 
 error: useless conversion to the same type: `Foo<'a'>`
-  --> $DIR/useless_conversion.rs:78:23
+  --> $DIR/useless_conversion.rs:145:23
    |
 LL |     let _: Foo<'a'> = s2.into();
    |                       ^^^^^^^^^ help: consider removing `.into()`: `s2`
 
 error: useless conversion to the same type: `Foo<'a'>`
-  --> $DIR/useless_conversion.rs:80:13
+  --> $DIR/useless_conversion.rs:147:13
    |
 LL |     let _ = Foo::<'a'>::from(s3);
    |             ^^^^^^^^^^^^^^^^^^^^ help: consider removing `Foo::<'a'>::from()`: `s3`
 
 error: useless conversion to the same type: `std::vec::IntoIter<Foo<'a'>>`
-  --> $DIR/useless_conversion.rs:82:13
+  --> $DIR/useless_conversion.rs:149:13
    |
 LL |     let _ = vec![s4, s4, s4].into_iter().into_iter();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider removing `.into_iter()`: `vec![s4, s4, s4].into_iter()`
 
-error: aborting due to 14 previous errors
+error: aborting due to 19 previous errors