]> git.lizzy.rs Git - rust.git/blobdiff - clippy_lints/src/loops.rs
Auto merge of #6325 - flip1995:rustup, r=flip1995
[rust.git] / clippy_lints / src / loops.rs
index 647537933f722b1b70ae46f25a6815915681737b..0d31e9cfc3decb073691ca18766184563141b25f 100644 (file)
@@ -3,10 +3,11 @@
 use crate::utils::sugg::Sugg;
 use crate::utils::usage::{is_unused, mutated_variables};
 use crate::utils::{
-    get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait,
-    is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_trait_method,
-    match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_with_applicability, snippet_with_macro_callsite,
-    span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq,
+    contains_name, get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait,
+    indent_of, is_in_panic_handler, is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item,
+    last_path_segment, match_trait_method, match_type, match_var, multispan_sugg, qpath_res, single_segment_path,
+    snippet, snippet_with_applicability, snippet_with_macro_callsite, span_lint, span_lint_and_help,
+    span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq,
 };
 use if_chain::if_chain;
 use rustc_ast::ast;
 declare_clippy_lint! {
     /// **What it does:** Checks for empty `loop` expressions.
     ///
-    /// **Why is this bad?** Those busy loops burn CPU cycles without doing
-    /// anything. Think of the environment and either block on something or at least
-    /// make the thread sleep for some microseconds.
+    /// **Why is this bad?** These busy loops burn CPU cycles without doing
+    /// anything. It is _almost always_ a better idea to `panic!` than to have
+    /// a busy loop.
+    ///
+    /// If panicking isn't possible, think of the environment and either:
+    ///   - block on something
+    ///   - sleep the thread for some microseconds
+    ///   - yield or pause the thread
+    ///
+    /// For `std` targets, this can be done with
+    /// [`std::thread::sleep`](https://doc.rust-lang.org/std/thread/fn.sleep.html)
+    /// or [`std::thread::yield_now`](https://doc.rust-lang.org/std/thread/fn.yield_now.html).
+    ///
+    /// For `no_std` targets, doing this is more complicated, especially because
+    /// `#[panic_handler]`s can't panic. To stop/pause the thread, you will
+    /// probably need to invoke some target-specific intrinsic. Examples include:
+    ///   - [`x86_64::instructions::hlt`](https://docs.rs/x86_64/0.12.2/x86_64/instructions/fn.hlt.html)
+    ///   - [`cortex_m::asm::wfi`](https://docs.rs/cortex-m/0.6.3/cortex_m/asm/fn.wfi.html)
     ///
     /// **Known problems:** None.
     ///
     "the same item is pushed inside of a for loop"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks whether a for loop has a single element.
+    ///
+    /// **Why is this bad?** There is no reason to have a loop of a
+    /// single element.
+    /// **Known problems:** None
+    ///
+    /// **Example:**
+    /// ```rust
+    /// let item1 = 2;
+    /// for item in &[item1] {
+    ///     println!("{}", item);
+    /// }
+    /// ```
+    /// could be written as
+    /// ```rust
+    /// let item1 = 2;
+    /// let item = &item1;
+    /// println!("{}", item);
+    /// ```
+    pub SINGLE_ELEMENT_LOOP,
+    complexity,
+    "there is no reason to have a single element loop"
+}
+
 declare_lint_pass!(Loops => [
     MANUAL_MEMCPY,
     NEEDLESS_RANGE_LOOP,
     MUT_RANGE_BOUND,
     WHILE_IMMUTABLE_CONDITION,
     SAME_ITEM_PUSH,
+    SINGLE_ELEMENT_LOOP,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Loops {
@@ -501,15 +543,15 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         // (also matches an explicit "match" instead of "if let")
         // (even if the "match" or "if let" is used for declaration)
         if let ExprKind::Loop(ref block, _, LoopSource::Loop) = expr.kind {
-            // also check for empty `loop {}` statements
-            if block.stmts.is_empty() && block.expr.is_none() && !is_no_std_crate(cx.tcx.hir().krate()) {
-                span_lint(
-                    cx,
-                    EMPTY_LOOP,
-                    expr.span,
-                    "empty `loop {}` detected. You may want to either use `panic!()` or add \
-                     `std::thread::sleep(..);` to the loop body.",
-                );
+            // also check for empty `loop {}` statements, skipping those in #[panic_handler]
+            if block.stmts.is_empty() && block.expr.is_none() && !is_in_panic_handler(cx, expr) {
+                let msg = "empty `loop {}` wastes CPU cycles";
+                let help = if is_no_std_crate(cx.tcx.hir().krate()) {
+                    "you should either use `panic!()` or add a call pausing or sleeping the thread to the loop body"
+                } else {
+                    "you should either use `panic!()` or add `std::thread::sleep(..);` to the loop body"
+                };
+                span_lint_and_help(cx, EMPTY_LOOP, expr.span, msg, None, help);
             }
 
             // extract the expression from the first statement (if any) in a block
@@ -575,9 +617,9 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
                 }
 
                 let lhs_constructor = last_path_segment(qpath);
-                if method_path.ident.name == sym!(next)
+                if method_path.ident.name == sym::next
                     && match_trait_method(cx, match_expr, &paths::ITERATOR)
-                    && lhs_constructor.ident.name == sym!(Some)
+                    && lhs_constructor.ident.name == sym::Some
                     && (pat_args.is_empty()
                         || !is_refutable(cx, &pat_args[0])
                             && !is_used_inside(cx, iter_expr, &arms[0].body)
@@ -741,6 +783,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult {
         | ExprKind::Closure(_, _, _, _, _)
         | ExprKind::LlvmInlineAsm(_)
         | ExprKind::Path(_)
+        | ExprKind::ConstBlock(_)
         | ExprKind::Lit(_)
         | ExprKind::Err => NeverLoopResult::Otherwise,
     }
@@ -768,15 +811,29 @@ fn check_for_loop<'tcx>(
     body: &'tcx Expr<'_>,
     expr: &'tcx Expr<'_>,
 ) {
-    check_for_loop_range(cx, pat, arg, body, expr);
+    let is_manual_memcpy_triggered = detect_manual_memcpy(cx, pat, arg, body, expr);
+    if !is_manual_memcpy_triggered {
+        check_for_loop_range(cx, pat, arg, body, expr);
+        check_for_loop_explicit_counter(cx, pat, arg, body, expr);
+    }
     check_for_loop_arg(cx, pat, arg, expr);
-    check_for_loop_explicit_counter(cx, pat, arg, body, expr);
     check_for_loop_over_map_kv(cx, pat, arg, body, expr);
     check_for_mut_range_bound(cx, arg, body);
-    detect_manual_memcpy(cx, pat, arg, body, expr);
+    check_for_single_element_loop(cx, pat, arg, body, expr);
     detect_same_item_push(cx, pat, arg, body, expr);
 }
 
+// this function assumes the given expression is a `for` loop.
+fn get_span_of_entire_for_loop(expr: &Expr<'_>) -> Span {
+    // for some reason this is the only way to get the `Span`
+    // of the entire `for` loop
+    if let ExprKind::Match(_, arms, _) = &expr.kind {
+        arms[0].body.span
+    } else {
+        unreachable!()
+    }
+}
+
 fn same_var<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, var: HirId) -> bool {
     if_chain! {
         if let ExprKind::Path(qpath) = &expr.kind;
@@ -792,69 +849,27 @@ fn same_var<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, var: HirId) -> bool {
     }
 }
 
-#[derive(Clone, Copy)]
-enum OffsetSign {
-    Positive,
-    Negative,
-}
-
-struct Offset {
-    value: MinifyingSugg<'static>,
-    sign: OffsetSign,
-}
-
-impl Offset {
-    fn negative(value: MinifyingSugg<'static>) -> Self {
-        Self {
-            value,
-            sign: OffsetSign::Negative,
-        }
-    }
-
-    fn positive(value: MinifyingSugg<'static>) -> Self {
-        Self {
-            value,
-            sign: OffsetSign::Positive,
-        }
-    }
-
-    fn empty() -> Self {
-        Self::positive(MinifyingSugg::non_paren("0"))
-    }
-}
-
-fn apply_offset(lhs: &MinifyingSugg<'static>, rhs: &Offset) -> MinifyingSugg<'static> {
-    match rhs.sign {
-        OffsetSign::Positive => lhs + &rhs.value,
-        OffsetSign::Negative => lhs - &rhs.value,
-    }
-}
-
+/// a wrapper of `Sugg`. Besides what `Sugg` do, this removes unnecessary `0`;
+/// and also, it avoids subtracting a variable from the same one by replacing it with `0`.
+/// it exists for the convenience of the overloaded operators while normal functions can do the
+/// same.
 #[derive(Clone)]
-struct MinifyingSugg<'a>(sugg::Sugg<'a>);
-
-impl std::fmt::Display for MinifyingSugg<'_> {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
-        std::fmt::Display::fmt(&self.0, f)
-    }
-}
+struct MinifyingSugg<'a>(Sugg<'a>);
 
 impl<'a> MinifyingSugg<'a> {
     fn as_str(&self) -> &str {
-        let sugg::Sugg::NonParen(s) | sugg::Sugg::MaybeParen(s) | sugg::Sugg::BinOp(_, s) = &self.0;
+        let Sugg::NonParen(s) | Sugg::MaybeParen(s) | Sugg::BinOp(_, s) = &self.0;
         s.as_ref()
     }
 
-    fn hir(cx: &LateContext<'_>, expr: &Expr<'_>, default: &'a str) -> Self {
-        Self(sugg::Sugg::hir(cx, expr, default))
-    }
-
-    fn maybe_par(self) -> Self {
-        Self(self.0.maybe_par())
+    fn into_sugg(self) -> Sugg<'a> {
+        self.0
     }
+}
 
