diff --git a/README.md b/README.md index 696836f417d2..ae19f6ff9d7a 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 76 lints included in this crate: +There are 77 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -52,6 +52,7 @@ name [no_effect](https://github.com/Manishearth/rust-clippy/wiki#no_effect) | warn | statements with no effect [non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal) | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead [nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options) | warn | nonsensical combination of options for opening a file +[ok_expect](https://github.com/Manishearth/rust-clippy/wiki#ok_expect) | warn | using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result [option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used) | allow | using `Option.unwrap()`, which should at least get a better message using `expect()` [precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence) | warn | catches operations where precedence may be unclear. See the wiki for a list of cases caught [ptr_arg](https://github.com/Manishearth/rust-clippy/wiki#ptr_arg) | warn | fn arguments of the type `&Vec<...>` or `&String`, suggesting to use `&[...]` or `&str` instead, respectively diff --git a/src/lib.rs b/src/lib.rs index 9507b1d86f50..3664803d0223 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -155,6 +155,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 275de8d8ebdc..b8c6402544d6 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -1,17 +1,18 @@ 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 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)] +#[derive(Clone)] pub struct MethodsPass; declare_lint!(pub OPTION_UNWRAP_USED, Allow, @@ -30,16 +31,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 +76,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 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 +137,41 @@ 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 +} + +// 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<'a, 'b>(ty: ty::Ty<'a>, cx: &LateContext<'b, 'a>) -> bool { + let no_ref_ty = walk_ptrs_ty(ty); + let debug = match cx.tcx.lang_items.debug_trait() { + Some(debug) => debug, + None => return false + }; + let debug_def = cx.tcx.lookup_trait_def(debug); + let mut debug_impl_exists = false; + debug_def.for_each_relevant_impl(cx.tcx, no_ref_ty, |d| { + let self_ty = &cx.tcx.impl_trait_ref(d).and_then(|im| im.substs.self_ty()); + if let Some(self_ty) = *self_ty { + if !self_ty.flags.get().contains(ty::TypeFlags::HAS_PARAMS) { + debug_impl_exists = true; + } + } + }); + debug_impl_exists +} + 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 314601f6dbd3..6d543596cf5c 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,27 @@ 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 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()` + let res6: Result = Ok(0); + res6.ok().expect("meh"); //~ERROR called `ok().expect()` +} + +struct MyError(()); // doesn't implement Debug + +#[derive(Debug)] +struct MyErrorWithParam { + x: T }