From 290fb8de6637262c79bda37723bf784d6341056b Mon Sep 17 00:00:00 2001 From: dswij Date: Thu, 26 Aug 2021 18:20:13 +0800 Subject: [PATCH 1/4] Add additional test for broken loop in `mut_range_bound` --- tests/ui/mut_range_bound.rs | 39 ++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/tests/ui/mut_range_bound.rs b/tests/ui/mut_range_bound.rs index 1348dd2a3d8b..e1ae1ef92822 100644 --- 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; + } + } +} From 8fbf75e0f9f7abf0f7820f1a89dfc230d3bc918f Mon Sep 17 00:00:00 2001 From: dswij Date: Thu, 2 Sep 2021 16:04:46 +0800 Subject: [PATCH 2/4] `mut_range_bound` to check for immediate break from loop --- clippy_lints/src/loops/mut_range_bound.rs | 98 ++++++++++++++++++----- 1 file changed, 77 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/loops/mut_range_bound.rs b/clippy_lints/src/loops/mut_range_bound.rs index 344dc5074d36..358d53e8859d 100644 --- 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) { 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 fn check_for_mutation<'tcx>( cx: &LateContext<'tcx>, body: &Expr<'_>, - bound_ids: &[Option], + bound_id_start: Option, + bound_id_end: Option, ) -> (Option, Option) { 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 { + 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); + } + } +} From 7515d9c6f77a3325e6142c45ebe1bc78f580b894 Mon Sep 17 00:00:00 2001 From: dswij Date: Fri, 3 Sep 2021 12:27:05 +0800 Subject: [PATCH 3/4] Update test output for `mut_range_bound` --- tests/ui/mut_range_bound.stderr | 47 +++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/tests/ui/mut_range_bound.stderr b/tests/ui/mut_range_bound.stderr index 0eeb76e0ec5f..4b5a3fc1e418 100644 --- 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 From dc6f7dc6bf13295d5249c7f2a9aa71024b6555ad Mon Sep 17 00:00:00 2001 From: dswij Date: Tue, 7 Sep 2021 06:07:50 +0800 Subject: [PATCH 4/4] Add known problems to `mut_range_bound` docs --- clippy_lints/src/loops/mod.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index 97cbe2c53dda..2860cb68f42f 100644 --- 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;