Better detect when a field can be moved from in `while_let_on_iterator`

diff --git a/clippy_lints/src/loops/while_let_on_iterator.rs b/clippy_lints/src/loops/while_let_on_iterator.rs
index b390476a664d9e54f4c4490df4763204cc9c3265..942d6ca6b4a3afa06d22c8a029d8d6bfd014dbed 100644 (file)
--- a/clippy_lints/src/loops/while_let_on_iterator.rs
+++ b/clippy_lints/src/loops/while_let_on_iterator.rs
@@ -8,12 +8,13 @@
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor};
-use rustc_hir::{def::Res, Expr, ExprKind, HirId, Local, PatKind, QPath, UnOp};
+use rustc_hir::{def::Res, Expr, ExprKind, HirId, Local, Mutability, PatKind, QPath, UnOp};
 use rustc_lint::LateContext;
-use rustc_span::{symbol::sym, Span, Symbol};
+use rustc_middle::ty::adjustment::Adjust;
+use rustc_span::{symbol::sym, Symbol};
 
 pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-    let (scrutinee_expr, iter_expr, some_pat, loop_expr) = if_chain! {
+    let (scrutinee_expr, iter_expr_struct, iter_expr, some_pat, loop_expr) = if_chain! {
         if let Some(higher::WhileLet { if_then, let_pat, let_expr }) = higher::WhileLet::hir(expr);
         // check for `Some(..)` pattern
         if let PatKind::TupleStruct(QPath::Resolved(None, pat_path), some_pat, _) = let_pat.kind;
@@ -27,7 +28,7 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         // get the loop containing the match expression
         if !uses_iter(cx, &iter_expr_struct, if_then);
         then {
-            (let_expr, iter_expr_struct, some_pat, expr)
+            (let_expr, iter_expr_struct, iter_expr, some_pat, expr)
         } else {
             return;
         }
@@ -47,7 +48,11 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
     // If the iterator is a field or the iterator is accessed after the loop is complete it needs to be
     // borrowed mutably. TODO: If the struct can be partially moved from and the struct isn't used
     // afterwards a mutable borrow of a field isn't necessary.
-    let by_ref = if !iter_expr.fields.is_empty() || needs_mutable_borrow(cx, &iter_expr, loop_expr) {
+    let by_ref = if cx.typeck_results().expr_ty(iter_expr).ref_mutability() == Some(Mutability::Mut)
+        || !iter_expr_struct.can_move
+        || !iter_expr_struct.fields.is_empty()
+        || needs_mutable_borrow(cx, &iter_expr_struct, loop_expr)
+    {
         ".by_ref()"
     } else {
         ""
@@ -67,26 +72,36 @@ pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
 
 #[derive(Debug)]
 struct IterExpr {
-    /// The span of the whole expression, not just the path and fields stored here.
-    span: Span,
     /// The fields used, in order of child to parent.
     fields: Vec<Symbol>,
     /// The path being used.
     path: Res,
+    /// Whether or not the iterator can be moved.
+    can_move: bool,
 }
 
 /// Parses any expression to find out which field of which variable is used. Will return `None` if
 /// the expression might have side effects.
 fn try_parse_iter_expr(cx: &LateContext<'_>, mut e: &Expr<'_>) -> Option<IterExpr> {
-    let span = e.span;
     let mut fields = Vec::new();
+    let mut can_move = true;
     loop {
+        if cx
+            .typeck_results()
+            .expr_adjustments(e)
+            .iter()
+            .any(|a| matches!(a.kind, Adjust::Deref(Some(..))))
+        {
+            // Custom deref impls need to borrow the whole value as it's captured by reference
+            can_move = false;
+            fields.clear();
+        }
         match e.kind {
             ExprKind::Path(ref path) => {
                 break Some(IterExpr {
-                    span,
                     fields,
                     path: cx.qpath_res(path, e.hir_id),
+                    can_move,
                 });
             },
             ExprKind::Field(base, name) => {
@@ -99,10 +114,12 @@ fn try_parse_iter_expr(cx: &LateContext<'_>, mut e: &Expr<'_>) -> Option<IterExp
             // Shouldn't have side effects, but there's no way to trace which field is used. So forget which fields have
             // already been seen.
             ExprKind::Index(base, idx) if !idx.can_have_side_effects() => {
+                can_move = false;
                 fields.clear();
                 e = base;
             },
             ExprKind::Unary(UnOp::Deref, base) => {
+                can_move = false;
                 fields.clear();
                 e = base;
             },

diff --git a/tests/ui/while_let_on_iterator.fixed b/tests/ui/while_let_on_iterator.fixed
index 1e74ad2de655aa53c624c6c98bcf1be915f269dc..cb8892a3f009bdb99bbbc5c2ba6d4b3e4e02558e 100644 (file)
--- a/tests/ui/while_let_on_iterator.fixed
+++ b/tests/ui/while_let_on_iterator.fixed
@@ -372,6 +372,36 @@ fn exact_match_with_single_field() {
     }
 }
 
+fn custom_deref() {
+    struct S1<T> {
+        x: T,
+    }
+    struct S2<T>(S1<T>);
+    impl<T> core::ops::Deref for S2<T> {
+        type Target = S1<T>;
+        fn deref(&self) -> &Self::Target {
+            &self.0
+        }
+    }
+    impl<T> core::ops::DerefMut for S2<T> {
+        fn deref_mut(&mut self) -> &mut Self::Target {
+            &mut self.0
+        }
+    }
+
+    let mut s = S2(S1 { x: 0..10 });
+    for x in s.x.by_ref() {
+        println!("{}", x);
+    }
+}
+
+fn issue_8113() {
+    let mut x = [0..10];
+    for x in x[0].by_ref() {
+        println!("{}", x);
+    }
+}
+
 fn main() {
     let mut it = 0..20;
     for _ in it {

diff --git a/tests/ui/while_let_on_iterator.rs b/tests/ui/while_let_on_iterator.rs
index 69cb636cee8260bb31ce151a97f2ea9a11118955..d91571844959e1439fbc743d300fd037812c68f9 100644 (file)
--- a/tests/ui/while_let_on_iterator.rs
+++ b/tests/ui/while_let_on_iterator.rs
@@ -372,6 +372,36 @@ fn exact_match_with_single_field() {
     }
 }
 
+fn custom_deref() {
+    struct S1<T> {
+        x: T,
+    }
+    struct S2<T>(S1<T>);
+    impl<T> core::ops::Deref for S2<T> {
+        type Target = S1<T>;
+        fn deref(&self) -> &Self::Target {
+            &self.0
+        }
+    }
+    impl<T> core::ops::DerefMut for S2<T> {
+        fn deref_mut(&mut self) -> &mut Self::Target {
+            &mut self.0
+        }
+    }
+
+    let mut s = S2(S1 { x: 0..10 });
+    while let Some(x) = s.x.next() {
+        println!("{}", x);
+    }
+}
+
+fn issue_8113() {
+    let mut x = [0..10];
+    while let Some(x) = x[0].next() {
+        println!("{}", x);
+    }
+}
+
 fn main() {
     let mut it = 0..20;
     while let Some(..) = it.next() {

diff --git a/tests/ui/while_let_on_iterator.stderr b/tests/ui/while_let_on_iterator.stderr
index 1a11ba26eef0ffdc7c5c4d9bf75432ca9ad7aff4..fb2b0f2467c0d115924aa232f40e8d6b6706d3ef 100644 (file)
--- a/tests/ui/while_let_on_iterator.stderr
+++ b/tests/ui/while_let_on_iterator.stderr
@@ -123,10 +123,22 @@ LL |     while let Some(x) = it.0.next() {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in it.0.by_ref()`
 
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:377:5
+  --> $DIR/while_let_on_iterator.rs:393:5
+   |
+LL |     while let Some(x) = s.x.next() {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in s.x.by_ref()`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:400:5
+   |
+LL |     while let Some(x) = x[0].next() {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x in x[0].by_ref()`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:407:5
    |
 LL |     while let Some(..) = it.next() {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in it`
 
-error: aborting due to 21 previous errors
+error: aborting due to 23 previous errors