OpenE2K/rust
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

new lint: loop-match-break, which could be while-let (fixes #118)

Browse Source
This commit is contained in:
Georg Brandl 2015-08-29 11:41:06 +02:00
parent 92563a9970
commit b72ef5a173
6 changed files with 129 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
README.md
View File

@ -4,7 +4,7 @@
A collection of lints that give helpful tips to newbies and catch oversights.
##Lints
There are 49 lints included in this crate:
There are 50 lints included in this crate:
name | default | meaning
-----------------------------------------------------------------------------------------------------|---------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@ -56,6 +56,7 @@ name
[toplevel_ref_arg](https://github.com/Manishearth/rust-clippy/wiki#toplevel_ref_arg) | warn | a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not `fn foo((ref x, ref y): (u8, u8))`)
[type_complexity](https://github.com/Manishearth/rust-clippy/wiki#type_complexity) | warn | usage of very complex types; recommends factoring out parts into `type` definitions
[unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp) | warn | comparing unit values (which is always `true` or `false`, respectively)
[while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop) | warn | `loop { if let { ... } else break }` can be written as a `while let` loop
[zero_width_space](https://github.com/Manishearth/rust-clippy/wiki#zero_width_space) | deny | using a zero-width space in a string literal, which is confusing
More to come, please [file an issue](https://github.com/Manishearth/rust-clippy/issues) if you have ideas!

1
src/lib.rs
View File

@ -96,6 +96,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
loops::EXPLICIT_ITER_LOOP,
loops::ITER_NEXT_LOOP,
loops::NEEDLESS_RANGE_LOOP,
loops::WHILE_LET_LOOP,
matches::MATCH_REF_PATS,
matches::SINGLE_MATCH,
methods::OPTION_UNWRAP_USED,

65
src/loops.rs
View File

@ -4,7 +4,8 @@ use syntax::visit::{Visitor, walk_expr};
use rustc::middle::ty;
use std::collections::HashSet;
use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, walk_ptrs_ty};
use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, walk_ptrs_ty,
in_external_macro, expr_block, span_help_and_lint};
use utils::{VEC_PATH, LL_PATH};
declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn,
@ -16,12 +17,16 @@ declare_lint!{ pub EXPLICIT_ITER_LOOP, Warn,
declare_lint!{ pub ITER_NEXT_LOOP, Warn,
"for-looping over `_.next()` which is probably not intended" }
declare_lint!{ pub WHILE_LET_LOOP, Warn,
"`loop { if let { ... } else break }` can be written as a `while let` loop" }
#[derive(Copy, Clone)]
pub struct LoopsPass;
impl LintPass for LoopsPass {
fn get_lints(&self) -> LintArray {
lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP)
lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP,
WHILE_LET_LOOP)
}
fn check_expr(&mut self, cx: &Context, expr: &Expr) {
@ -36,7 +41,8 @@ impl LintPass for LoopsPass {
walk_expr(&mut visitor, body);
// linting condition: we only indexed one variable
if visitor.indexed.len() == 1 {
let indexed = visitor.indexed.into_iter().next().expect("Len was nonzero, but no contents found");
let indexed = visitor.indexed.into_iter().next().expect(
"Len was nonzero, but no contents found");
if visitor.nonindex {
span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!(
"the loop variable `{}` is used to index `{}`. Consider using \
@ -77,6 +83,34 @@ impl LintPass for LoopsPass {
}
}
}
// check for `loop { if let {} else break }` that could be `while let`
// (also matches explicit "match" instead of "if let")
if let ExprLoop(ref block, _) = expr.node {
// extract a single expression
if let Some(inner) = extract_single_expr(block) {
if let ExprMatch(ref matchexpr, ref arms, ref source) = inner.node {
// ensure "if let" compatible match structure
match *source {
MatchSource::Normal | MatchSource::IfLetDesugar{..} => if
arms.len() == 2 &&
arms[0].pats.len() == 1 && arms[0].guard.is_none() &&
arms[1].pats.len() == 1 && arms[1].guard.is_none() &&
// finally, check for "break" in the second clause
is_break_expr(&arms[1].body)
{
if in_external_macro(cx, expr.span) { return; }
span_help_and_lint(cx, WHILE_LET_LOOP, expr.span,
"this loop could be written as a `while let` loop",
&format!("try\nwhile let {} = {} {}",
snippet(cx, arms[0].pats[0].span, ".."),
snippet(cx, matchexpr.span, ".."),
expr_block(cx, &arms[0].body, "..")));
},
_ => ()
}
}
}
}
}
}
@ -158,3 +192,28 @@ fn is_array(ty: ty::Ty) -> bool {
_ => false
}
}
/// If block consists of a single expression (with or without semicolon), return it.
fn extract_single_expr(block: &Block) -> Option<&Expr> {
match (&block.stmts.len(), &block.expr) {
(&1, &None) => match block.stmts[0].node {
StmtExpr(ref expr, _) |
StmtSemi(ref expr, _) => Some(expr),
_ => None,
},
(&0, &Some(ref expr)) => Some(expr),
_ => None
}
}
/// Return true if expr contains a single break expr (maybe within a block).
fn is_break_expr(expr: &Expr) -> bool {
match expr.node {
ExprBreak(None) => true,
ExprBlock(ref b) => match extract_single_expr(b) {
Some(ref subexpr) => is_break_expr(subexpr),
None => false,
},
_ => false,
}
}

13
src/matches.rs
View File

@ -1,10 +1,8 @@
use rustc::lint::*;
use syntax::ast;
use syntax::ast::*;
use std::borrow::Cow;
use utils::{snippet, snippet_block};
use utils::{span_lint, span_help_and_lint, in_external_macro};
use utils::{snippet, span_lint, span_help_and_lint, in_external_macro, expr_block};
declare_lint!(pub SINGLE_MATCH, Warn,
"a match statement with a single nontrivial arm (i.e, where the other arm \
@ -38,12 +36,6 @@ impl LintPass for MatchPass {
is_unit_expr(&arms[1].body)
{
if in_external_macro(cx, expr.span) {return;}
let body_code = snippet_block(cx, arms[0].body.span, "..");
let body_code = if let ExprBlock(_) = arms[0].body.node {
body_code
} else {
Cow::Owned(format!("{{ {} }}", body_code))
};
span_help_and_lint(cx, SINGLE_MATCH, expr.span,
"you seem to be trying to use match for \
destructuring a single pattern. Did you mean to \
@ -51,8 +43,7 @@ impl LintPass for MatchPass {
&format!("try\nif let {} = {} {}",
snippet(cx, arms[0].pats[0].span, ".."),
snippet(cx, ex.span, ".."),
body_code)
);
expr_block(cx, &arms[0].body, "..")));
}
// check preconditions for MATCH_REF_PATS

10
src/utils.rs
View File

@ -103,6 +103,16 @@ pub fn snippet_block<'a>(cx: &Context, span: Span, default: &'a str) -> Cow<'a,
trim_multiline(snip, true)
}
/// Like snippet_block, but add braces if the expr is not an ExprBlock
pub fn expr_block<'a>(cx: &Context, expr: &Expr, default: &'a str) -> Cow<'a, str> {
let code = snippet_block(cx, expr.span, default);
if let ExprBlock(_) = expr.node {
code
} else {
Cow::Owned(format!("{{ {} }}", code))
}
}
/// Trim indentation from a multiline string
/// with possibility of ignoring the first line
pub fn trim_multiline(s: Cow<str>, ignore_first: bool) -> Cow<str> {

52
tests/compile-fail/while_loop.rs Executable file
View File

@ -0,0 +1,52 @@
#![feature(plugin)]
#![plugin(clippy)]
#[deny(while_let_loop)]
fn main() {
let y = Some(true);
loop { //~ERROR
if let Some(_x) = y {
let _v = 1;
} else {
break;
}
}
loop { //~ERROR
if let Some(_x) = y {
let _v = 1;
} else {
break
}
}
loop { // no error, break is not in else clause
if let Some(_x) = y {
let _v = 1;
}
break;
}
loop { //~ERROR
match y {
Some(_x) => true,
None => break
};
}
loop { // no error, match is not the only statement
match y {
Some(_x) => true,
None => break
};
let _x = 1;
}
loop { // no error, else branch does something other than break
match y {
Some(_x) => true,
_ => {
let _z = 1;
break;
}
};
}
while let Some(x) = y { // no error, obviously
println!("{}", x);
}
}