Merge pull request #129 from birkenfeld/unwrap_lint

Lint for Result/Option.unwrap()

src/len_zero.rs

@@ -9,8 +9,7 @@ use rustc::middle::ty::{self, TypeVariants, TypeAndMut, MethodTraitItemId, ImplO
 use rustc::middle::def::{DefTy, DefStruct, DefTrait};
 use syntax::codemap::{Span, Spanned};
 use syntax::ast::*;
-use misc::walk_ty;
-use utils::span_lint;
+use utils::{span_lint, walk_ptrs_ty};
 
 declare_lint!(pub LEN_ZERO, Warn,
               "Warn when .is_empty() could be used instead of checking .len()");
@@ -136,7 +135,7 @@ fn has_is_empty(cx: &Context, expr: &Expr) -> bool {
                 |iids| iids.iter().any(|i| is_is_empty(cx, i)))))
     }
 
-    let ty = &walk_ty(&cx.tcx.expr_ty(expr));
+    let ty = &walk_ptrs_ty(&cx.tcx.expr_ty(expr));
     match ty.sty {
         ty::TyTrait(_) => cx.tcx.trait_item_def_ids.borrow().get(
             &ty.ty_to_def_id().expect("trait impl not found")).map_or(false,

src/lib.rs

@@ -29,6 +29,7 @@ pub mod collapsible_if;
 pub mod unicode;
 pub mod utils;
 pub mod strings;
+pub mod methods;
 
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {
@@ -55,6 +56,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_lint_pass(box unicode::Unicode as LintPassObject);
     reg.register_lint_pass(box strings::StringAdd as LintPassObject);
     reg.register_lint_pass(box misc::NeedlessReturn as LintPassObject);
+    reg.register_lint_pass(box methods::MethodsPass as LintPassObject);
 
     reg.register_lint_group("clippy", vec![types::BOX_VEC, types::LINKEDLIST,
                                            misc::SINGLE_MATCH, misc::STR_TO_STRING,
@@ -77,5 +79,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
                                            strings::STRING_ADD_ASSIGN,
                                            misc::NEEDLESS_RETURN,
                                            misc::MODULO_ONE,
+                                           methods::OPTION_UNWRAP_USED,
+                                           methods::RESULT_UNWRAP_USED,
                                            ]);
 }

src/methods.rs

@@ -0,0 +1,39 @@
+use syntax::ast::*;
+use rustc::lint::{Context, LintPass, LintArray};
+use rustc::middle::ty;
+
+use utils::{span_lint, match_def_path, walk_ptrs_ty};
+
+#[derive(Copy,Clone)]
+pub struct MethodsPass;
+
+declare_lint!(pub OPTION_UNWRAP_USED, Warn,
+              "Warn on using unwrap() on an Option value");
+declare_lint!(pub RESULT_UNWRAP_USED, Allow,
+              "Warn on using unwrap() on a Result value");
+
+impl LintPass for MethodsPass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED)
+    }
+
+    fn check_expr(&mut self, cx: &Context, expr: &Expr) {
+        if let ExprMethodCall(ref ident, _, ref args) = expr.node {
+            if ident.node.name == "unwrap" {
+                if let ty::TyEnum(did, _) = walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])).sty {
+                    if match_def_path(cx, did.did, &["core", "option", "Option"]) {
+                        span_lint(cx, OPTION_UNWRAP_USED, expr.span,
+                                  "used unwrap() on an Option value. If you don't want \
+                                   to handle the None case gracefully, consider using
+                                   expect() to provide a better panic message.");
+                    }
+                    else if match_def_path(cx, did.did, &["core", "result", "Result"]) {
+                        span_lint(cx, RESULT_UNWRAP_USED, expr.span,
+                                  "used unwrap() on a Result value. Graceful handling \
+                                   of Err values is preferred.");
+                    }
+                }
+            }
+        }
+    }
+}

src/misc.rs

@@ -7,14 +7,7 @@ use rustc::lint::{Context, LintPass, LintArray, Lint, Level};
 use rustc::middle::ty;
 use syntax::codemap::{Span, Spanned};
 
-use utils::{match_path, snippet, span_lint, span_help_and_lint};
-
-pub fn walk_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> {
-    match ty.sty {
-        ty::TyRef(_, ref tm) | ty::TyRawPtr(ref tm) => walk_ty(tm.ty),
-        _ => ty
-    }
-}
+use utils::{match_path, snippet, span_lint, span_help_and_lint, walk_ptrs_ty};
 
 /// Handles uncategorized lints
 /// Currently handles linting of if-let-able matches
@@ -87,7 +80,7 @@ impl LintPass for StrToStringPass {
         }
 
         fn is_str(cx: &Context, expr: &ast::Expr) -> bool {
-            match walk_ty(cx.tcx.expr_ty(expr)).sty {
+            match walk_ptrs_ty(cx.tcx.expr_ty(expr)).sty {
                 ty::TyStr => true,
                 _ => false
             }
@@ -175,7 +168,7 @@ impl LintPass for FloatCmp {
 }
 
 fn is_float(cx: &Context, expr: &Expr) -> bool {
-    if let ty::TyFloat(_) = walk_ty(cx.tcx.expr_ty(expr)).sty {
+    if let ty::TyFloat(_) = walk_ptrs_ty(cx.tcx.expr_ty(expr)).sty {
         true
     } else {
         false
@@ -274,7 +267,7 @@ fn check_to_owned(cx: &Context, expr: &Expr, other_span: Span) {
 
 fn is_str_arg(cx: &Context, args: &[P<Expr>]) -> bool {
     args.len() == 1 && if let ty::TyStr =
-        walk_ty(cx.tcx.expr_ty(&*args[0])).sty { true } else { false }
+        walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])).sty { true } else { false }
 }
 
 declare_lint!(pub NEEDLESS_RETURN, Warn,

src/strings.rs

@@ -8,9 +8,8 @@ use rustc::middle::ty::TypeVariants::TyStruct;
 use syntax::ast::*;
 use syntax::codemap::{Span, Spanned};
 use eq_op::is_exp_equal;
-use misc::walk_ty;
 use types::match_ty_unwrap;
-use utils::{match_def_path, span_lint};
+use utils::{match_def_path, span_lint, walk_ptrs_ty};
 
 declare_lint! {
     pub STRING_ADD_ASSIGN,
@@ -38,7 +37,7 @@ impl LintPass for StringAdd {
 }
 
 fn is_string(cx: &Context, e: &Expr) -> bool {
-    if let TyStruct(did, _) = walk_ty(cx.tcx.expr_ty(e)).sty {
+    if let TyStruct(did, _) = walk_ptrs_ty(cx.tcx.expr_ty(e)).sty {
         match_def_path(cx, did.did, &["std", "string", "String"])
     } else { false }
 }

src/utils.rs

@@ -84,3 +84,11 @@ pub fn span_help_and_lint(cx: &Context, lint: &'static Lint, span: Span,
         cx.sess().fileline_help(span, help);
     }
 }
+
+/// return the base type for references and raw pointers
+pub fn walk_ptrs_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> {
+    match ty.sty {
+        ty::TyRef(_, ref tm) | ty::TyRawPtr(ref tm) => walk_ptrs_ty(tm.ty),
+        _ => ty
+    }
+}

tests/compile-fail/methods.rs

@@ -0,0 +1,11 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#[deny(option_unwrap_used, result_unwrap_used)]
+fn main() {
+    let opt = Some(0);
+    let _ = opt.unwrap();  //~ERROR
+
+    let res: Result<i32, ()> = Ok(0);
+    let _ = res.unwrap();  //~ERROR
+}