From cad88a91371a988a0078016c44b1d6f3c24dec57 Mon Sep 17 00:00:00 2001 From: Florian Hartwig Date: Thu, 19 Nov 2015 14:39:27 +0100 Subject: [PATCH 1/4] 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 afbf3d2a92c9..35e303b749b7 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 275de8d8ebdc..dbc18fdfe33c 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 314601f6dbd3..aeb79503504b 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 } From 516f6484607166f062d723544f335d7d5e5c2fb5 Mon Sep 17 00:00:00 2001 From: Florian Hartwig Date: Thu, 19 Nov 2015 14:40:51 +0100 Subject: [PATCH 2/4] Run update_lints --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index b893c4e7a179..64585cfcc029 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 74 lints included in this crate: +There are 75 lints included in this crate: name | default | meaning -------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -50,6 +50,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 From 096c064d4374daf6292cb54f7cf4fac4a8eda718 Mon Sep 17 00:00:00 2001 From: Florian Hartwig Date: Thu, 19 Nov 2015 20:13:36 +0100 Subject: [PATCH 3/4] Simplify has_debug_impl --- src/lib.rs | 2 +- src/methods.rs | 87 +++++++++-------------------------- tests/compile-fail/methods.rs | 4 +- 3 files changed, 27 insertions(+), 66 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 35e303b749b7..d977ed07bfd2 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::new()); + reg.register_late_lint_pass(box methods::MethodsPass); reg.register_late_lint_pass(box shadow::ShadowPass); reg.register_late_lint_pass(box types::LetPass); reg.register_late_lint_pass(box types::UnitCmp); diff --git a/src/methods.rs b/src/methods.rs index dbc18fdfe33c..858a5a3dca08 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -4,7 +4,6 @@ use rustc::middle::ty; 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, walk_ptrs_ty}; @@ -13,69 +12,8 @@ use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH}; use self::SelfKind::*; use self::OutType::*; -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 - } - } -} +pub struct MethodsPass; declare_lint!(pub OPTION_UNWRAP_USED, Allow, "using `Option.unwrap()`, which should at least get a better message using `expect()`"); @@ -144,7 +82,7 @@ impl LateLintPass for MethodsPass { && 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) { + 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 @@ -212,6 +150,27 @@ fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { 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 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, 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]), diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index aeb79503504b..6d543596cf5c 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -54,7 +54,7 @@ fn main() { // 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 + // 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"); @@ -62,6 +62,8 @@ fn main() { 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 From a36707bffd41d5197cf7b081c7594b5663106586 Mon Sep 17 00:00:00 2001 From: Florian Hartwig Date: Thu, 19 Nov 2015 20:19:19 +0100 Subject: [PATCH 4/4] Appease clippy by not shadowing variables --- src/methods.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index 858a5a3dca08..b8c6402544d6 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -154,14 +154,14 @@ fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { // 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 ty = walk_ptrs_ty(ty); + 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, ty, |d| { + 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) {