Add a USELESS_LET_IF_SEQ lint

2024-11-26 06:30:32 +00:00 · 2016-03-30 19:53:43 +02:00 · 2016-03-30 19:53:43 +02:00 · 42879bcdcb
commit 42879bcdcb
parent ed7ac0d9b5
5 changed files with 264 additions and 1 deletions
--- 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
--- 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
--- a/clippy_lints/src/let_if_seq.rs
+++ 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) => "<mut> ",
+                    _ => "",
+                };
+
+                // 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, "<value>"),
+                    default=snippet(cx, default.span, "<default>"),
+                );
+                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
+}
--- 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,
--- a/tests/compile-fail/let_if_seq.rs
+++ 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 <mut> 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 <mut> 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 <mut> baz = if f() { 42 } else { 0 };
+    if f() {
+        baz = 42;
+    }
+
+    baz = 1337;
+}