Auto merge of rust-lang#7607 - dswij:mut-range-bound-break, r=flip1995

bors · bors · commit f35678401c63 · 2021-09-08T08:14:17.000Z
`mut_range_bound` check for immediate break after mutation closes rust-lang#7532 `mut_range_bound` ignores mutation on range bounds that is placed immediately before break. Still warns if the break is not always reachable. changelog: [`mut_range_bound`] ignore range bound mutations before immediate break
diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs
@@ -397,6 +397,21 @@ declare_clippy_lint! {
     /// ### Why is this bad?
     /// One might think that modifying the mutable variable changes the loop bounds
     ///
+    /// ### Known problems
+    /// False positive when mutation is followed by a `break`, but the `break` is not immediately
+    /// after the mutation:
+    ///
+    /// ```rust
+    /// let mut x = 5;
+    /// for _ in 0..x {
+    ///     x += 1; // x is a range bound that is mutated
+    ///     ..; // some other expression
+    ///     break; // leaves the loop, so mutation is not an issue
+    /// }
+    /// ```
+    ///
+    /// False positive on nested loops ([#6072](https://github.com/rust-lang/rust-clippy/issues/6072))
+    ///
     /// ### Example
     /// ```rust
     /// let mut foo = 42;
diff --git a/clippy_lints/src/loops/mut_range_bound.rs b/clippy_lints/src/loops/mut_range_bound.rs
@@ -1,24 +1,27 @@
 use super::MUT_RANGE_BOUND;
-use clippy_utils::diagnostics::span_lint;
-use clippy_utils::{higher, path_to_local};
+use clippy_utils::diagnostics::span_lint_and_note;
+use clippy_utils::{get_enclosing_block, higher, path_to_local};
 use if_chain::if_chain;
-use rustc_hir::{BindingAnnotation, Expr, HirId, Node, PatKind};
+use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
+use rustc_hir::{BindingAnnotation, Expr, ExprKind, HirId, Node, PatKind};
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
+use rustc_middle::hir::map::Map;
 use rustc_middle::{mir::FakeReadCause, ty};
 use rustc_span::source_map::Span;
 use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
 
 pub(super) fn check(cx: &LateContext<'_>, arg: &Expr<'_>, body: &Expr<'_>) {
-    if let Some(higher::Range {
-        start: Some(start),
-        end: Some(end),
-        ..
-    }) = higher::Range::hir(arg)
-    {
-        let mut_ids = vec![check_for_mutability(cx, start), check_for_mutability(cx, end)];
-        if mut_ids[0].is_some() || mut_ids[1].is_some() {
-            let (span_low, span_high) = check_for_mutation(cx, body, &mut_ids);
+    if_chain! {
+        if let Some(higher::Range {
+            start: Some(start),
+            end: Some(end),
+            ..
+        }) = higher::Range::hir(arg);
+        let (mut_id_start, mut_id_end) = (check_for_mutability(cx, start), check_for_mutability(cx, end));
+        if mut_id_start.is_some() || mut_id_end.is_some();
+        then {
+            let (span_low, span_high) = check_for_mutation(cx, body, mut_id_start, mut_id_end);
             mut_warn_with_span(cx, span_low);
             mut_warn_with_span(cx, span_high);
         }
@@ -27,11 +30,13 @@ pub(super) fn check(cx: &LateContext<'_>, arg: &Expr<'_>, body: &Expr<'_>) {
 
 fn mut_warn_with_span(cx: &LateContext<'_>, span: Option<Span>) {
     if let Some(sp) = span {
-        span_lint(
+        span_lint_and_note(
             cx,
             MUT_RANGE_BOUND,
             sp,
-            "attempt to mutate range bound within loop; note that the range of the loop is unchanged",
+            "attempt to mutate range bound within loop",
+            None,
+            "the range of the loop is unchanged",
         );
     }
 }
@@ -51,12 +56,13 @@ fn check_for_mutability(cx: &LateContext<'_>, bound: &Expr<'_>) -> Option<HirId>
 fn check_for_mutation<'tcx>(
     cx: &LateContext<'tcx>,
     body: &Expr<'_>,
-    bound_ids: &[Option<HirId>],
+    bound_id_start: Option<HirId>,
+    bound_id_end: Option<HirId>,
 ) -> (Option<Span>, Option<Span>) {
     let mut delegate = MutatePairDelegate {
         cx,
-        hir_id_low: bound_ids[0],
-        hir_id_high: bound_ids[1],
+        hir_id_low: bound_id_start,
+        hir_id_high: bound_id_end,
         span_low: None,
         span_high: None,
     };
@@ -70,6 +76,7 @@ fn check_for_mutation<'tcx>(
         )
         .walk_expr(body);
     });
+
     delegate.mutation_span()
 }
 
@@ -87,10 +94,10 @@ impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, 'tcx> {
     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 {
+                if Some(id) == self.hir_id_low && !BreakAfterExprVisitor::is_found(self.cx, diag_expr_id) {
                     self.span_low = Some(self.cx.tcx.hir().span(diag_expr_id));
                 }
-                if Some(id) == self.hir_id_high {
+                if Some(id) == self.hir_id_high && !BreakAfterExprVisitor::is_found(self.cx, diag_expr_id) {
                     self.span_high = Some(self.cx.tcx.hir().span(diag_expr_id));
                 }
             }
@@ -99,10 +106,10 @@ impl<'tcx> Delegate<'tcx> for MutatePairDelegate<'_, '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 {
+            if Some(id) == self.hir_id_low && !BreakAfterExprVisitor::is_found(self.cx, diag_expr_id) {
                 self.span_low = Some(self.cx.tcx.hir().span(diag_expr_id));
             }
-            if Some(id) == self.hir_id_high {
+            if Some(id) == self.hir_id_high && !BreakAfterExprVisitor::is_found(self.cx, diag_expr_id) {
                 self.span_high = Some(self.cx.tcx.hir().span(diag_expr_id));
             }
         }
@@ -116,3 +123,52 @@ impl MutatePairDelegate<'_, '_> {
         (self.span_low, self.span_high)
     }
 }
+
+struct BreakAfterExprVisitor {
+    hir_id: HirId,
+    past_expr: bool,
+    past_candidate: bool,
+    break_after_expr: bool,
+}
+
+impl BreakAfterExprVisitor {
+    pub fn is_found(cx: &LateContext<'_>, hir_id: HirId) -> bool {
+        let mut visitor = BreakAfterExprVisitor {
+            hir_id,
+            past_expr: false,
+            past_candidate: false,
+            break_after_expr: false,
+        };
+
+        get_enclosing_block(cx, hir_id).map_or(false, |block| {
+            visitor.visit_block(block);
+            visitor.break_after_expr
+        })
+    }
+}
+
+impl intravisit::Visitor<'tcx> for BreakAfterExprVisitor {
+    type Map = Map<'tcx>;
+
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::None
+    }
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
+        if self.past_candidate {
+            return;
+        }
+
+        if expr.hir_id == self.hir_id {
+            self.past_expr = true;
+        } else if self.past_expr {
+            if matches!(&expr.kind, ExprKind::Break(..)) {
+                self.break_after_expr = true;
+            }
+
+            self.past_candidate = true;
+        } else {
+            intravisit::walk_expr(self, expr);
+        }
+    }
+}
diff --git a/tests/ui/mut_range_bound.rs b/tests/ui/mut_range_bound.rs
@@ -1,14 +1,6 @@
 #![allow(unused)]
 
-fn main() {
-    mut_range_bound_upper();
-    mut_range_bound_lower();
-    mut_range_bound_both();
-    mut_range_bound_no_mutation();
-    immut_range_bound();
-    mut_borrow_range_bound();
-    immut_borrow_range_bound();
-}
+fn main() {}
 
 fn mut_range_bound_upper() {
     let mut m = 4;
@@ -61,3 +53,32 @@ fn immut_range_bound() {
         continue;
     } // no warning
 }
+
+fn mut_range_bound_break() {
+    let mut m = 4;
+    for i in 0..m {
+        if m == 4 {
+            m = 5; // no warning because of immediate break
+            break;
+        }
+    }
+}
+
+fn mut_range_bound_no_immediate_break() {
+    let mut m = 4;
+    for i in 0..m {
+        m = 2; // warning because it is not immediately followed by break
+        if m == 4 {
+            break;
+        }
+    }
+
+    let mut n = 3;
+    for i in n..10 {
+        if n == 4 {
+            n = 1; // FIXME: warning because is is not immediately followed by break
+            let _ = 2;
+            break;
+        }
+    }
+}
diff --git a/tests/ui/mut_range_bound.stderr b/tests/ui/mut_range_bound.stderr
@@ -1,34 +1,59 @@
-error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
-  --> $DIR/mut_range_bound.rs:16:9
+error: attempt to mutate range bound within loop
+  --> $DIR/mut_range_bound.rs:8:9
    |
 LL |         m = 5;
    |         ^
    |
    = note: `-D clippy::mut-range-bound` implied by `-D warnings`
+   = note: the range of the loop is unchanged
 
-error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
-  --> $DIR/mut_range_bound.rs:23:9
+error: attempt to mutate range bound within loop
+  --> $DIR/mut_range_bound.rs:15:9
    |
 LL |         m *= 2;
    |         ^
+   |
+   = note: the range of the loop is unchanged
 
-error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
-  --> $DIR/mut_range_bound.rs:31:9
+error: attempt to mutate range bound within loop
+  --> $DIR/mut_range_bound.rs:23:9
    |
 LL |         m = 5;
    |         ^
+   |
+   = note: the range of the loop is unchanged
 
-error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
-  --> $DIR/mut_range_bound.rs:32:9
+error: attempt to mutate range bound within loop
+  --> $DIR/mut_range_bound.rs:24:9
    |
 LL |         n = 7;
    |         ^
+   |
+   = note: the range of the loop is unchanged
 
-error: attempt to mutate range bound within loop; note that the range of the loop is unchanged
-  --> $DIR/mut_range_bound.rs:46:22
+error: attempt to mutate range bound within loop
+  --> $DIR/mut_range_bound.rs:38:22
    |
 LL |         let n = &mut m; // warning
    |                      ^
+   |
+   = note: the range of the loop is unchanged
+
+error: attempt to mutate range bound within loop
+  --> $DIR/mut_range_bound.rs:70:9
+   |
+LL |         m = 2; // warning because it is not immediately followed by break
+   |         ^
+   |
+   = note: the range of the loop is unchanged
+
+error: attempt to mutate range bound within loop
+  --> $DIR/mut_range_bound.rs:79:13
+   |
+LL |             n = 1; // FIXME: warning because is is not immediately followed by break
+   |             ^
+   |
+   = note: the range of the loop is unchanged
 
-error: aborting due to 5 previous errors
+error: aborting due to 7 previous errors
 
