warn on use of ok().expect()

diff --git a/src/lib.rs b/src/lib.rs
index afbf3d2a92c9d4894dafb4882cb767e948ffeafe..35e303b749b7ea431437b41cc6065045f2936259 100644 (file)
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -85,7 +85,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_late_lint_pass(box unicode::Unicode);
     reg.register_late_lint_pass(box strings::StringAdd);
     reg.register_early_lint_pass(box returns::ReturnPass);
-    reg.register_late_lint_pass(box methods::MethodsPass);
+    reg.register_late_lint_pass(box methods::MethodsPass::new());
     reg.register_late_lint_pass(box shadow::ShadowPass);
     reg.register_late_lint_pass(box types::LetPass);
     reg.register_late_lint_pass(box types::UnitCmp);
@@ -151,6 +151,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         matches::MATCH_BOOL,
         matches::MATCH_REF_PATS,
         matches::SINGLE_MATCH,
+        methods::OK_EXPECT,
         methods::SHOULD_IMPLEMENT_TRAIT,
         methods::STR_TO_STRING,
         methods::STRING_TO_STRING,

diff --git a/src/methods.rs b/src/methods.rs
index 275de8d8ebdc8ddae2a57ad4f83fd7af6fcf5d61..dbc18fdfe33ceb3ea6821d809c948cdb67dcc7b7 100644 (file)
--- a/src/methods.rs
+++ b/src/methods.rs
@@ -1,18 +1,81 @@
 use rustc_front::hir::*;
 use rustc::lint::*;
 use rustc::middle::ty;
-use rustc::middle::subst::Subst;
+use rustc::middle::subst::{Subst, TypeSpace};
 use std::iter;
 use std::borrow::Cow;
+use std::collections::HashSet;
 
-use utils::{snippet, span_lint, match_path, match_type, walk_ptrs_ty_depth};
+use utils::{snippet, span_lint, match_path, match_type, walk_ptrs_ty_depth,
+    walk_ptrs_ty};
 use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH};
 
 use self::SelfKind::*;
 use self::OutType::*;
 
-#[derive(Copy,Clone)]
-pub struct MethodsPass;
+use rustc::middle::def_id::DefId;
+
+use rustc::middle::ty::TypeFlags;
+
+#[derive(Clone)]
+pub struct MethodsPass { types_implementing_debug: Option<HashSet<DefId>> }
+
+impl MethodsPass {
+    pub fn new() -> MethodsPass {
+        MethodsPass { types_implementing_debug: None }
+    }
+
+    fn get_debug_impls(&mut self, cx: &LateContext) -> Option<&HashSet<DefId>> {
+        if self.types_implementing_debug.is_none() {
+            let debug = match cx.tcx.lang_items.debug_trait() {
+                Some(debug) => debug,
+                None => return None
+            };
+            let debug_def = cx.tcx.lookup_trait_def(debug);
+            let mut impls = HashSet::new();
+            debug_def.for_each_impl(cx.tcx, |d| {
+                let o_self_ty = &cx.tcx.impl_trait_ref(d)
+                                    .map(|x| x.substs)
+                                    .and_then(|x| x.self_ty());
+                let self_ty = match *o_self_ty {
+                    Some(self_type) => self_type,
+                    None => return
+                };
+                let self_ty_def_id = self_ty.ty_to_def_id();
+                if let Some(self_ty_def_id) = self_ty_def_id {
+                    let has_params = self_ty.flags.get().contains(TypeFlags::HAS_PARAMS);
+                    if !has_params {
+                        impls.insert(self_ty_def_id);
+                    }
+                }
+            });
+            self.types_implementing_debug = Some(impls);
+        }
+        self.types_implementing_debug.as_ref()
+    }
+
+    // This checks whether a given type is known to implement Debug. It's
+    // conservative, i.e. it should not return false positives, but will return
+    // false negatives.
+    fn has_debug_impl(&mut self, ty: ty::Ty, cx: &LateContext) -> bool {
+        let debug_impls = match self.get_debug_impls(cx) {
+            Some(debug_impls) => debug_impls,
+            None => return false
+        };
+        match walk_ptrs_ty(ty).sty {
+            ty::TyBool | ty::TyChar | ty::TyInt(..) | ty::TyUint(..)
+                       | ty::TyFloat(..) | ty::TyStr => true,
+            ty::TyTuple(ref v) if v.is_empty() => true,
+            ty::TyStruct(..) | ty::TyEnum(..) => {
+                match ty.ty_to_def_id() {
+                    Some(ref ty_def_id) => debug_impls.contains(ty_def_id),
+                    None => false
+                }
+            },
+            _ => false
+        }
+    }
+}
 
 declare_lint!(pub OPTION_UNWRAP_USED, Allow,
               "using `Option.unwrap()`, which should at least get a better message using `expect()`");