-    fn non_paren(str: impl Into<std::borrow::Cow<'a, str>>) -> Self {
-        Self(sugg::Sugg::NonParen(str.into()))
+impl<'a> From<Sugg<'a>> for MinifyingSugg<'a> {
+    fn from(sugg: Sugg<'a>) -> Self {
+        Self(sugg)
     }
 }
 
@@ -864,7 +879,7 @@ fn add(self, rhs: &MinifyingSugg<'static>) -> MinifyingSugg<'static> {
         match (self.as_str(), rhs.as_str()) {
             ("0", _) => rhs.clone(),
             (_, "0") => self.clone(),
-            (_, _) => MinifyingSugg(&self.0 + &rhs.0),
+            (_, _) => (&self.0 + &rhs.0).into(),
         }
     }
 }
@@ -874,9 +889,9 @@ impl std::ops::Sub for &MinifyingSugg<'static> {
     fn sub(self, rhs: &MinifyingSugg<'static>) -> MinifyingSugg<'static> {
         match (self.as_str(), rhs.as_str()) {
             (_, "0") => self.clone(),
-            ("0", _) => MinifyingSugg(-(rhs.0.clone())),
-            (x, y) if x == y => MinifyingSugg::non_paren("0"),
-            (_, _) => MinifyingSugg(&self.0 - &rhs.0),
+            ("0", _) => (-rhs.0.clone()).into(),
+            (x, y) if x == y => sugg::ZERO.into(),
+            (_, _) => (&self.0 - &rhs.0).into(),
         }
     }
 }
