From cad88a91371a988a0078016c44b1d6f3c24dec57 Mon Sep 17 00:00:00 2001 From: Florian Hartwig Date: Thu, 19 Nov 2015 14:39:27 +0100 Subject: [PATCH] warn on use of ok().expect() --- src/lib.rs | 3 +- src/methods.rs | 108 ++++++++++++++++++++++++++++++++-- tests/compile-fail/methods.rs | 23 ++++++++ 3 files changed, 128 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index afbf3d2a92c..35e303b749b 100644 --- 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 275de8d8ebd..dbc18fdfe33 100644 --- 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> } + +impl MethodsPass { + pub fn new() -> MethodsPass { + MethodsPass { types_implementing_debug: None } + } + + fn get_debug_impls(&mut self, cx: &LateContext) -> Option<&HashSet> { + 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_SELF_CONVENTION, Warn, 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 @@ impl LateLintPass for MethodsPass { `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 @@ impl LateLintPass for MethodsPass { } } +// Given a `Result` type, return its error type (`E`) +fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { + 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 314601f6dbd..aeb79503504 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -35,6 +35,8 @@ impl Mul for T { } 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 = 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>= Ok(0); + res3.ok().expect("whoof"); + let res4: Result = Ok(0); + res4.ok().expect("argh"); //~ERROR called `ok().expect()` + let res5: io::Result = Ok(0); + res5.ok().expect("oops"); //~ERROR called `ok().expect()` +} + +struct MyError(()); // doesn't implement Debug + +#[derive(Debug)] +struct MyErrorWithParam { + x: T }