From 0579c3e0aa21df8c63cc10036107027aee723fd8 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sat, 6 Jul 2019 19:06:49 +0200 Subject: [PATCH 1/3] Fix breakage due to rust-lang/rust#61988 --- clippy_lints/src/loops.rs | 47 ++++++++++++++--------------- clippy_lints/src/shadow.rs | 4 --- clippy_lints/src/unused_label.rs | 2 +- clippy_lints/src/utils/author.rs | 15 ++------- clippy_lints/src/utils/hir_utils.rs | 12 -------- clippy_lints/src/utils/inspector.rs | 5 --- clippy_lints/src/utils/sugg.rs | 1 - 7 files changed, 25 insertions(+), 61 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index fccf8694ec51..cd36afba6af4 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -481,17 +481,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Loops { } // check for never_loop - match expr.node { - ExprKind::While(_, ref block, _) | ExprKind::Loop(ref block, _, _) => { - match never_loop_block(block, expr.hir_id) { - NeverLoopResult::AlwaysBreak => { - span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops") - }, - NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (), - } - }, - _ => (), - } + if let ExprKind::Loop(ref block, _, _) = expr.node { + match never_loop_block(block, expr.hir_id) { + NeverLoopResult::AlwaysBreak => { + span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops") + }, + NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (), + } + }; // check for `loop { if let {} else break }` that could be `while let` // (also matches an explicit "match" instead of "if let") @@ -590,9 +587,16 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Loops { } } - // check for while loops which conditions never change - if let ExprKind::While(ref cond, _, _) = expr.node { - check_infinite_loop(cx, cond, expr); + if_chain! { + if let ExprKind::Loop(block, _, LoopSource::While) = &expr.node; + if let Block { expr: Some(expr), .. } = &**block; + if let ExprKind::Match(cond, arms, MatchSource::WhileDesugar) = &expr.node; + if let ExprKind::DropTemps(cond) = &cond.node; + if let [arm, ..] = &arms[..]; + if let Arm { body, .. } = arm; + then { + check_infinite_loop(cx, cond, body); + } } check_needless_collect(expr, cx); @@ -701,12 +705,6 @@ fn never_loop_expr(expr: &Expr, main_loop_id: HirId) -> NeverLoopResult { // Break can come from the inner loop so remove them. absorb_break(&never_loop_block(b, main_loop_id)) }, - ExprKind::While(ref e, ref b, _) => { - let e = never_loop_expr(e, main_loop_id); - let result = never_loop_block(b, main_loop_id); - // Break can come from the inner loop so remove them. - combine_seq(e, absorb_break(&result)) - }, ExprKind::Match(ref e, ref arms, _) => { let e = never_loop_expr(e, main_loop_id); if arms.is_empty() { @@ -2202,7 +2200,7 @@ fn var_def_id(cx: &LateContext<'_, '_>, expr: &Expr) -> Option { fn is_loop(expr: &Expr) -> bool { match expr.node { - ExprKind::Loop(..) | ExprKind::While(..) => true, + ExprKind::Loop(..) => true, _ => false, } } @@ -2239,11 +2237,10 @@ fn is_loop_nested(cx: &LateContext<'_, '_>, loop_expr: &Expr, iter_expr: &Expr) return false; } match cx.tcx.hir().find(parent) { - Some(Node::Expr(expr)) => match expr.node { - ExprKind::Loop(..) | ExprKind::While(..) => { + Some(Node::Expr(expr)) => { + if let ExprKind::Loop(..) = expr.node { return true; - }, - _ => (), + }; }, Some(Node::Block(block)) => { let mut block_visitor = LoopNestVisitor { diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index 95cd118c98eb..c75e33e8406c 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -319,10 +319,6 @@ fn check_expr<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, bindings: check_expr(cx, e, bindings) } }, - ExprKind::While(ref cond, ref block, _) => { - check_expr(cx, cond, bindings); - check_block(cx, block, bindings); - }, ExprKind::Match(ref init, ref arms, _) => { check_expr(cx, init, bindings); let len = bindings.len(); diff --git a/clippy_lints/src/unused_label.rs b/clippy_lints/src/unused_label.rs index a9d78d30f2bf..9c02c1c31042 100644 --- a/clippy_lints/src/unused_label.rs +++ b/clippy_lints/src/unused_label.rs @@ -68,7 +68,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnusedLabelVisitor<'a, 'tcx> { self.labels.remove(&label.ident.as_str()); } }, - hir::ExprKind::Loop(_, Some(label), _) | hir::ExprKind::While(_, _, Some(label)) => { + hir::ExprKind::Loop(_, Some(label), _) => { self.labels.insert(label.ident.as_str(), expr.span); }, _ => (), diff --git a/clippy_lints/src/utils/author.rs b/clippy_lints/src/utils/author.rs index 3e3a5c48dabf..a00eee67166e 100644 --- a/clippy_lints/src/utils/author.rs +++ b/clippy_lints/src/utils/author.rs @@ -322,19 +322,6 @@ impl<'tcx> Visitor<'tcx> for PrintVisitor { self.current = cast_pat; self.visit_expr(expr); }, - ExprKind::While(ref cond, ref body, _) => { - let cond_pat = self.next("cond"); - let body_pat = self.next("body"); - let label_pat = self.next("label"); - println!( - "While(ref {}, ref {}, ref {}) = {};", - cond_pat, body_pat, label_pat, current - ); - self.current = cond_pat; - self.visit_expr(cond); - self.current = body_pat; - self.visit_block(body); - }, ExprKind::Loop(ref body, _, desugaring) => { let body_pat = self.next("body"); let des = loop_desugaring_name(desugaring); @@ -696,6 +683,7 @@ fn desugaring_name(des: hir::MatchSource) -> String { match des { hir::MatchSource::ForLoopDesugar => "MatchSource::ForLoopDesugar".to_string(), hir::MatchSource::TryDesugar => "MatchSource::TryDesugar".to_string(), + hir::MatchSource::WhileDesugar => "MatchSource::WhileDesugar".to_string(), hir::MatchSource::WhileLetDesugar => "MatchSource::WhileLetDesugar".to_string(), hir::MatchSource::Normal => "MatchSource::Normal".to_string(), hir::MatchSource::IfLetDesugar { contains_else_clause } => format!( @@ -714,6 +702,7 @@ fn loop_desugaring_name(des: hir::LoopSource) -> &'static str { match des { hir::LoopSource::ForLoop => "LoopSource::ForLoop", hir::LoopSource::Loop => "LoopSource::Loop", + hir::LoopSource::While => "LoopSource::WhileDesugar", hir::LoopSource::WhileLet => "LoopSource::WhileLet", } } diff --git a/clippy_lints/src/utils/hir_utils.rs b/clippy_lints/src/utils/hir_utils.rs index 5b2d24a7cb34..e9b5cee430e7 100644 --- a/clippy_lints/src/utils/hir_utils.rs +++ b/clippy_lints/src/utils/hir_utils.rs @@ -148,11 +148,6 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> { (&ExprKind::Tup(ref l_tup), &ExprKind::Tup(ref r_tup)) => self.eq_exprs(l_tup, r_tup), (&ExprKind::Unary(l_op, ref le), &ExprKind::Unary(r_op, ref re)) => l_op == r_op && self.eq_expr(le, re), (&ExprKind::Array(ref l), &ExprKind::Array(ref r)) => self.eq_exprs(l, r), - (&ExprKind::While(ref lc, ref lb, ref ll), &ExprKind::While(ref rc, ref rb, ref rl)) => { - self.eq_expr(lc, rc) - && self.eq_block(lb, rb) - && both(ll, rl, |l, r| l.ident.as_str() == r.ident.as_str()) - }, (&ExprKind::DropTemps(ref le), &ExprKind::DropTemps(ref re)) => self.eq_expr(le, re), _ => false, } @@ -524,13 +519,6 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> { lop.hash(&mut self.s); self.hash_expr(le); }, - ExprKind::While(ref cond, ref b, l) => { - self.hash_expr(cond); - self.hash_block(b); - if let Some(l) = l { - self.hash_name(l.ident.name); - } - }, } } diff --git a/clippy_lints/src/utils/inspector.rs b/clippy_lints/src/utils/inspector.rs index 00edb4fafcb1..ea169481ac6b 100644 --- a/clippy_lints/src/utils/inspector.rs +++ b/clippy_lints/src/utils/inspector.rs @@ -209,11 +209,6 @@ fn print_expr(cx: &LateContext<'_, '_>, expr: &hir::Expr, indent: usize) { print_expr(cx, e, indent + 1); println!("{}target type: {:?}", ind, target); }, - hir::ExprKind::While(ref cond, _, _) => { - println!("{}While", ind); - println!("{}condition:", ind); - print_expr(cx, cond, indent + 1); - }, hir::ExprKind::Loop(..) => { println!("{}Loop", ind); }, diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index a9643a74085e..0e45750e94df 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -113,7 +113,6 @@ impl<'a> Sugg<'a> { | hir::ExprKind::Ret(..) | hir::ExprKind::Struct(..) | hir::ExprKind::Tup(..) - | hir::ExprKind::While(..) | hir::ExprKind::DropTemps(_) | hir::ExprKind::Err => Sugg::NonParen(snippet), hir::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, snippet), From adcc02ed8ac1cdf040719d5b32574fb6fccb674a Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sat, 6 Jul 2019 19:35:08 +0200 Subject: [PATCH 2/3] Address reviews --- clippy_lints/src/loops.rs | 14 +++----------- clippy_lints/src/utils/author.rs | 2 +- clippy_lints/src/utils/higher.rs | 17 +++++++++++++++++ 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index cd36afba6af4..c3ca1de3a23b 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -488,7 +488,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Loops { }, NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (), } - }; + } // check for `loop { if let {} else break }` that could be `while let` // (also matches an explicit "match" instead of "if let") @@ -587,16 +587,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Loops { } } - if_chain! { - if let ExprKind::Loop(block, _, LoopSource::While) = &expr.node; - if let Block { expr: Some(expr), .. } = &**block; - if let ExprKind::Match(cond, arms, MatchSource::WhileDesugar) = &expr.node; - if let ExprKind::DropTemps(cond) = &cond.node; - if let [arm, ..] = &arms[..]; - if let Arm { body, .. } = arm; - then { - check_infinite_loop(cx, cond, body); - } + if let Some((cond, body)) = higher::while_loop(&expr) { + check_infinite_loop(cx, cond, body); } check_needless_collect(expr, cx); diff --git a/clippy_lints/src/utils/author.rs b/clippy_lints/src/utils/author.rs index a00eee67166e..f419338097d2 100644 --- a/clippy_lints/src/utils/author.rs +++ b/clippy_lints/src/utils/author.rs @@ -702,7 +702,7 @@ fn loop_desugaring_name(des: hir::LoopSource) -> &'static str { match des { hir::LoopSource::ForLoop => "LoopSource::ForLoop", hir::LoopSource::Loop => "LoopSource::Loop", - hir::LoopSource::While => "LoopSource::WhileDesugar", + hir::LoopSource::While => "LoopSource::While", hir::LoopSource::WhileLet => "LoopSource::WhileLet", } } diff --git a/clippy_lints/src/utils/higher.rs b/clippy_lints/src/utils/higher.rs index dda05d3fbc52..6fb2a3622c60 100644 --- a/clippy_lints/src/utils/higher.rs +++ b/clippy_lints/src/utils/higher.rs @@ -199,6 +199,23 @@ pub fn for_loop(expr: &hir::Expr) -> Option<(&hir::Pat, &hir::Expr, &hir::Expr)> None } +/// Recover the essential nodes of a desugared while loop: +/// `while cond { body }` becomes `(cond, body)`. +pub fn while_loop(expr: &hir::Expr) -> Option<(&hir::Expr, &hir::Expr)> { + if_chain! { + if let hir::ExprKind::Loop(block, _, hir::LoopSource::While) = &expr.node; + if let hir::Block { expr: Some(expr), .. } = &**block; + if let hir::ExprKind::Match(cond, arms, hir::MatchSource::WhileDesugar) = &expr.node; + if let hir::ExprKind::DropTemps(cond) = &cond.node; + if let [arm, ..] = &arms[..]; + if let hir::Arm { body, .. } = arm; + then { + return Some((cond, body)); + } + } + None +} + /// Recover the essential nodes of a desugared if block /// `if cond { then } else { els }` becomes `(cond, then, Some(els))` pub fn if_block(expr: &hir::Expr) -> Option<(&hir::Expr, &hir::Expr, Option<&hir::Expr>)> { From c72be0f65a20c02dde25be66fa0a78c755a55530 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sat, 6 Jul 2019 19:43:34 +0200 Subject: [PATCH 3/3] rustfmt --- clippy_lints/src/loops.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index c3ca1de3a23b..02074c79b41e 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -483,9 +483,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Loops { // check for never_loop if let ExprKind::Loop(ref block, _, _) = expr.node { match never_loop_block(block, expr.hir_id) { - NeverLoopResult::AlwaysBreak => { - span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops") - }, + NeverLoopResult::AlwaysBreak => span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops"), NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (), } }