@@ -887,7 +902,7 @@ fn add(self, rhs: &MinifyingSugg<'static>) -> MinifyingSugg<'static> {
         match (self.as_str(), rhs.as_str()) {
             ("0", _) => rhs.clone(),
             (_, "0") => self,
-            (_, _) => MinifyingSugg(self.0 + &rhs.0),
+            (_, _) => (self.0 + &rhs.0).into(),
         }
     }
 }
@@ -897,13 +912,53 @@ impl std::ops::Sub<&MinifyingSugg<'static>> for MinifyingSugg<'static> {
     fn sub(self, rhs: &MinifyingSugg<'static>) -> MinifyingSugg<'static> {
         match (self.as_str(), rhs.as_str()) {
             (_, "0") => self,
-            ("0", _) => MinifyingSugg(-(rhs.0.clone())),
-            (x, y) if x == y => MinifyingSugg::non_paren("0"),
-            (_, _) => MinifyingSugg(self.0 - &rhs.0),
+            ("0", _) => (-rhs.0.clone()).into(),
+            (x, y) if x == y => sugg::ZERO.into(),
+            (_, _) => (self.0 - &rhs.0).into(),
         }
     }
 }
 
+/// a wrapper around `MinifyingSugg`, which carries a operator like currying
+/// so that the suggested code become more efficient (e.g. `foo + -bar` `foo - bar`).
+struct Offset {
+    value: MinifyingSugg<'static>,
+    sign: OffsetSign,
+}
+
+#[derive(Clone, Copy)]
+enum OffsetSign {
+    Positive,
+    Negative,
+}
+
+impl Offset {
+    fn negative(value: Sugg<'static>) -> Self {
+        Self {
+            value: value.into(),
+            sign: OffsetSign::Negative,
+        }
+    }
+
+    fn positive(value: Sugg<'static>) -> Self {
+        Self {
+            value: value.into(),
+            sign: OffsetSign::Positive,
+        }
+    }
+
+    fn empty() -> Self {
+        Self::positive(sugg::ZERO)
+    }
+}
+
+fn apply_offset(lhs: &MinifyingSugg<'static>, rhs: &Offset) -> MinifyingSugg<'static> {
+    match rhs.sign {
+        OffsetSign::Positive => lhs + &rhs.value,
+        OffsetSign::Negative => lhs - &rhs.value,
+    }
+}
+
 #[derive(Debug, Clone, Copy)]
 enum StartKind<'hir> {
     Range,
@@ -928,13 +983,13 @@ fn is_slice_like<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'_>) -> bool {
         _ => false,
     };
 
-    is_slice || is_type_diagnostic_item(cx, ty, sym!(vec_type)) || is_type_diagnostic_item(cx, ty, sym!(vecdeque_type))
+    is_slice || is_type_diagnostic_item(cx, ty, sym::vec_type) || is_type_diagnostic_item(cx, ty, sym!(vecdeque_type))
 }
 
 fn fetch_cloned_expr<'tcx>(expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
     if_chain! {
         if let ExprKind::MethodCall(method, _, args, _) = expr.kind;
-        if method.ident.name == sym!(clone);
+        if method.ident.name == sym::clone;
         if args.len() == 1;
         if let Some(arg) = args.get(0);
         then { arg } else { expr }
@@ -956,20 +1011,13 @@ fn get_start<'tcx>(cx: &LateContext<'tcx>, e: &Expr<'_>, starts: &[Start<'tcx>])
         })
     }
 
