new lint for "let x = EXPR; x" at the end of functions (fixes #104)

diff --git a/README.md b/README.md
index f0d282e450afc89e4dcc62ecc8d4ee364fd25a85..1176841ca201cdff012bc561c733709fe07681a3 100644 (file)
--- a/README.md
+++ b/README.md
@@ -31,6 +31,7 @@ Lints included in this crate:
  - `zero_width_space`: Warns on encountering a unicode zero-width space
  - `string_add_assign`: Warns on `x = x + ..` where `x` is a `String` and suggests using `push_str(..)` instead.
  - `needless_return`: Warns on using `return expr;` when a simple `expr` would suffice.
+ - `let_and_return`: Warns on doing `let x = expr; x` at the end of a function.
  - `option_unwrap_used`: Warns when `Option.unwrap()` is used, and suggests `.expect()`.
  - `result_unwrap_used`: Warns when `Result.unwrap()` is used (silent by default).
  - `modulo_one`: Warns on taking a number modulo 1, which always has a result of 0.

diff --git a/src/returns.rs b/src/returns.rs
index d6a4b33b6d1f7ad58bd13df146d7b460e64d2d78..9bfc99972c9698e80c24c72e901e25e9c81c94ee 100644 (file)
--- a/src/returns.rs
+++ b/src/returns.rs
@@ -1,13 +1,15 @@
 use syntax::ast;
 use syntax::ast::*;
-use syntax::codemap::Span;
+use syntax::codemap::{Span, Spanned};
 use syntax::visit::FnKind;
-use rustc::lint::{Context, LintPass, LintArray};
+use rustc::lint::{Context, LintPass, LintArray, Level};
 
-use utils::{span_lint, snippet};
+use utils::{span_lint, snippet, match_path};
 
 declare_lint!(pub NEEDLESS_RETURN, Warn,
               "Warn on using a return statement where an expression would be enough");
+declare_lint!(pub LET_AND_RETURN, Warn,
+              "Warn on creating a let-binding and then immediately returning it");
 
 #[derive(Copy,Clone)]
 pub struct ReturnPass;
@@ -20,7 +22,7 @@ fn check_block_return(&mut self, cx: &Context, block: &Block) {
         } else if let Some(stmt) = block.stmts.last() {
             if let StmtSemi(ref expr, _) = stmt.node {
                 if let ExprRet(Some(ref inner)) = expr.node {
-                    self.emit_lint(cx, (expr.span, inner.span));
+                    self.emit_return_lint(cx, (expr.span, inner.span));
                 }
             }
         }
@@ -31,7 +33,7 @@ fn check_final_expr(&mut self, cx: &Context, expr: &Expr) {
         match expr.node {
             // simple return is always "bad"
             ExprRet(Some(ref inner)) => {
-                self.emit_lint(cx, (expr.span, inner.span));
+                self.emit_return_lint(cx, (expr.span, inner.span));
             }
             // a whole block? check it!
             ExprBlock(ref block) => {
@@ -55,21 +57,54 @@ fn check_final_expr(&mut self, cx: &Context, expr: &Expr) {
         }
     }
 
-    fn emit_lint(&mut self, cx: &Context, spans: (Span, Span)) {
+    fn emit_return_lint(&mut self, cx: &Context, spans: (Span, Span)) {
         span_lint(cx, NEEDLESS_RETURN, spans.0, &format!(
             "unneeded return statement. Consider using {} \
              without the trailing semicolon",
             snippet(cx, spans.1, "..")))
     }
+
+    // Check for "let x = EXPR; x"
+    fn check_let_return(&mut self, cx: &Context, block: &Block) {
+        // we need both a let-binding stmt and an expr
+        if let Some(stmt) = block.stmts.last() {
+            if let StmtDecl(ref decl, _) = stmt.node {
+                if let DeclLocal(ref local) = decl.node {
+                    if let Some(ref initexpr) = local.init {
+                        if let PatIdent(_, Spanned { node: id, .. }, _) = local.pat.node {
+                            if let Some(ref retexpr) = block.expr {
+                                if let ExprPath(_, ref path) = retexpr.node {
+                                    if match_path(path, &[&*id.name.as_str()]) {
+                                        self.emit_let_lint(cx, retexpr.span, initexpr.span);
+                                    }
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    fn emit_let_lint(&mut self, cx: &Context, lint_span: Span, note_span: Span) {
+        span_lint(cx, LET_AND_RETURN, lint_span,
+                  "returning the result of a let binding. \
+                   Consider returning the expression directly.");
+        if cx.current_level(LET_AND_RETURN) != Level::Allow {
+            cx.sess().span_note(note_span,
+                                "this expression can be directly returned");
+        }
+    }
 }
 
 impl LintPass for ReturnPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(NEEDLESS_RETURN)
+        lint_array!(NEEDLESS_RETURN, LET_AND_RETURN)
     }
 
     fn check_fn(&mut self, cx: &Context, _: FnKind, _: &FnDecl,
                 block: &Block, _: Span, _: ast::NodeId) {
         self.check_block_return(cx, block);
+        self.check_let_return(cx, block);
     }
 }

diff --git a/tests/compile-fail/let_return.rs b/tests/compile-fail/let_return.rs
new file mode 100755 (executable)
index 0000000..8ea4653
--- /dev/null
+++ b/tests/compile-fail/let_return.rs
@@ -0,0 +1,34 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(let_and_return)]
+
+fn test() -> i32 {
+    let _y = 0; // no warning
+    let x = 5;   //~NOTE
+    x            //~ERROR:
+}
+
+fn test_nowarn_1() -> i32 {
+    let mut x = 5;
+    x += 1;
+    x
+}
+
+fn test_nowarn_2() -> i32 {
+    let x = 5;
+    x + 1
+}
+
+fn test_nowarn_3() -> (i32, i32) {
+    // this should technically warn, but we do not compare complex patterns
+    let (x, y) = (5, 9);
+    (x, y)
+}
+
+fn main() {
+    test();
+    test_nowarn_1();
+    test_nowarn_2();
+    test_nowarn_3();
+}