diff --git a/clippy_lints/src/impl_from_str.rs b/clippy_lints/src/fallible_impl_from.rs similarity index 78% rename from clippy_lints/src/impl_from_str.rs rename to clippy_lints/src/fallible_impl_from.rs index 5d9b3226e11..bdcda99124c 100644 --- a/clippy_lints/src/impl_from_str.rs +++ b/clippy_lints/src/fallible_impl_from.rs @@ -3,12 +3,11 @@ use rustc::hir; use rustc::ty; use syntax_pos::Span; use utils::{method_chain_args, match_def_path, span_lint_and_then, walk_ptrs_ty}; -use utils::paths::{BEGIN_PANIC, BEGIN_PANIC_FMT, FROM_TRAIT, OPTION, RESULT, STRING}; +use utils::paths::{BEGIN_PANIC, BEGIN_PANIC_FMT, FROM_TRAIT, OPTION, RESULT}; -/// **What it does:** Checks for impls of `From<&str>` and `From` that contain `panic!()` or -/// `unwrap()` +/// **What it does:** Checks for impls of `From<..>` that contain `panic!()` or `unwrap()` /// -/// **Why is this bad?** `FromStr` should be used if there's a possibility of failure. +/// **Why is this bad?** `TryFrom` should be used if there's a possibility of failure. /// /// **Known problems:** None. /// @@ -22,19 +21,19 @@ use utils::paths::{BEGIN_PANIC, BEGIN_PANIC_FMT, FROM_TRAIT, OPTION, RESULT, STR /// } /// ``` declare_lint! { - pub IMPL_FROM_STR, Warn, - "Warn on impls of `From<&str>` and `From` that contain `panic!()` or `unwrap()`" + pub FALLIBLE_IMPL_FROM, Allow, + "Warn on impls of `From<..>` that contain `panic!()` or `unwrap()`" } -pub struct ImplFromStr; +pub struct FallibleImplFrom; -impl LintPass for ImplFromStr { +impl LintPass for FallibleImplFrom { fn get_lints(&self) -> LintArray { - lint_array!(IMPL_FROM_STR) + lint_array!(FALLIBLE_IMPL_FROM) } } -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplFromStr { +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for FallibleImplFrom { fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) { // check for `impl From for ..` let impl_def_id = cx.tcx.hir.local_def_id(item.id); @@ -43,15 +42,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplFromStr { let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(impl_def_id), match_def_path(cx.tcx, impl_trait_ref.def_id, &FROM_TRAIT), ], { - // check if the type parameter is `str` or `String` - let from_ty_param = impl_trait_ref.substs.type_at(1); - let base_from_ty_param = - walk_ptrs_ty(cx.tcx.normalize_associated_type(&from_ty_param)); - if base_from_ty_param.sty == ty::TyStr || - match_type(cx.tcx, base_from_ty_param, &STRING) - { - lint_impl_body(cx, item.span, impl_items); - } + lint_impl_body(cx, item.span, impl_items); }} } } @@ -117,10 +108,13 @@ fn lint_impl_body<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, impl_span: Span, impl_it if !fpu.result.is_empty() { span_lint_and_then( cx, - IMPL_FROM_STR, + FALLIBLE_IMPL_FROM, impl_span, - "consider implementing `FromStr` instead", + "consider implementing `TryFrom` instead", move |db| { + db.help( + "`From` is intended for infallible conversions only. \ + Use `TryFrom` if there's a possibility for the conversion to fail."); db.span_note(fpu.result, "potential failure(s)"); }); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 96bc4fe729c..81839b92cd5 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -100,7 +100,7 @@ pub mod identity_conversion; pub mod identity_op; pub mod if_let_redundant_pattern_matching; pub mod if_not_else; -pub mod impl_from_str; +pub mod fallible_impl_from; pub mod infinite_iter; pub mod int_plus_one; pub mod invalid_ref; @@ -342,7 +342,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box identity_conversion::IdentityConversion::default()); reg.register_late_lint_pass(box types::ImplicitHasher); reg.register_early_lint_pass(box const_static_lifetime::StaticConst); - reg.register_late_lint_pass(box impl_from_str::ImplFromStr); + reg.register_late_lint_pass(box fallible_impl_from::FallibleImplFrom); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -448,7 +448,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { identity_conversion::IDENTITY_CONVERSION, identity_op::IDENTITY_OP, if_let_redundant_pattern_matching::IF_LET_REDUNDANT_PATTERN_MATCHING, - impl_from_str::IMPL_FROM_STR, + fallible_impl_from::FALLIBLE_IMPL_FROM, infinite_iter::INFINITE_ITER, invalid_ref::INVALID_REF, is_unit_expr::UNIT_EXPR, diff --git a/tests/ui/impl_from_str.rs b/tests/ui/fallible_impl_from.rs similarity index 75% rename from tests/ui/impl_from_str.rs rename to tests/ui/fallible_impl_from.rs index d0ebe5d988a..eb1cd4c5e9a 100644 --- a/tests/ui/impl_from_str.rs +++ b/tests/ui/fallible_impl_from.rs @@ -1,3 +1,5 @@ +#![deny(fallible_impl_from)] + // docs example struct Foo(i32); impl From for Foo { @@ -14,16 +16,8 @@ impl<'a> From<&'a str> for Valid { Valid(s.to_owned().into_bytes()) } } -impl From for Valid { - fn from(s: String) -> Valid { - Valid(s.into_bytes()) - } -} impl From for Valid { fn from(i: usize) -> Valid { - if i == 0 { - panic!(); - } Valid(Vec::with_capacity(i)) } } @@ -31,17 +25,18 @@ impl From for Valid { struct Invalid; -impl<'a> From<&'a str> for Invalid { - fn from(s: &'a str) -> Invalid { - if !s.is_empty() { +impl From for Invalid { + fn from(i: usize) -> Invalid { + if i != 42 { panic!(); } Invalid } } -impl From for Invalid { - fn from(s: String) -> Invalid { +impl From> for Invalid { + fn from(s: Option) -> Invalid { + let s = s.unwrap(); if !s.is_empty() { panic!(42); } else if s.parse::().unwrap() != 42 { diff --git a/tests/ui/fallible_impl_from.stderr b/tests/ui/fallible_impl_from.stderr new file mode 100644 index 00000000000..89dfaf623ed --- /dev/null +++ b/tests/ui/fallible_impl_from.stderr @@ -0,0 +1,91 @@ +error: consider implementing `TryFrom` instead + --> $DIR/fallible_impl_from.rs:5:1 + | +5 | / impl From for Foo { +6 | | fn from(s: String) -> Self { +7 | | Foo(s.parse().unwrap()) +8 | | } +9 | | } + | |_^ + | +note: lint level defined here + --> $DIR/fallible_impl_from.rs:1:9 + | +1 | #![deny(fallible_impl_from)] + | ^^^^^^^^^^^^^^^^^^ + = help: `From` is intended for infallible conversions only. Use `TryFrom` if there's a possibility for the conversion to fail. +note: potential failure(s) + --> $DIR/fallible_impl_from.rs:7:13 + | +7 | Foo(s.parse().unwrap()) + | ^^^^^^^^^^^^^^^^^^ + +error: consider implementing `TryFrom` instead + --> $DIR/fallible_impl_from.rs:28:1 + | +28 | / impl From for Invalid { +29 | | fn from(i: usize) -> Invalid { +30 | | if i != 42 { +31 | | panic!(); +... | +34 | | } +35 | | } + | |_^ + | + = help: `From` is intended for infallible conversions only. Use `TryFrom` if there's a possibility for the conversion to fail. +note: potential failure(s) + --> $DIR/fallible_impl_from.rs:31:13 + | +31 | panic!(); + | ^^^^^^^^^ + = note: this error originates in a macro outside of the current crate + +error: consider implementing `TryFrom` instead + --> $DIR/fallible_impl_from.rs:37:1 + | +37 | / impl From> for Invalid { +38 | | fn from(s: Option) -> Invalid { +39 | | let s = s.unwrap(); +40 | | if !s.is_empty() { +... | +46 | | } +47 | | } + | |_^ + | + = help: `From` is intended for infallible conversions only. Use `TryFrom` if there's a possibility for the conversion to fail. +note: potential failure(s) + --> $DIR/fallible_impl_from.rs:39:17 + | +39 | let s = s.unwrap(); + | ^^^^^^^^^^ +40 | if !s.is_empty() { +41 | panic!(42); + | ^^^^^^^^^^^ +42 | } else if s.parse::().unwrap() != 42 { + | ^^^^^^^^^^^^^^^^^^^^^^^^^ +43 | panic!("{:?}", s); + | ^^^^^^^^^^^^^^^^^^ + = note: this error originates in a macro outside of the current crate + +error: consider implementing `TryFrom` instead + --> $DIR/fallible_impl_from.rs:55:1 + | +55 | / impl<'a> From<&'a mut as ProjStrTrait>::ProjString> for Invalid { +56 | | fn from(s: &'a mut as ProjStrTrait>::ProjString) -> Invalid { +57 | | if s.parse::().ok().unwrap() != 42 { +58 | | panic!("{:?}", s); +... | +61 | | } +62 | | } + | |_^ + | + = help: `From` is intended for infallible conversions only. Use `TryFrom` if there's a possibility for the conversion to fail. +note: potential failure(s) + --> $DIR/fallible_impl_from.rs:57:12 + | +57 | if s.parse::().ok().unwrap() != 42 { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +58 | panic!("{:?}", s); + | ^^^^^^^^^^^^^^^^^^ + = note: this error originates in a macro outside of the current crate + diff --git a/tests/ui/impl_from_str.stderr b/tests/ui/impl_from_str.stderr deleted file mode 100644 index b394df153e2..00000000000 --- a/tests/ui/impl_from_str.stderr +++ /dev/null @@ -1,80 +0,0 @@ -error: consider implementing `FromStr` instead - --> $DIR/impl_from_str.rs:3:1 - | -3 | / impl From for Foo { -4 | | fn from(s: String) -> Self { -5 | | Foo(s.parse().unwrap()) -6 | | } -7 | | } - | |_^ - | - = note: `-D impl-from-str` implied by `-D warnings` -note: potential failure(s) - --> $DIR/impl_from_str.rs:5:13 - | -5 | Foo(s.parse().unwrap()) - | ^^^^^^^^^^^^^^^^^^ - -error: consider implementing `FromStr` instead - --> $DIR/impl_from_str.rs:34:1 - | -34 | / impl<'a> From<&'a str> for Invalid { -35 | | fn from(s: &'a str) -> Invalid { -36 | | if !s.is_empty() { -37 | | panic!(); -... | -40 | | } -41 | | } - | |_^ - | -note: potential failure(s) - --> $DIR/impl_from_str.rs:37:13 - | -37 | panic!(); - | ^^^^^^^^^ - = note: this error originates in a macro outside of the current crate - -error: consider implementing `FromStr` instead - --> $DIR/impl_from_str.rs:43:1 - | -43 | / impl From for Invalid { -44 | | fn from(s: String) -> Invalid { -45 | | if !s.is_empty() { -46 | | panic!(42); -... | -51 | | } -52 | | } - | |_^ - | -note: potential failure(s) - --> $DIR/impl_from_str.rs:46:13 - | -46 | panic!(42); - | ^^^^^^^^^^^ -47 | } else if s.parse::().unwrap() != 42 { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -48 | panic!("{:?}", s); - | ^^^^^^^^^^^^^^^^^^ - = note: this error originates in a macro outside of the current crate - -error: consider implementing `FromStr` instead - --> $DIR/impl_from_str.rs:60:1 - | -60 | / impl<'a> From<&'a mut as ProjStrTrait>::ProjString> for Invalid { -61 | | fn from(s: &'a mut as ProjStrTrait>::ProjString) -> Invalid { -62 | | if s.parse::().ok().unwrap() != 42 { -63 | | panic!("{:?}", s); -... | -66 | | } -67 | | } - | |_^ - | -note: potential failure(s) - --> $DIR/impl_from_str.rs:62:12 - | -62 | if s.parse::().ok().unwrap() != 42 { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -63 | panic!("{:?}", s); - | ^^^^^^^^^^^^^^^^^^ - = note: this error originates in a macro outside of the current crate -