From 505eb53d2972844efe336a88036fb219341cfc07 Mon Sep 17 00:00:00 2001
From: Andre Bogus <bogusandre@gmail.com>
Date: Fri, 17 Feb 2017 04:53:14 +0100
Subject: [PATCH 1/2] New never loop lint
This lint detects loops that unconditionally break or return.
Closes #257
---
CHANGELOG.md | 2 ++
README.md | 3 +-
clippy_lints/src/lib.rs | 1 +
clippy_lints/src/loops.rs | 68 +++++++++++++++++++++++++++++++++++++-
tests/ui/never_loop.rs | 34 +++++++++++++++++++
tests/ui/never_loop.stderr | 41 +++++++++++++++++++++++
tests/ui/unused_labels.rs | 2 +-
tests/ui/while_loop.rs | 2 +-
8 files changed, 149 insertions(+), 4 deletions(-)
create mode 100644 tests/ui/never_loop.rs
create mode 100644 tests/ui/never_loop.stderr
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7ca1336afaf..90f67e8aec7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,6 +1,7 @@
# Change Log
All notable changes to this project will be documented in this file.
+* New [`never_loop`] lint
* New [`mut_from_ref`] lint
## 0.0.114 — 2017-02-08
@@ -384,6 +385,7 @@ All notable changes to this project will be documented in this file.
[`needless_return`]: https://github.com/Manishearth/rust-clippy/wiki#needless_return
[`needless_update`]: https://github.com/Manishearth/rust-clippy/wiki#needless_update
[`neg_multiply`]: https://github.com/Manishearth/rust-clippy/wiki#neg_multiply
+[`never_loop`]: https://github.com/Manishearth/rust-clippy/wiki#never_loop
[`new_ret_no_self`]: https://github.com/Manishearth/rust-clippy/wiki#new_ret_no_self
[`new_without_default`]: https://github.com/Manishearth/rust-clippy/wiki#new_without_default
[`new_without_default_derive`]: https://github.com/Manishearth/rust-clippy/wiki#new_without_default_derive
diff --git a/README.md b/README.md
index 0a6a62e9185..9e4b9fba1b6 100644
--- a/README.md
+++ b/README.md
@@ -180,7 +180,7 @@ transparently:
## Lints
-There are 189 lints included in this crate:
+There are 190 lints included in this crate:
name | default | triggers on
-----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -291,6 +291,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) | warn | any loop with an unconditional `break` statement
[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
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 80a369513e2..13ad8b41f96 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -405,6 +405,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,
diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs
index c863b11e0b8..2b175d14afd 100644
--- a/clippy_lints/src/loops.rs
+++ b/clippy_lints/src/loops.rs
@@ -286,6 +286,23 @@ 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`.
+///
+/// **Why is this bad?** This loop never loops, all it does is obfuscating the
+/// code.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// loop { ..; break; }
+/// ```
+declare_lint! {
+ pub NEVER_LOOP,
+ Warn,
+ "any loop with an unconditional `break` statement"
+}
+
#[derive(Copy, Clone)]
pub struct Pass;
@@ -303,7 +320,8 @@ impl LintPass for Pass {
EXPLICIT_COUNTER_LOOP,
EMPTY_LOOP,
WHILE_LET_ON_ITERATOR,
- FOR_KV_MAP)
+ FOR_KV_MAP,
+ NEVER_LOOP)
}
}
@@ -324,6 +342,9 @@ 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);
@@ -402,6 +423,51 @@ 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_stmt(stmt: &Stmt) -> bool {
+ match stmt.node {
+ StmtSemi(ref e, _) |
+ StmtExpr(ref e, _) => never_loop_expr(e),
+ StmtDecl(ref d, _) => never_loop_decl(d),
+ }
+}
+
+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 never_loop_expr(expr: &Expr) -> bool {
+ match expr.node {
+ ExprBreak(..) | ExprRet(..) => true,
+ ExprBox(ref e) |
+ ExprUnary(_, ref e) |
+ ExprCast(ref e, _) |
+ ExprType(ref e, _) |
+ ExprField(ref e, _) |
+ ExprTupField(ref e, _) |
+ ExprRepeat(ref e, _) |
+ ExprAddrOf(_, ref e) => never_loop_expr(e),
+ 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),
+ 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)),
+ _ => false,
+ }
+}
+
fn check_for_loop<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,
pat: &'tcx Pat,
diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs
new file mode 100644
index 00000000000..087f1a1e6af
--- /dev/null
+++ b/tests/ui/never_loop.rs
@@ -0,0 +1,34 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(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
+ if x == 1 {
+ break;
+ }
+ }
+
+ loop {
+ loop {
+ // another one
+ break;
+ }
+ break;
+ }
+
+ loop {
+ loop {
+ if x == 1 { return; }
+ }
+ }
+}
diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr
new file mode 100644
index 00000000000..ebb98a6cf14
--- /dev/null
+++ b/tests/ui/never_loop.stderr
@@ -0,0 +1,41 @@
+error: this loop never actually loops
+ --> $DIR/never_loop.rs:8:5
+ |
+8 | loop {
+ | _____^ starting here...
+9 | | println!("This is only ever printed once");
+10 | | break;
+11 | | }
+ | |_____^ ...ending here
+ |
+note: lint level defined here
+ --> $DIR/never_loop.rs:4:9
+ |
+4 | #![deny(never_loop)]
+ | ^^^^^^^^^^
+
+error: this loop never actually loops
+ --> $DIR/never_loop.rs:21:5
+ |
+21 | loop {
+ | _____^ starting here...
+22 | | loop {
+23 | | // another one
+24 | | break;
+25 | | }
+26 | | break;
+27 | | }
+ | |_____^ ...ending here
+
+error: this loop never actually loops
+ --> $DIR/never_loop.rs:22:9
+ |
+22 | loop {
+ | _________^ starting here...
+23 | | // another one
+24 | | break;
+25 | | }
+ | |_________^ ...ending here
+
+error: aborting due to 3 previous errors
+
diff --git a/tests/ui/unused_labels.rs b/tests/ui/unused_labels.rs
index ce718c4903a..1eb067e9684 100644
--- a/tests/ui/unused_labels.rs
+++ b/tests/ui/unused_labels.rs
@@ -1,7 +1,7 @@
#![plugin(clippy)]
#![feature(plugin)]
-#![allow(dead_code, items_after_statements)]
+#![allow(dead_code, items_after_statements, never_loop)]
#![deny(unused_label)]
fn unused_label() {
diff --git a/tests/ui/while_loop.rs b/tests/ui/while_loop.rs
index 2b75e5b83e8..253aebe4444 100644
--- a/tests/ui/while_loop.rs
+++ b/tests/ui/while_loop.rs
@@ -2,7 +2,7 @@
#![plugin(clippy)]
#![deny(while_let_loop, empty_loop, while_let_on_iterator)]
-#![allow(dead_code, unused, cyclomatic_complexity)]
+#![allow(dead_code, never_loop, unused, cyclomatic_complexity)]
fn main() {
let y = Some(true);
From 6c8a6c18ab3639c9f33bbc2fa39eaef97e6c6aac Mon Sep 17 00:00:00 2001
From: Andre Bogus <bogusandre@gmail.com>
Date: Fri, 17 Feb 2017 08:49:34 +0100
Subject: [PATCH 2/2] deal with binary op short-circuit
---
clippy_lints/src/loops.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs
index 2b175d14afd..d7f952255fe 100644
--- a/clippy_lints/src/loops.rs
+++ b/clippy_lints/src/loops.rs
@@ -448,13 +448,13 @@ fn never_loop_expr(expr: &Expr) -> bool {
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),
- 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),