-    fn get_offset<'tcx>(
-        cx: &LateContext<'tcx>,
-        e: &Expr<'_>,
-        starts: &[Start<'tcx>],
-    ) -> Option<MinifyingSugg<'static>> {
+    fn get_offset<'tcx>(cx: &LateContext<'tcx>, e: &Expr<'_>, starts: &[Start<'tcx>]) -> Option<Sugg<'static>> {
         match &e.kind {
             ExprKind::Lit(l) => match l.node {
-                ast::LitKind::Int(x, _ty) => Some(MinifyingSugg::non_paren(x.to_string())),
+                ast::LitKind::Int(x, _ty) => Some(Sugg::NonParen(x.to_string().into())),
                 _ => None,
             },
-            ExprKind::Path(..) if get_start(cx, e, starts).is_none() => {
-                // `e` is always non paren as it's a `Path`
-                Some(MinifyingSugg::non_paren(snippet(cx, e.span, "???")))
-            },
+            ExprKind::Path(..) if get_start(cx, e, starts).is_none() => Some(Sugg::hir(cx, e, "???")),
             _ => None,
         }
     }
@@ -1001,24 +1049,37 @@ fn get_assignment<'tcx>(e: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, &'tcx
     }
 }
 
+/// Get assignments from the given block.
+/// The returned iterator yields `None` if no assignment expressions are there,
+/// filtering out the increments of the given whitelisted loop counters;
+/// because its job is to make sure there's nothing other than assignments and the increments.
 fn get_assignments<'a: 'c, 'tcx: 'c, 'c>(
     cx: &'a LateContext<'tcx>,
-    stmts: &'tcx [Stmt<'tcx>],
-    expr: Option<&'tcx Expr<'tcx>>,
+    Block { stmts, expr, .. }: &'tcx Block<'tcx>,
     loop_counters: &'c [Start<'tcx>],
 ) -> impl Iterator<Item = Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)>> + 'c {