@@ -30,16 +93,21 @@
 declare_lint!(pub WRONG_PUB_SELF_CONVENTION, Allow,
               "defining a public method named with an established prefix (like \"into_\") that takes \
                `self` with the wrong convention");
+declare_lint!(pub OK_EXPECT, Warn,
+              "using `ok().expect()`, which gives worse error messages than \
+               calling `expect` directly on the Result");
+
 
 impl LintPass for MethodsPass {
     fn get_lints(&self) -> LintArray {
         lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING,
-                    SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION)
+                    SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION, OK_EXPECT)
     }
 }
 
 impl LateLintPass for MethodsPass {
     fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+
         if let ExprMethodCall(ref name, _, ref args) = expr.node {
             let (obj_ty, ptr_depth) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0]));
             if name.node.as_str() == "unwrap" {
@@ -70,6 +138,22 @@ fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
                                                                 `clone()` to make a copy");
                 }
             }
+            else if name.node.as_str() == "expect" {
+                if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node {
+                    if inner_name.node.as_str() == "ok"
+                            && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &RESULT_PATH) {
+                        let result_type = cx.tcx.expr_ty(&inner_args[0]);
+                        if let Some(error_type) = get_error_type(cx, result_type) {
+                            if self.has_debug_impl(error_type, cx) {
+                                span_lint(cx, OK_EXPECT, expr.span,
+                                         "called `ok().expect()` on a Result \
+                                          value. You can call `expect` directly
+                                          on the `Result`");
+                            }
+                        }
+                    }
+                }
+            }
         }
     }
 
@@ -115,6 +199,20 @@ fn check_item(&mut self, cx: &LateContext, item: &Item) {
     }
 }
 
+// Given a `Result<T, E>` type, return its error type (`E`)
+fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option<ty::Ty<'a>> {
+    if !match_type(cx, ty, &RESULT_PATH) {
+        return None;
+    }
+    if let ty::TyEnum(_, substs) = ty.sty {
+        if let Some(err_ty) = substs.types.opt_get(TypeSpace, 1) {
+            return Some(err_ty);
+        }
+    }
+    None
+}
+
+
 const CONVENTIONS: [(&'static str, &'static [SelfKind]); 5] = [
     ("into_", &[ValueSelf]),
     ("to_",   &[RefSelf]),

diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs
index 314601f6dbd3662435898de04a0223363403d8ba..aeb79503504b8bc82aa79a2f12c68eea3d3cdc80 100644 (file)
--- a/tests/compile-fail/methods.rs
+++ b/tests/compile-fail/methods.rs
@@ -35,6 +35,8 @@ fn mul(self, other: T) -> T { self } // no error, obviously
 }
 
 fn main() {
+    use std::io;
+
     let opt = Some(0);
     let _ = opt.unwrap();  //~ERROR used unwrap() on an Option
 
@@ -46,4 +48,25 @@ fn main() {
     let v = &"str";
     let string = v.to_string();  //~ERROR `(*v).to_owned()` is faster
     let _again = string.to_string();  //~ERROR `String.to_string()` is a no-op
+
+    res.ok().expect("disaster!"); //~ERROR called `ok().expect()`
+    // the following should not warn, since `expect` isn't implemented unless
+    // the error type implements `Debug`
+    let res2: Result<i32, MyError> = Ok(0);
+    res2.ok().expect("oh noes!");
+    // we're currently don't warn if the error type has a type parameter
+    // (but it would be nice if we did)
+    let res3: Result<u32, MyErrorWithParam<u8>>= Ok(0);
+    res3.ok().expect("whoof");
+    let res4: Result<u32, io::Error> = Ok(0);
+    res4.ok().expect("argh"); //~ERROR called `ok().expect()`
+    let res5: io::Result<u32> = Ok(0);
+    res5.ok().expect("oops"); //~ERROR called `ok().expect()`
+}
+
+struct MyError(()); // doesn't implement Debug
+
+#[derive(Debug)]
+struct MyErrorWithParam<T> {
+    x: T
 }