Skip to content

Commit 410760d

Browse files
oli-obkoli-obk
authored
Merge pull request #2206 from LaurentMazare/master
Refactor the never-loop lint.
2 parents 9cd778a + c968190 commit 410760d

File tree

5 files changed

+122
-59
lines changed

5 files changed

+122
-59
lines changed

‎clippy_lints/src/loops.rs

Lines changed: 103 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -378,13 +378,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
378378
// check for never_loop
379379
match expr.node {
380380
ExprWhile(_, ref block, _) | ExprLoop(ref block, _, _) => {
381-
let mut state = NeverLoopState {
382-
breaks: HashSet::new(),
383-
continues: HashSet::new(),
384-
};
385-
let may_complete = never_loop_block(block, &mut state);
386-
if !may_complete && !state.continues.contains(&expr.id) {
387-
span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops");
381+
match never_loop_block(block, &expr.id) {
382+
NeverLoopResult::AlwaysBreak =>
383+
span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops"),
384+
NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (),
388385
}
389386
},
390387
_ => (),
@@ -491,16 +488,59 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
491488
}
492489
}
493490

494-
struct NeverLoopState {
495-
breaks: HashSet<NodeId>,
496-
continues: HashSet<NodeId>,
491+
enum NeverLoopResult {
492+
// A break/return always get triggered but not necessarily for the main loop.
493+
AlwaysBreak,
494+
// A continue may occur for the main loop.
495+
MayContinueMainLoop,
496+
Otherwise,
497+
}
498+
499+
fn absorb_break(arg: &NeverLoopResult) -> NeverLoopResult {
500+
match *arg {
501+
NeverLoopResult::AlwaysBreak |
502+
NeverLoopResult::Otherwise => NeverLoopResult::Otherwise,
503+
NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop,
504+
}
505+
}
506+
507+
// Combine two results for parts that are called in order.
508+
fn combine_seq(first: NeverLoopResult, second: NeverLoopResult) -> NeverLoopResult {
509+
match first {
510+
NeverLoopResult::AlwaysBreak | NeverLoopResult::MayContinueMainLoop => first,
511+
NeverLoopResult::Otherwise => second,
512+
}
513+
}
514+
515+
// Combine two results where both parts are called but not necessarily in order.
516+
fn combine_both(left: NeverLoopResult, right: NeverLoopResult) -> NeverLoopResult {
517+
match (left, right) {
518+
(NeverLoopResult::MayContinueMainLoop, _) | (_, NeverLoopResult::MayContinueMainLoop) =>
519+
NeverLoopResult::MayContinueMainLoop,
520+
(NeverLoopResult::AlwaysBreak, _) | (_, NeverLoopResult::AlwaysBreak) =>
521+
NeverLoopResult::AlwaysBreak,
522+
(NeverLoopResult::Otherwise, NeverLoopResult::Otherwise) =>
523+
NeverLoopResult::Otherwise,
524+
}
525+
}
526+
527+
// Combine two results where only one of the part may have been executed.
528+
fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult {
529+
match (b1, b2) {
530+
(NeverLoopResult::AlwaysBreak, NeverLoopResult::AlwaysBreak) =>
531+
NeverLoopResult::AlwaysBreak,
532+
(NeverLoopResult::MayContinueMainLoop, _) | (_, NeverLoopResult::MayContinueMainLoop) =>
533+
NeverLoopResult::MayContinueMainLoop,
534+
(NeverLoopResult::Otherwise, _) | (_, NeverLoopResult::Otherwise) =>
535+
NeverLoopResult::Otherwise,
536+
}
497537
}
498538

499-
fn never_loop_block(block: &Block, state: &mut NeverLoopState) -> bool {
539+
fn never_loop_block(block: &Block, main_loop_id: &NodeId) -> NeverLoopResult {
500540
let stmts = block.stmts.iter().map(stmt_to_expr);
501541
let expr = once(block.expr.as_ref().map(|p| &**p));
502542
let mut iter = stmts.chain(expr).filter_map(|e| e);
503-
never_loop_expr_seq(&mut iter, state)
543+
never_loop_expr_seq(&mut iter, main_loop_id)
504544
}
505545

506546
fn stmt_to_expr(stmt: &Stmt) -> Option<&Expr> {
@@ -517,7 +557,7 @@ fn decl_to_expr(decl: &Decl) -> Option<&Expr> {
517557
}
518558
}
519559

520-
fn never_loop_expr(expr: &Expr, state: &mut NeverLoopState) -> bool {
560+
fn never_loop_expr(expr: &Expr, main_loop_id: &NodeId) -> NeverLoopResult {
521561
match expr.node {
522562
ExprBox(ref e) |
523563
ExprUnary(_, ref e) |
@@ -526,77 +566,84 @@ fn never_loop_expr(expr: &Expr, state: &mut NeverLoopState) -> bool {
526566
ExprField(ref e, _) |
527567
ExprTupField(ref e, _) |
528568
ExprAddrOf(_, ref e) |
529-
ExprRepeat(ref e, _) => never_loop_expr(e, state),
569+
ExprStruct(_, _, Some(ref e)) |
570+
ExprRepeat(ref e, _) => never_loop_expr(e, main_loop_id),
530571
ExprArray(ref es) | ExprMethodCall(_, _, ref es) | ExprTup(ref es) => {
531-
never_loop_expr_seq(&mut es.iter(), state)
572+
never_loop_expr_all(&mut es.iter(), main_loop_id)
532573
},
533-
ExprCall(ref e, ref es) => never_loop_expr_seq(&mut once(&**e).chain(es.iter()), state),
574+
ExprCall(ref e, ref es) => never_loop_expr_all(&mut once(&**e).chain(es.iter()), main_loop_id),
534575
ExprBinary(_, ref e1, ref e2) |
535576
ExprAssign(ref e1, ref e2) |
536577
ExprAssignOp(_, ref e1, ref e2) |
537-
ExprIndex(ref e1, ref e2) => never_loop_expr_seq(&mut [&**e1, &**e2].iter().cloned(), state),
578+
ExprIndex(ref e1, ref e2) => never_loop_expr_all(&mut [&**e1, &**e2].iter().cloned(), main_loop_id),
538579
ExprIf(ref e, ref e2, ref e3) => {
539-
let e1 = never_loop_expr(e, state);
540-
let e2 = never_loop_expr(e2, state);
541-
match *e3 {
542-
Some(ref e3) => {
543-
let e3 = never_loop_expr(e3, state);
544-
e1 && (e2 || e3)
545-
},
546-
None => e1,
547-
}
580+
let e1 = never_loop_expr(e, main_loop_id);
581+
let e2 = never_loop_expr(e2, main_loop_id);
582+
let e3 = e3.as_ref().map_or(NeverLoopResult::Otherwise, |e| never_loop_expr(e, main_loop_id));
583+
combine_seq(e1, combine_branches(e2, e3))
548584
},
549585
ExprLoop(ref b, _, _) => {
550-
let block_may_complete = never_loop_block(b, state);
551-
let has_break = state.breaks.remove(&expr.id);
552-
state.continues.remove(&expr.id);
553-
block_may_complete || has_break
586+
// Break can come from the inner loop so remove them.
587+
absorb_break(&never_loop_block(b, main_loop_id))
554588
},
555589
ExprWhile(ref e, ref b, _) => {
556-
let e = never_loop_expr(e, state);
557-
let block_may_complete = never_loop_block(b, state);
558-
let has_break = state.breaks.remove(&expr.id);
559-
let has_continue = state.continues.remove(&expr.id);
560-
e && (block_may_complete || has_break || has_continue)
590+
let e = never_loop_expr(e, main_loop_id);
591+
let result = never_loop_block(b, main_loop_id);
592+
// Break can come from the inner loop so remove them.
593+
combine_seq(e, absorb_break(&result))
561594
},
562595
ExprMatch(ref e, ref arms, _) => {
563-
let e = never_loop_expr(e, state);
564-
let arms = never_loop_expr_branch(&mut arms.iter().map(|a| &*a.body), state);
565-
e && arms
596+
let e = never_loop_expr(e, main_loop_id);
597+
if arms.is_empty() {
598+
e
599+
} else {
600+
let arms = never_loop_expr_branch(&mut arms.iter().map(|a| &*a.body), main_loop_id);
601+
combine_seq(e, arms)
602+
}
566603
},
567-
ExprBlock(ref b) => never_loop_block(b, state),
604+
ExprBlock(ref b) => never_loop_block(b, main_loop_id),
568605
ExprAgain(d) => {
569606
let id = d.target_id
570607
.opt_id()
571608
.expect("target id can only be missing in the presence of compilation errors");
572-
state.continues.insert(id);
573-
false
609+
if id == *main_loop_id {
610+
NeverLoopResult::MayContinueMainLoop
611+
} else {
612+
NeverLoopResult::AlwaysBreak
613+
}
574614
},
575-
ExprBreak(d, _) => {
576-
let id = d.target_id
577-
.opt_id()
578-
.expect("target id can only be missing in the presence of compilation errors");
579-
state.breaks.insert(id);
580-
false
615+
ExprBreak(_, _) => {
616+
NeverLoopResult::AlwaysBreak
581617
},
582618
ExprRet(ref e) => {
583619
if let Some(ref e) = *e {
584-
never_loop_expr(e, state);
620+
combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak)
621+
} else {
622+
NeverLoopResult::AlwaysBreak
585623
}
586-
false
587624
},
588-
_ => true,
625+
ExprStruct(_, _, None) |
626+
ExprYield(_) |
627+
ExprClosure(_, _, _, _, _) |
628+
ExprInlineAsm(_, _, _) |
629+
ExprPath(_) |
630+
ExprLit(_) => NeverLoopResult::Otherwise,
589631
}
590632
}
591633

592-
fn never_loop_expr_seq<'a, T: Iterator<Item = &'a Expr>>(es: &mut T, state: &mut NeverLoopState) -> bool {
593-
es.map(|e| never_loop_expr(e, state))
594-
.fold(true, |a, b| a && b)
634+
fn never_loop_expr_seq<'a, T: Iterator<Item=&'a Expr>>(es: &mut T, main_loop_id: &NodeId) -> NeverLoopResult {
635+
es.map(|e| never_loop_expr(e, main_loop_id))
636+
.fold(NeverLoopResult::Otherwise, combine_seq)
637+
}
638+
639+
fn never_loop_expr_all<'a, T: Iterator<Item=&'a Expr>>(es: &mut T, main_loop_id: &NodeId) -> NeverLoopResult {
640+
es.map(|e| never_loop_expr(e, main_loop_id))
641+
.fold(NeverLoopResult::Otherwise, combine_both)
595642
}
596643

597-
fn never_loop_expr_branch<'a, T: Iterator<Item = &'a Expr>>(e: &mut T, state: &mut NeverLoopState) -> bool {
598-
e.map(|e| never_loop_expr(e, state))
599-
.fold(false, |a, b| a || b)
644+
fn never_loop_expr_branch<'a, T: Iterator<Item=&'a Expr>>(e: &mut T, main_loop_id: &NodeId) -> NeverLoopResult {
645+
e.map(|e| never_loop_expr(e, main_loop_id))
646+
.fold(NeverLoopResult::AlwaysBreak, combine_branches)
600647
}
601648

602649
fn check_for_loop<'a, 'tcx>(

‎clippy_lints/src/strings.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,6 @@ impl LintPass for StringLitAsBytes {
144144

145145
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes {
146146
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) {
147-
use std::ascii::AsciiExt;
148147
use syntax::ast::LitKind;
149148
use utils::{in_macro, snippet};
150149

‎tests/ui/never_loop.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,15 @@ pub fn test14() {
152152
}
153153
}
154154

155+
// Issue #1991: the outter loop should not warn.
156+
pub fn test15() {
157+
'label: loop {
158+
while false {
159+
break 'label;
160+
}
161+
}
162+
}
163+
155164
fn main() {
156165
test1();
157166
test2();

‎tests/ui/never_loop.stderr

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,3 +80,11 @@ error: this loop never actually loops
8080
152 | | }
8181
| |_____^
8282

83+
error: this loop never actually loops
84+
--> $DIR/never_loop.rs:158:9
85+
|
86+
158 | / while false {
87+
159 | | break 'label;
88+
160 | | }
89+
| |_________^
90+

‎tests/ui/unicode.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ error: zero-width space detected
22
--> $DIR/unicode.rs:6:12
33
|
44
6 | print!("Here >​< is a ZWS, and ​another");
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
66
|
77
= note: `-D zero-width-space` implied by `-D warnings`
88
= help: Consider replacing the string with:
@@ -12,7 +12,7 @@ error: non-nfc unicode sequence detected
1212
--> $DIR/unicode.rs:12:12
1313
|
1414
12 | print!("̀àh?");
15-
| ^^^^^^^
15+
| ^^^^^
1616
|
1717
= note: `-D unicode-not-nfc` implied by `-D warnings`
1818
= help: Consider replacing the string with:

0 commit comments

Comments
 (0)