+    // As the `filter` and `map` below do different things, I think putting together
+    // just increases complexity. (cc #3188 and #4193)
+    #[allow(clippy::filter_map)]
     stmts
         .iter()
         .filter_map(move |stmt| match stmt.kind {
             StmtKind::Local(..) | StmtKind::Item(..) => None,
-            StmtKind::Expr(e) | StmtKind::Semi(e) => if_chain! {
-                if let ExprKind::AssignOp(_, var, _) = e.kind;
-                // skip StartKind::Range
-                if loop_counters.iter().skip(1).any(|counter| Some(counter.id) == var_def_id(cx, var));
-                then { None } else { Some(e) }
-            },
+            StmtKind::Expr(e) | StmtKind::Semi(e) => Some(e),
+        })
+        .chain((*expr).into_iter())
+        .filter(move |e| {
+            if let ExprKind::AssignOp(_, place, _) = e.kind {
+                !loop_counters
+                    .iter()
+                    // skip the first item which should be `StartKind::Range`
+                    // this makes it possible to use the slice with `StartKind::Range` in the same iterator loop.
+                    .skip(1)
+                    .any(|counter| same_var(cx, place, counter.id))
+            } else {
+                true
+            }
         })
-        .chain(expr.into_iter())
         .map(get_assignment)
 }
 
@@ -1059,60 +1120,61 @@ fn build_manual_memcpy_suggestion<'tcx>(
 ) -> String {
     fn print_offset(offset: MinifyingSugg<'static>) -> MinifyingSugg<'static> {
         if offset.as_str() == "0" {
-            MinifyingSugg::non_paren("")
+            sugg::EMPTY.into()
         } else {
             offset
         }
     }
 
-    let print_limit =
-        |end: &Expr<'_>, end_str: &str, base: &Expr<'_>, sugg: MinifyingSugg<'static>| -> MinifyingSugg<'static> {
-            if_chain! {
-                if let ExprKind::MethodCall(method, _, len_args, _) = end.kind;
-                if method.ident.name == sym!(len);
-                if len_args.len() == 1;
-                if let Some(arg) = len_args.get(0);
-                if var_def_id(cx, arg) == var_def_id(cx, base);
-                then {
-                    if sugg.as_str() == end_str {
-                        MinifyingSugg::non_paren("")
-                    } else {
-                        sugg
-                    }
+    let print_limit = |end: &Expr<'_>, end_str: &str, base: &Expr<'_>, sugg: MinifyingSugg<'static>| {
+        if_chain! {
+            if let ExprKind::MethodCall(method, _, len_args, _) = end.kind;
+            if method.ident.name == sym!(len);
+            if len_args.len() == 1;
+            if let Some(arg) = len_args.get(0);
+            if var_def_id(cx, arg) == var_def_id(cx, base);
+            then {
+                if sugg.as_str() == end_str {
+                    sugg::EMPTY.into()
                 } else {
-                    match limits {
-                        ast::RangeLimits::Closed => {
-                            sugg + &MinifyingSugg::non_paren("1")
-                        },
-                        ast::RangeLimits::HalfOpen => sugg,
-                    }
+                    sugg
+                }
+            } else {
+                match limits {
+                    ast::RangeLimits::Closed => {
+                        sugg + &sugg::ONE.into()
+                    },
+                    ast::RangeLimits::HalfOpen => sugg,
                 }
             }
-        };
+        }
+    };
 
-    let start_str = MinifyingSugg::hir(cx, start, "");
-    let end_str = MinifyingSugg::hir(cx, end, "");
+    let start_str = Sugg::hir(cx, start, "").into();
+    let end_str: MinifyingSugg<'_> = Sugg::hir(cx, end, "").into();
 
     let print_offset_and_limit = |idx_expr: &IndexExpr<'_>| match idx_expr.idx {
         StartKind::Range => (
-            print_offset(apply_offset(&start_str, &idx_expr.idx_offset)),
+            print_offset(apply_offset(&start_str, &idx_expr.idx_offset)).into_sugg(),
             print_limit(
                 end,
                 end_str.as_str(),
                 idx_expr.base,
                 apply_offset(&end_str, &idx_expr.idx_offset),
-            ),
+            )
+            .into_sugg(),
         ),
         StartKind::Counter { initializer } => {
-            let counter_start = MinifyingSugg::hir(cx, initializer, "");
+            let counter_start = Sugg::hir(cx, initializer, "").into();
             (
-                print_offset(apply_offset(&counter_start, &idx_expr.idx_offset)),
+                print_offset(apply_offset(&counter_start, &idx_expr.idx_offset)).into_sugg(),
                 print_limit(
                     end,
                     end_str.as_str(),
                     idx_expr.base,
                     apply_offset(&end_str, &idx_expr.idx_offset) + &counter_start - &start_str,
-                ),
+                )
+                .into_sugg(),
             )
         },
     };
