diff --git a/CHANGELOG.md b/CHANGELOG.md index 4aaaceb3b..b59249b25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ # Change Log All notable changes to this project will be documented in this file. +## 0.0.71 — TBD +* New lint: [`useless_let_if_seq`] + ## 0.0.70 — 2016-05-28 * Rustup to *rustc 1.10.0-nightly (7bddce693 2016-05-27)* * [`invalid_regex`] and [`trivial_regex`] can now warn on `RegexSet::new`, @@ -240,6 +243,7 @@ All notable changes to this project will be documented in this file. [`use_debug`]: https://github.com/Manishearth/rust-clippy/wiki#use_debug [`used_underscore_binding`]: https://github.com/Manishearth/rust-clippy/wiki#used_underscore_binding [`useless_format`]: https://github.com/Manishearth/rust-clippy/wiki#useless_format +[`useless_let_if_seq`]: https://github.com/Manishearth/rust-clippy/wiki#useless_let_if_seq [`useless_transmute`]: https://github.com/Manishearth/rust-clippy/wiki#useless_transmute [`useless_vec`]: https://github.com/Manishearth/rust-clippy/wiki#useless_vec [`while_let_loop`]: https://github.com/Manishearth/rust-clippy/wiki#while_let_loop diff --git a/README.md b/README.md index c28911680..3b5b6768c 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Table of contents: ## Lints -There are 151 lints included in this crate: +There are 152 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -164,6 +164,7 @@ name [use_debug](https://github.com/Manishearth/rust-clippy/wiki#use_debug) | allow | use `Debug`-based formatting [used_underscore_binding](https://github.com/Manishearth/rust-clippy/wiki#used_underscore_binding) | allow | using a binding which is prefixed with an underscore [useless_format](https://github.com/Manishearth/rust-clippy/wiki#useless_format) | warn | useless use of `format!` +[useless_let_if_seq](https://github.com/Manishearth/rust-clippy/wiki#useless_let_if_seq) | warn | Checks for unidiomatic `let mut` declaration followed by initialization in `if` [useless_transmute](https://github.com/Manishearth/rust-clippy/wiki#useless_transmute) | warn | transmutes that have the same to and from types [useless_vec](https://github.com/Manishearth/rust-clippy/wiki#useless_vec) | warn | useless `vec!` [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 diff --git a/clippy_lints/src/let_if_seq.rs b/clippy_lints/src/let_if_seq.rs new file mode 100644 index 000000000..29551a413 --- /dev/null +++ b/clippy_lints/src/let_if_seq.rs @@ -0,0 +1,176 @@ +use rustc::lint::*; +use rustc::hir; +use syntax::codemap; +use utils::{snippet, span_lint_and_then}; + +/// **What it does:** This lint checks for variable declarations immediately followed by a +/// conditional affectation. +/// +/// **Why is this bad?** This is not idiomatic Rust. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust,ignore +/// let foo; +/// +/// if bar() { +/// foo = 42; +/// } else { +/// foo = 0; +/// } +/// +/// let mut baz = None; +/// +/// if bar() { +/// baz = Some(42); +/// } +/// ``` +/// +/// should be written +/// +/// ```rust,ignore +/// let foo = if bar() { +/// 42; +/// } else { +/// 0; +/// }; +/// +/// let baz = if bar() { +/// Some(42); +/// } else { +/// None +/// }; +/// ``` +declare_lint! { + pub USELESS_LET_IF_SEQ, + Warn, + "Checks for unidiomatic `let mut` declaration followed by initialization in `if`" +} + +#[derive(Copy,Clone)] +pub struct LetIfSeq; + +impl LintPass for LetIfSeq { + fn get_lints(&self) -> LintArray { + lint_array!(USELESS_LET_IF_SEQ) + } +} + +impl LateLintPass for LetIfSeq { + fn check_block(&mut self, cx: &LateContext, block: &hir::Block) { + let mut it = block.stmts.iter().peekable(); + while let Some(ref stmt) = it.next() { + if_let_chain! {[ + let Some(expr) = it.peek(), + let hir::StmtDecl(ref decl, _) = stmt.node, + let hir::DeclLocal(ref decl) = decl.node, + let hir::PatKind::Ident(mode, ref name, None) = decl.pat.node, + let Some(def) = cx.tcx.def_map.borrow().get(&decl.pat.id), + let hir::StmtExpr(ref if_, _) = expr.node, + let hir::ExprIf(ref cond, ref then, ref else_) = if_.node, + let Some(value) = check_assign(cx, def.def_id(), then), + ], { + let span = codemap::mk_sp(stmt.span.lo, if_.span.hi); + + let (default_multi_stmts, default) = if let Some(ref else_) = *else_ { + if let hir::ExprBlock(ref else_) = else_.node { + if let Some(default) = check_assign(cx, def.def_id(), else_) { + (else_.stmts.len() > 1, default) + } else if let Some(ref default) = decl.init { + (true, &**default) + } else { + continue; + } + } else { + continue; + } + } else if let Some(ref default) = decl.init { + (false, &**default) + } else { + continue; + }; + + let mutability = match mode { + hir::BindByRef(hir::MutMutable) | hir::BindByValue(hir::MutMutable) => " ", + _ => "", + }; + + // FIXME: this should not suggest `mut` if we can detect that the variable is not + // use mutably after the `if` + + let sug = format!( + "let {mut}{name} = if {cond} {{{then} {value} }} else {{{else} {default} }};", + mut=mutability, + name=name.node, + cond=snippet(cx, cond.span, "_"), + then=if then.stmts.len() > 1 { " ..;" } else { "" }, + else=if default_multi_stmts { " ..;" } else { "" }, + value=snippet(cx, value.span, ""), + default=snippet(cx, default.span, ""), + ); + span_lint_and_then(cx, + USELESS_LET_IF_SEQ, + span, + "`if _ { .. } else { .. }` is an expression", + |db| { + db.span_suggestion(span, + "it is more idiomatic to write", + sug); + if !mutability.is_empty() { + db.note("you might not need `mut` at all"); + } + }); + }} + } + } +} + +struct UsedVisitor<'a, 'tcx: 'a> { + cx: &'a LateContext<'a, 'tcx>, + id: hir::def_id::DefId, + used: bool, +} + +impl<'a, 'tcx, 'v> hir::intravisit::Visitor<'v> for UsedVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'v hir::Expr) { + if_let_chain! {[ + let hir::ExprPath(None, _) = expr.node, + let Some(def) = self.cx.tcx.def_map.borrow().get(&expr.id), + self.id == def.def_id(), + ], { + self.used = true; + return; + }} + hir::intravisit::walk_expr(self, expr); + } +} + +fn check_assign<'e>(cx: &LateContext, decl: hir::def_id::DefId, block: &'e hir::Block) -> Option<&'e hir::Expr> { + if_let_chain! {[ + let Some(expr) = block.stmts.iter().last(), + let hir::StmtSemi(ref expr, _) = expr.node, + let hir::ExprAssign(ref var, ref value) = expr.node, + let hir::ExprPath(None, _) = var.node, + let Some(def) = cx.tcx.def_map.borrow().get(&var.id), + decl == def.def_id(), + ], { + let mut v = UsedVisitor { + cx: cx, + id: decl, + used: false, + }; + + for s in block.stmts.iter().take(block.stmts.len()-1) { + hir::intravisit::walk_stmt(&mut v, s); + } + + return if v.used { + None + } else { + Some(value) + }; + }} + + None +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f6db7a3a1..4a5c3a87c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -80,6 +80,7 @@ pub mod identity_op; pub mod if_not_else; pub mod items_after_statements; pub mod len_zero; +pub mod let_if_seq; pub mod lifetimes; pub mod loops; pub mod map_clone; @@ -246,6 +247,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box mem_forget::MemForget); reg.register_late_lint_pass(box arithmetic::Arithmetic::default()); reg.register_late_lint_pass(box assign_ops::AssignOps); + reg.register_late_lint_pass(box let_if_seq::LetIfSeq); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -318,6 +320,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { identity_op::IDENTITY_OP, len_zero::LEN_WITHOUT_IS_EMPTY, len_zero::LEN_ZERO, + let_if_seq::USELESS_LET_IF_SEQ, lifetimes::NEEDLESS_LIFETIMES, lifetimes::UNUSED_LIFETIMES, loops::EMPTY_LOOP, diff --git a/tests/compile-fail/let_if_seq.rs b/tests/compile-fail/let_if_seq.rs new file mode 100644 index 000000000..011848e95 --- /dev/null +++ b/tests/compile-fail/let_if_seq.rs @@ -0,0 +1,79 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![allow(unused_variables, unused_assignments, similar_names, blacklisted_name)] +#![deny(useless_let_if_seq)] + +fn f() -> bool { true } + +fn early_return() -> u8 { + // FIXME: we could extend the lint to include such cases: + let foo; + + if f() { + return 42; + } else { + foo = 0; + } + + foo +} + +fn main() { + early_return(); + + let mut foo = 0; + //~^ ERROR `if _ { .. } else { .. }` is an expression + //~| HELP more idiomatic + //~| SUGGESTION let foo = if f() { 42 } else { 0 }; + if f() { + foo = 42; + } + + let mut bar = 0; + //~^ ERROR `if _ { .. } else { .. }` is an expression + //~| HELP more idiomatic + //~| SUGGESTION let bar = if f() { ..; 42 } else { ..; 0 }; + if f() { + f(); + bar = 42; + } + else { + f(); + } + + let quz; + //~^ ERROR `if _ { .. } else { .. }` is an expression + //~| HELP more idiomatic + //~| SUGGESTION let quz = if f() { 42 } else { 0 }; + + if f() { + quz = 42; + } else { + quz = 0; + } + + // `toto` is used several times + let mut toto; + + if f() { + toto = 42; + } else { + for i in &[1, 2] { + toto = *i; + } + + toto = 2; + } + + // baz needs to be mut + let mut baz = 0; + //~^ ERROR `if _ { .. } else { .. }` is an expression + //~| HELP more idiomatic + //~| SUGGESTION let baz = if f() { 42 } else { 0 }; + if f() { + baz = 42; + } + + baz = 1337; +}