mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #1804 from camsteffen/never_loop

Browse Source 
fix never_loop
This commit is contained in:
Oliver Schneider 2017-06-12 08:08:19 +02:00 committed by GitHub
commit b1e9c1b7e7
8 changed files with 313 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
README.md
View File

@ -295,7 +295,7 @@ name
[needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return) | warn | using a return statement like `return expr;` where an expression would suffice
[needless_update](https://github.com/Manishearth/rust-clippy/wiki#needless_update) | warn | using `Foo { ..base }` when there are no missing fields
[neg_multiply](https://github.com/Manishearth/rust-clippy/wiki#neg_multiply) | warn | multiplying integers with -1
[never_loop](https://github.com/Manishearth/rust-clippy/wiki#never_loop) | allow | any loop with an unconditional `break` or `return` statement
[never_loop](https://github.com/Manishearth/rust-clippy/wiki#never_loop) | warn | any loop that will always `break` or `return`
[new_ret_no_self](https://github.com/Manishearth/rust-clippy/wiki#new_ret_no_self) | warn | not returning `Self` in a `new` method
[new_without_default](https://github.com/Manishearth/rust-clippy/wiki#new_without_default) | warn | `fn new() -> Self` method without `Default` implementation
[new_without_default_derive](https://github.com/Manishearth/rust-clippy/wiki#new_without_default_derive) | warn | `fn new() -> Self` without `#[derive]`able `Default` implementation

11
clippy_lints/src/attrs.rs
View File

@ -184,16 +184,15 @@ fn is_relevant_trait(tcx: TyCtxt, item: &TraitItem) -> bool {
}
fn is_relevant_block(tcx: TyCtxt, tables: &ty::TypeckTables, block: &Block) -> bool {
for stmt in &block.stmts {
if let Some(stmt) = block.stmts.first() {
match stmt.node {
StmtDecl(_, _) => return true,
StmtDecl(_, _) => true,
StmtExpr(ref expr, _) |
StmtSemi(ref expr, _) => {
return is_relevant_expr(tcx, tables, expr);
},
StmtSemi(ref expr, _) => is_relevant_expr(tcx, tables, expr),
}
} else {
block.expr.as_ref().map_or(false, |e| is_relevant_expr(tcx, tables, e))
}
block.expr.as_ref().map_or(false, |e| is_relevant_expr(tcx, tables, e))
}
fn is_relevant_expr(tcx: TyCtxt, tables: &ty::TypeckTables, expr: &Expr) -> bool {

12
clippy_lints/src/doc.rs
View File

@ -200,7 +200,7 @@ fn check_doc(cx: &EarlyContext, valid_idents: &[String], docs: &[(String, Span)]
type Item = (bool, char);
fn next(&mut self) -> Option<(bool, char)> {
while self.line < self.docs.len() {
if self.line < self.docs.len() {
if self.reset {
self.line += 1;
self.reset = false;
@ -215,18 +215,18 @@ fn check_doc(cx: &EarlyContext, valid_idents: &[String], docs: &[(String, Span)]
self.pos += c.len_utf8();
let new_line = self.new_line;
self.new_line = c == '\n' || (self.new_line && c.is_whitespace());
return Some((new_line, c));
Some((new_line, c))
} else if self.line == self.docs.len() - 1 {
return None;
None
} else {
self.new_line = true;
self.reset = true;
self.pos += 1;
return Some((true, '\n'));
Some((true, '\n'))
}
} else {
None
}
None
}
}

2
clippy_lints/src/lib.rs
View File

@ -324,7 +324,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
enum_variants::STUTTER,
if_not_else::IF_NOT_ELSE,
items_after_statements::ITEMS_AFTER_STATEMENTS,
loops::NEVER_LOOP,
matches::SINGLE_MATCH_ELSE,
mem_forget::MEM_FORGET,
methods::FILTER_MAP,
@ -420,6 +419,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
loops::FOR_LOOP_OVER_RESULT,
loops::ITER_NEXT_LOOP,
loops::NEEDLESS_RANGE_LOOP,
loops::NEVER_LOOP,
loops::REVERSE_RANGE_LOOP,
loops::UNUSED_COLLECT,
loops::WHILE_LET_LOOP,

125
clippy_lints/src/loops.rs
View File

@ -286,15 +286,13 @@ declare_lint! {
"looping on a map using `iter` when `keys` or `values` would do"
}
/// **What it does:** Checks for loops that contain an unconditional `break`
/// or `return`.
/// **What it does:** Checks for loops that will always `break`, `return` or
/// `continue` an outer loop.
///
/// **Why is this bad?** This loop never loops, all it does is obfuscating the
/// code.
///
/// **Known problems:** Ignores `continue` statements in the loop that create
/// nontrivial control flow. Therefore set to `Allow` by default.
/// See https://github.com/Manishearth/rust-clippy/issues/1586
/// **Known problems:** None
///
/// **Example:**
/// ```rust
@ -302,8 +300,8 @@ declare_lint! {
/// ```
declare_lint! {
pub NEVER_LOOP,
Allow,
"any loop with an unconditional `break` or `return` statement"
Warn,
"any loop that will always `break` or `return`"
}
#[derive(Copy, Clone)]
@ -333,6 +331,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
if let Some((pat, arg, body)) = higher::for_loop(expr) {
check_for_loop(cx, pat, arg, body, expr);
}
// check for never_loop
match expr.node {
ExprWhile(_, ref block, _) |
ExprLoop(ref block, _, _) => {
if never_loop(block, &expr.id) {
span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops");
}
},
_ => (),
}
// check for `loop { if let {} else break }` that could be `while let`
// (also matches an explicit "match" instead of "if let")
// (even if the "match" or "if let" is used for declaration)
@ -345,9 +355,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
"empty `loop {}` detected. You may want to either use `panic!()` or add \
`std::thread::sleep(..);` to the loop body.");
}
if never_loop_block(block) {
span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops");
}
// extract the expression from the first statement (if any) in a block
let inner_stmt_expr = extract_expr_from_first_stmt(block);
@ -425,47 +432,103 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
}
}
fn never_loop_block(block: &Block) -> bool {
block.stmts.iter().any(never_loop_stmt) || block.expr.as_ref().map_or(false, |e| never_loop_expr(e))
fn never_loop(block: &Block, id: &NodeId) -> bool {
!contains_continue_block(block, id) && loop_exit_block(block)
}
fn never_loop_stmt(stmt: &Stmt) -> bool {
fn contains_continue_block(block: &Block, dest: &NodeId) -> bool {
block.stmts.iter().any(|e| contains_continue_stmt(e, dest))
|| block.expr.as_ref().map_or(false, |e| contains_continue_expr(e, dest))
}
fn contains_continue_stmt(stmt: &Stmt, dest: &NodeId) -> bool {
match stmt.node {
StmtSemi(ref e, _) |
StmtExpr(ref e, _) => never_loop_expr(e),
StmtDecl(ref d, _) => never_loop_decl(d),
StmtExpr(ref e, _) => contains_continue_expr(e, dest),
StmtDecl(ref d, _) => contains_continue_decl(d, dest),
}
}
fn never_loop_decl(decl: &Decl) -> bool {
if let DeclLocal(ref local) = decl.node {
local.init.as_ref().map_or(false, |e| never_loop_expr(e))
} else {
false
fn contains_continue_decl(decl: &Decl, dest: &NodeId) -> bool {
match decl.node {
DeclLocal(ref local) => local.init.as_ref().map_or(false, |e| contains_continue_expr(e, dest)),
_ => false
}
}
fn never_loop_expr(expr: &Expr) -> bool {
fn contains_continue_expr(expr: &Expr, dest: &NodeId) -> bool {
match expr.node {
ExprBreak(..) | ExprRet(..) => true,
ExprBox(ref e) |
ExprUnary(_, ref e) |
ExprBinary(_, ref e, _) | // because short-circuiting
ExprCast(ref e, _) |
ExprType(ref e, _) |
ExprField(ref e, _) |
ExprTupField(ref e, _) |
ExprRepeat(ref e, _) |
ExprAddrOf(_, ref e) => never_loop_expr(e),
ExprAddrOf(_, ref e) |
ExprRepeat(ref e, _) => contains_continue_expr(e, dest),
ExprArray(ref es) |
ExprMethodCall(_, _, ref es) |
ExprTup(ref es) => es.iter().any(|e| contains_continue_expr(e, dest)),
ExprCall(ref e, ref es) => contains_continue_expr(e, dest) || es.iter().any(|e| contains_continue_expr(e, dest)),
ExprBinary(_, ref e1, ref e2) |
ExprAssign(ref e1, ref e2) |
ExprAssignOp(_, ref e1, ref e2) |
ExprIndex(ref e1, ref e2) => never_loop_expr(e1) || never_loop_expr(e2),
ExprIndex(ref e1, ref e2) => [e1, e2].iter().any(|e| contains_continue_expr(e, dest)),
ExprIf(ref e, ref e2, ref e3) => [e, e2].iter().chain(e3.as_ref().iter()).any(|e| contains_continue_expr(e, dest)),
ExprWhile(ref e, ref b, _) => contains_continue_expr(e, dest) || contains_continue_block(b, dest),
ExprMatch(ref e, ref arms, _) => contains_continue_expr(e, dest) || arms.iter().any(|a| contains_continue_expr(&a.body, dest)),
ExprBlock(ref block) => contains_continue_block(block, dest),
ExprStruct(_, _, ref base) => base.as_ref().map_or(false, |e| contains_continue_expr(e, dest)),
ExprAgain(d) => d.target_id.opt_id().map_or(false, |id| id == *dest),
_ => false,
}
}
fn loop_exit_block(block: &Block) -> bool {
block.stmts.iter().any(|e| loop_exit_stmt(e))
|| block.expr.as_ref().map_or(false, |e| loop_exit_expr(e))
}
fn loop_exit_stmt(stmt: &Stmt) -> bool {
match stmt.node {
StmtSemi(ref e, _) |
StmtExpr(ref e, _) => loop_exit_expr(e),
StmtDecl(ref d, _) => loop_exit_decl(d),
}
}
fn loop_exit_decl(decl: &Decl) -> bool {
match decl.node {
DeclLocal(ref local) => local.init.as_ref().map_or(false, |e| loop_exit_expr(e)),
_ => false
}
}
fn loop_exit_expr(expr: &Expr) -> bool {
match expr.node {
ExprBox(ref e) |
ExprUnary(_, ref e) |
ExprCast(ref e, _) |
ExprType(ref e, _) |
ExprField(ref e, _) |
ExprTupField(ref e, _) |
ExprAddrOf(_, ref e) |
ExprRepeat(ref e, _) => loop_exit_expr(e),
ExprArray(ref es) |
ExprTup(ref es) |
ExprMethodCall(_, _, ref es) => es.iter().any(|e| never_loop_expr(e)),
ExprCall(ref e, ref es) => never_loop_expr(e) || es.iter().any(|e| never_loop_expr(e)),
ExprBlock(ref block) => never_loop_block(block),
ExprStruct(_, _, ref base) => base.as_ref().map_or(false, |e| never_loop_expr(e)),
ExprMethodCall(_, _, ref es) |
ExprTup(ref es) => es.iter().any(|e| loop_exit_expr(e)),
ExprCall(ref e, ref es) => loop_exit_expr(e) || es.iter().any(|e| loop_exit_expr(e)),
ExprBinary(_, ref e1, ref e2) |
ExprAssign(ref e1, ref e2) |
ExprAssignOp(_, ref e1, ref e2) |
ExprIndex(ref e1, ref e2) => [e1, e2].iter().any(|e| loop_exit_expr(e)),
ExprIf(ref e, ref e2, ref e3) => loop_exit_expr(e) || e3.as_ref().map_or(false, |e| loop_exit_expr(e)) && loop_exit_expr(e2),
ExprWhile(ref e, ref b, _) => loop_exit_expr(e) || loop_exit_block(b),
ExprMatch(ref e, ref arms, _) => loop_exit_expr(e) || arms.iter().all(|a| loop_exit_expr(&a.body)),
ExprBlock(ref b) => loop_exit_block(b),
ExprBreak(_, _) |
ExprAgain(_) |
ExprRet(_) => true,
_ => false,
}
}

22
clippy_tests/examples/for_loop.stderr
View File

@ -53,6 +53,28 @@ error: for loop over `v.iter().next().ok_or("x not found")`, which is a `Result`
= note: `-D for-loop-over-result` implied by `-D warnings`
= help: consider replacing `for x in v.iter().next().ok_or("x not found")` with `if let Ok(x) = v.iter().next().ok_or("x not found")`
error: this loop never actually loops
--> for_loop.rs:52:5
|
52 | / while let Some(x) = option {
53 | | println!("{}", x);
54 | | break;
55 | | }
| |_____^
|
= note: `-D never-loop` implied by `-D warnings`
error: this loop never actually loops
--> for_loop.rs:58:5
|
58 | / while let Ok(x) = result {
59 | | println!("{}", x);
60 | | break;
61 | | }
| |_____^
|
= note: `-D never-loop` implied by `-D warnings`
error: the loop variable `i` is only used to index `vec`.
--> for_loop.rs:84:5
|

128
clippy_tests/examples/never_loop.rs
View File

@ -1,34 +1,118 @@
#![feature(plugin)]
#![plugin(clippy)]
#![allow(single_match, unused_assignments, unused_variables)]
#![warn(never_loop)]
#![allow(dead_code, unused)]
fn main() {
loop {
println!("This is only ever printed once");
break;
}
let x = 1;
loop {
println!("This, too"); // but that's OK
fn test1() {
let mut x = 0;
loop { // never_loop
x += 1;
if x == 1 {
break;
}
}
loop {
loop {
// another one
break;
return
}
break;
}
}
fn test2() {
let mut x = 0;
loop {
loop {
if x == 1 { return; }
x += 1;
if x == 1 {
break
}
}
}
fn test3() {
let mut x = 0;
loop { // never loops
x += 1;
break
}
}
fn test4() {
let mut x = 1;
loop {
x += 1;
match x {
5 => return,
_ => (),
}
}
}
fn test5() {
let i = 0;
loop { // never loops
while i == 0 { // never loops
break
}
return
}
}
fn test6() {
let mut x = 0;
'outer: loop { // never loops
x += 1;
loop { // never loops
if x == 5 { break }
continue 'outer
}
return
}
}
fn test7() {
let mut x = 0;
loop {
x += 1;
match x {
1 => continue,
_ => (),
}
return
}
}
fn test8() {
let mut x = 0;
loop {
x += 1;
match x {
5 => return,
_ => continue,
}
}
}
fn test9() {
let x = Some(1);
while let Some(y) = x { // never loops
return
}
}
fn test10() {
for x in 0..10 { // never loops
match x {
1 => break,
_ => return,
}
}
}
fn main() {
test1();
test2();
test3();
test4();
test5();
test6();
test7();
test8();
test9();
test10();
}

96
clippy_tests/examples/never_loop.stderr
View File

@ -1,39 +1,99 @@
error: this loop never actually loops
--> never_loop.rs:8:5
--> never_loop.rs:7:5
|
8 | / loop {
9 | | println!("This is only ever printed once");
10 | | break;
11 | | }
7 | / loop { // never_loop
8 | | x += 1;
9 | | if x == 1 {
10 | | return
11 | | }
12 | | break;
13 | | }
| |_____^
|
= note: `-D never-loop` implied by `-D warnings`
error: this loop never actually loops
--> never_loop.rs:21:5
--> never_loop.rs:28:5
|
21 | / loop {
22 | | loop {
23 | | // another one
24 | | break;
25 | | }
26 | | break;
27 | | }
28 | / loop { // never loops
29 | | x += 1;
30 | | break
31 | | }
| |_____^
|
= note: `-D never-loop` implied by `-D warnings`
error: this loop never actually loops
--> never_loop.rs:22:9
--> never_loop.rs:47:2
|
22 | / loop {
23 | | // another one
24 | | break;
25 | | }
47 | / loop { // never loops
48 | | while i == 0 { // never loops
49 | | break
50 | | }
51 | | return
52 | | }
| |__^
|
= note: `-D never-loop` implied by `-D warnings`
error: this loop never actually loops
--> never_loop.rs:48:9
|
48 | / while i == 0 { // never loops
49 | | break
50 | | }
| |_________^
|
= note: `-D never-loop` implied by `-D warnings`
error: this loop never actually loops
--> never_loop.rs:57:5
|
57 | / 'outer: loop { // never loops
58 | | x += 1;
59 | | loop { // never loops
60 | | if x == 5 { break }
... |
63 | | return
64 | | }
| |__^
|
= note: `-D never-loop` implied by `-D warnings`
error: this loop never actually loops
--> never_loop.rs:59:3
|
59 | / loop { // never loops
60 | | if x == 5 { break }
61 | | continue 'outer
62 | | }
| |___^
|
= note: `-D never-loop` implied by `-D warnings`
error: this loop never actually loops
--> never_loop.rs:92:5
|
92 | / while let Some(y) = x { // never loops
93 | | return
94 | | }
| |_____^
|
= note: `-D never-loop` implied by `-D warnings`
error: this loop never actually loops
--> never_loop.rs:98:5
|
98 | / for x in 0..10 { // never loops
99 | | match x {
100 | | 1 => break,
101 | | _ => return,
102 | | }
103 | | }
| |_____^
|
= note: `-D never-loop` implied by `-D warnings`
error: aborting due to previous error(s)
error: Could not compile `clippy_tests`.