@@ -1123,7 +1185,7 @@ fn print_offset(offset: MinifyingSugg<'static>) -> MinifyingSugg<'static> {
     let dst_base_str = snippet(cx, dst.base.span, "???");
     let src_base_str = snippet(cx, src.base.span, "???");
 
-    let dst = if dst_offset.as_str() == "" && dst_limit.as_str() == "" {
+    let dst = if dst_offset == sugg::EMPTY && dst_limit == sugg::EMPTY {
         dst_base_str
     } else {
         format!(
@@ -1136,7 +1198,7 @@ fn print_offset(offset: MinifyingSugg<'static>) -> MinifyingSugg<'static> {
     };
 
     format!(
-        "{}.clone_from_slice(&{}[{}..{}])",
+        "{}.clone_from_slice(&{}[{}..{}]);",
         dst,
         src_base_str,
         src_offset.maybe_par(),
@@ -1152,7 +1214,7 @@ fn detect_manual_memcpy<'tcx>(
     arg: &'tcx Expr<'_>,
     body: &'tcx Expr<'_>,
     expr: &'tcx Expr<'_>,
-) {
+) -> bool {
     if let Some(higher::Range {
         start: Some(start),
         end: Some(end),
@@ -1175,16 +1237,16 @@ fn detect_manual_memcpy<'tcx>(
                 if let Some(loop_counters) = get_loop_counters(cx, block, expr) {
                     starts.extend(loop_counters);
                 }
-                iter_a = Some(get_assignments(cx, block.stmts, block.expr, &starts));
+                iter_a = Some(get_assignments(cx, block, &starts));
             } else {
                 iter_b = Some(get_assignment(body));
             }
 
-            // The only statements in the for loops can be indexed assignments from
-            // indexed retrievals.
             let assignments = iter_a.into_iter().flatten().chain(iter_b.into_iter());
 
             let big_sugg = assignments
+                // The only statements in the for loops can be indexed assignments from
+                // indexed retrievals (except increments of loop counters).
                 .map(|o| {
                     o.and_then(|(lhs, rhs)| {
                         let rhs = fetch_cloned_expr(rhs);
@@ -1216,15 +1278,17 @@ fn detect_manual_memcpy<'tcx>(
                 span_lint_and_sugg(
                     cx,
                     MANUAL_MEMCPY,
-                    expr.span,
+                    get_span_of_entire_for_loop(expr),
                     "it looks like you're manually copying between slices",
                     "try replacing the loop by",
                     big_sugg,
                     Applicability::Unspecified,
                 );
+                return true;
             }
         }
     }
+    false
 }
 
 // Scans the body of the for loop and determines whether lint should be given
@@ -1289,7 +1353,7 @@ fn get_vec_push<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) -> Option<(&
             if let Some(self_expr) = args.get(0);
             if let Some(pushed_item) = args.get(1);
             // Check that the method being called is push() on a Vec
-            if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_expr), sym!(vec_type));
+            if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(self_expr), sym::vec_type);
             if path.ident.name.as_str() == "push";
             then {
                 return Some((self_expr, pushed_item))
@@ -1451,6 +1515,8 @@ fn check_for_loop_range<'tcx>(
 
                 let skip = if starts_at_zero {
                     String::new()
+                } else if visitor.indexed_mut.contains(&indexed) && contains_name(indexed, start) {
+                    return;
                 } else {
                     format!(".skip({})", snippet(cx, start.span, ".."))
                 };
@@ -1477,6 +1543,8 @@ fn check_for_loop_range<'tcx>(
 
                     if is_len_call(end, indexed) || is_end_eq_array_len(cx, end, limits, indexed_ty) {
                         String::new()
+                    } else if visitor.indexed_mut.contains(&indexed) && contains_name(indexed, take_expr) {
+                        return;
                     } else {
                         match limits {
                             ast::RangeLimits::Closed => {
@@ -1666,7 +1734,7 @@ fn check_for_loop_arg(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>, expr:
 /// Checks for `for` loops over `Option`s and `Result`s.
 fn check_arg_type(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
     let ty = cx.typeck_results().expr_ty(arg);
-    if is_type_diagnostic_item(cx, ty, sym!(option_type)) {
+    if is_type_diagnostic_item(cx, ty, sym::option_type) {
         span_lint_and_help(
             cx,
             FOR_LOOPS_OVER_FALLIBLES,
@@ -1683,7 +1751,7 @@ fn check_arg_type(cx: &LateContext<'_>, pat: &Pat<'_>, arg: &Expr<'_>) {
                 snippet(cx, arg.span, "_")
             ),
         );
-    } else if is_type_diagnostic_item(cx, ty, sym!(result_type)) {
+    } else if is_type_diagnostic_item(cx, ty, sym::result_type) {
         span_lint_and_help(
             cx,
             FOR_LOOPS_OVER_FALLIBLES,
@@ -1730,13 +1798,7 @@ fn check_for_loop_explicit_counter<'tcx>(
                 then {
                     let mut applicability = Applicability::MachineApplicable;
 
-                    // for some reason this is the only way to get the `Span`
-                    // of the entire `for` loop
-                    let for_span = if let ExprKind::Match(_, arms, _) = &expr.kind {
-                        arms[0].body.span
-                    } else {
-                        unreachable!()
-                    };
+                    let for_span = get_span_of_entire_for_loop(expr);
 
                     span_lint_and_sugg(
                         cx,
@@ -1847,6 +1909,43 @@ fn check_for_loop_over_map_kv<'tcx>(
     }
 }
 
+fn check_for_single_element_loop<'tcx>(
+    cx: &LateContext<'tcx>,
+    pat: &'tcx Pat<'_>,
+    arg: &'tcx Expr<'_>,
+    body: &'tcx Expr<'_>,
+    expr: &'tcx Expr<'_>,
+) {
+    if_chain! {
+        if let ExprKind::AddrOf(BorrowKind::Ref, _, ref arg_expr) = arg.kind;
+        if let PatKind::Binding(.., target, _) = pat.kind;
+        if let ExprKind::Array(ref arg_expr_list) = arg_expr.kind;
+        if let [arg_expression] = arg_expr_list;
+        if let ExprKind::Path(ref list_item) = arg_expression.kind;
+        if let Some(list_item_name) = single_segment_path(list_item).map(|ps| ps.ident.name);
+        if let ExprKind::Block(ref block, _) = body.kind;
+        if !block.stmts.is_empty();
+
+        then {
+            let for_span = get_span_of_entire_for_loop(expr);
+            let mut block_str = snippet(cx, block.span, "..").into_owned();
+            block_str.remove(0);
+            block_str.pop();
+
+
+            span_lint_and_sugg(
+                cx,
+                SINGLE_ELEMENT_LOOP,
+                for_span,
+                "for loop over a single element",
+                "try",
+                format!("{{\n{}let {} = &{};{}}}", " ".repeat(indent_of(cx, block.stmts[0].span).unwrap_or(0)), target.name, list_item_name, block_str),
+                Applicability::MachineApplicable
+            )
+        }
+    }
+}
+
 struct MutatePairDelegate<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
     hir_id_low: Option<HirId>,
@@ -1856,28 +1955,28 @@ struct MutatePairDelegate<'a, 'tcx> {
 }
 
 impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, 'tcx> {
-    fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: ConsumeMode) {}
+    fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: ConsumeMode) {}
 
-    fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, bk: ty::BorrowKind) {
+    fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) {
         if let ty::BorrowKind::MutBorrow = bk {
             if let PlaceBase::Local(id) = cmt.place.base {
                 if Some(id) == self.hir_id_low {
-                    self.span_low = Some(self.cx.tcx.hir().span(cmt.hir_id))
+                    self.span_low = Some(self.cx.tcx.hir().span(diag_expr_id))
                 }
                 if Some(id) == self.hir_id_high {
-                    self.span_high = Some(self.cx.tcx.hir().span(cmt.hir_id))
+                    self.span_high = Some(self.cx.tcx.hir().span(diag_expr_id))
                 }
             }
         }
     }
 
-    fn mutate(&mut self, cmt: &PlaceWithHirId<'tcx>) {
+    fn mutate(&mut self, cmt: &PlaceWithHirId<'tcx>, diag_expr_id: HirId) {
         if let PlaceBase::Local(id) = cmt.place.base {
             if Some(id) == self.hir_id_low {
-                self.span_low = Some(self.cx.tcx.hir().span(cmt.hir_id))
+                self.span_low = Some(self.cx.tcx.hir().span(diag_expr_id))
             }
             if Some(id) == self.hir_id_high {
-                self.span_high = Some(self.cx.tcx.hir().span(cmt.hir_id))
+                self.span_high = Some(self.cx.tcx.hir().span(diag_expr_id))
             }
         }
     }
@@ -1950,12 +2049,11 @@ fn check_for_mutation<'tcx>(
         span_low: None,
         span_high: None,
     };
-    let def_id = body.hir_id.owner.to_def_id();
     cx.tcx.infer_ctxt().enter(|infcx| {
         ExprUseVisitor::new(
             &mut delegate,
             &infcx,
-            def_id.expect_local(),
+            body.hir_id.owner,
             cx.param_env,
             cx.typeck_results(),
         )
@@ -2086,8 +2184,8 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
         if_chain! {
             // a range index op
             if let ExprKind::MethodCall(ref meth, _, ref args, _) = expr.kind;
-            if (meth.ident.name == sym!(index) && match_trait_method(self.cx, expr, &paths::INDEX))
-                || (meth.ident.name == sym!(index_mut) && match_trait_method(self.cx, expr, &paths::INDEX_MUT));
+            if (meth.ident.name == sym::index && match_trait_method(self.cx, expr, &paths::INDEX))
+                || (meth.ident.name == sym::index_mut && match_trait_method(self.cx, expr, &paths::INDEX_MUT));
             if !self.check(&args[1], &args[0], expr);
             then { return }
         }
@@ -2233,7 +2331,7 @@ fn is_ref_iterable_type(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
     // will allow further borrows afterwards
     let ty = cx.typeck_results().expr_ty(e);
     is_iterable_array(ty, cx) ||
-    is_type_diagnostic_item(cx, ty, sym!(vec_type)) ||
+    is_type_diagnostic_item(cx, ty, sym::vec_type) ||
     match_type(cx, ty, &paths::LINKED_LIST) ||
     is_type_diagnostic_item(cx, ty, sym!(hashmap_type)) ||
     is_type_diagnostic_item(cx, ty, sym!(hashset_type)) ||
@@ -2790,7 +2888,7 @@ fn check_needless_collect_direct_usage<'tcx>(expr: &'tcx Expr<'_>, cx: &LateCont
         if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0);
         then {
             let ty = cx.typeck_results().node_type(ty.hir_id);
-            if is_type_diagnostic_item(cx, ty, sym!(vec_type)) ||
+            if is_type_diagnostic_item(cx, ty, sym::vec_type) ||
                 is_type_diagnostic_item(cx, ty, sym!(vecdeque_type)) ||
                 match_type(cx, ty, &paths::BTREEMAP) ||
                 is_type_diagnostic_item(cx, ty, sym!(hashmap_type)) {
@@ -2901,7 +2999,14 @@ fn get_iter_method(&self, cx: &LateContext<'_>) -> String {
             IterFunctionKind::IntoIter => String::new(),
             IterFunctionKind::Len => String::from(".count()"),
             IterFunctionKind::IsEmpty => String::from(".next().is_none()"),
-            IterFunctionKind::Contains(span) => format!(".any(|x| x == {})", snippet(cx, *span, "..")),
+            IterFunctionKind::Contains(span) => {
+                let s = snippet(cx, *span, "..");
+                if let Some(stripped) = s.strip_prefix('&') {
+                    format!(".any(|x| x == {})", stripped)
+                } else {
+                    format!(".any(|x| x == *{})", s)
+                }
+            },
         }
     }
     fn get_suggestion_text(&self) -> &'static str {
@@ -2982,7 +3087,7 @@ fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
     }
 }
 
-/// Detect the occurences of calls to `iter` or `into_iter` for the
+/// Detect the occurrences of calls to `iter` or `into_iter` for the
 /// given identifier
 fn detect_iter_and_into_iters<'tcx>(block: &'tcx Block<'tcx>, identifier: Ident) -> Option<Vec<IterFunction>> {
     let mut visitor = IterFunctionVisitor {