From e5076d06dbb767a3bd17b4bf70f03c2e5d1221d9 Mon Sep 17 00:00:00 2001 From: HMPerson1 Date: Mon, 16 Oct 2017 17:06:31 -0400 Subject: [PATCH] Add lint for `From` --- clippy_lints/src/impl_from_str.rs | 136 ++++++++++++++++++++++++++++++ clippy_lints/src/lib.rs | 3 + clippy_lints/src/utils/paths.rs | 2 + tests/ui/impl_from_str.rs | 69 +++++++++++++++ tests/ui/impl_from_str.stderr | 80 ++++++++++++++++++ 5 files changed, 290 insertions(+) create mode 100644 clippy_lints/src/impl_from_str.rs create mode 100644 tests/ui/impl_from_str.rs create mode 100644 tests/ui/impl_from_str.stderr diff --git a/clippy_lints/src/impl_from_str.rs b/clippy_lints/src/impl_from_str.rs new file mode 100644 index 00000000000..5d9b3226e11 --- /dev/null +++ b/clippy_lints/src/impl_from_str.rs @@ -0,0 +1,136 @@ +use rustc::lint::*; +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}; + +/// **What it does:** Checks for impls of `From<&str>` and `From` that contain `panic!()` or +/// `unwrap()` +/// +/// **Why is this bad?** `FromStr` should be used if there's a possibility of failure. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// struct Foo(i32); +/// impl From for Foo { +/// fn from(s: String) -> Self { +/// Foo(s.parse().unwrap()) +/// } +/// } +/// ``` +declare_lint! { + pub IMPL_FROM_STR, Warn, + "Warn on impls of `From<&str>` and `From` that contain `panic!()` or `unwrap()`" +} + +pub struct ImplFromStr; + +impl LintPass for ImplFromStr { + fn get_lints(&self) -> LintArray { + lint_array!(IMPL_FROM_STR) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplFromStr { + 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); + if_let_chain!{[ + let hir::ItemImpl(.., ref impl_items) = item.node, + 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); + } + }} + } +} + +fn lint_impl_body<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, impl_span: Span, impl_items: &hir::HirVec) { + use rustc::hir::*; + use rustc::hir::intravisit::{self, NestedVisitorMap, Visitor}; + + struct FindPanicUnwrap<'a, 'tcx: 'a> { + tcx: ty::TyCtxt<'a, 'tcx, 'tcx>, + tables: &'tcx ty::TypeckTables<'tcx>, + result: Vec, + } + + impl<'a, 'tcx: 'a> Visitor<'tcx> for FindPanicUnwrap<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr) { + // check for `begin_panic` + if_let_chain!{[ + let ExprCall(ref func_expr, _) = expr.node, + let ExprPath(QPath::Resolved(_, ref path)) = func_expr.node, + match_def_path(self.tcx, path.def.def_id(), &BEGIN_PANIC) || + match_def_path(self.tcx, path.def.def_id(), &BEGIN_PANIC_FMT), + ], { + self.result.push(expr.span); + }} + + // check for `unwrap` + if let Some(arglists) = method_chain_args(expr, &["unwrap"]) { + let reciever_ty = walk_ptrs_ty(self.tables.expr_ty(&arglists[0][0])); + if match_type(self.tcx, reciever_ty, &OPTION) || + match_type(self.tcx, reciever_ty, &RESULT) + { + self.result.push(expr.span); + } + } + + // and check sub-expressions + intravisit::walk_expr(self, expr); + } + + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } + } + + for impl_item in impl_items { + if_let_chain!{[ + impl_item.name == "from", + let ImplItemKind::Method(_, body_id) = + cx.tcx.hir.impl_item(impl_item.id).node, + ], { + // check the body for `begin_panic` or `unwrap` + let body = cx.tcx.hir.body(body_id); + let impl_item_def_id = cx.tcx.hir.local_def_id(impl_item.id.node_id); + let mut fpu = FindPanicUnwrap { + tcx: cx.tcx, + tables: cx.tcx.typeck_tables_of(impl_item_def_id), + result: Vec::new(), + }; + fpu.visit_expr(&body.value); + + // if we've found one, lint + if !fpu.result.is_empty() { + span_lint_and_then( + cx, + IMPL_FROM_STR, + impl_span, + "consider implementing `FromStr` instead", + move |db| { + db.span_note(fpu.result, "potential failure(s)"); + }); + } + }} + } +} + +fn match_type(tcx: ty::TyCtxt, ty: ty::Ty, path: &[&str]) -> bool { + match ty.sty { + ty::TyAdt(adt, _) => match_def_path(tcx, adt.did, path), + _ => false, + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 110f6b63c82..96bc4fe729c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -100,6 +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 infinite_iter; pub mod int_plus_one; pub mod invalid_ref; @@ -341,6 +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_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -446,6 +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, infinite_iter::INFINITE_ITER, invalid_ref::INVALID_REF, is_unit_expr::UNIT_EXPR, diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index c198ad64b0f..96ccddaf2d0 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -6,6 +6,7 @@ pub const ARC: [&str; 3] = ["alloc", "arc", "Arc"]; pub const ASMUT_TRAIT: [&str; 3] = ["core", "convert", "AsMut"]; pub const ASREF_TRAIT: [&str; 3] = ["core", "convert", "AsRef"]; pub const BEGIN_PANIC: [&str; 3] = ["std", "panicking", "begin_panic"]; +pub const BEGIN_PANIC_FMT: [&str; 3] = ["std", "panicking", "begin_panic_fmt"]; pub const BINARY_HEAP: [&str; 3] = ["alloc", "binary_heap", "BinaryHeap"]; pub const BORROW_TRAIT: [&str; 3] = ["core", "borrow", "Borrow"]; pub const BOX: [&str; 3] = ["std", "boxed", "Box"]; @@ -27,6 +28,7 @@ pub const DROP: [&str; 3] = ["core", "mem", "drop"]; pub const FMT_ARGUMENTS_NEWV1: [&str; 4] = ["core", "fmt", "Arguments", "new_v1"]; pub const FMT_ARGUMENTV1_NEW: [&str; 4] = ["core", "fmt", "ArgumentV1", "new"]; pub const FROM_FROM: [&str; 4] = ["core", "convert", "From", "from"]; +pub const FROM_TRAIT: [&str; 3] = ["core", "convert", "From"]; pub const HASH: [&str; 2] = ["hash", "Hash"]; pub const HASHMAP: [&str; 5] = ["std", "collections", "hash", "map", "HashMap"]; pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entry"]; diff --git a/tests/ui/impl_from_str.rs b/tests/ui/impl_from_str.rs new file mode 100644 index 00000000000..d0ebe5d988a --- /dev/null +++ b/tests/ui/impl_from_str.rs @@ -0,0 +1,69 @@ +// docs example +struct Foo(i32); +impl From for Foo { + fn from(s: String) -> Self { + Foo(s.parse().unwrap()) + } +} + + +struct Valid(Vec); + +impl<'a> From<&'a str> for Valid { + fn from(s: &'a str) -> 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)) + } +} + + +struct Invalid; + +impl<'a> From<&'a str> for Invalid { + fn from(s: &'a str) -> Invalid { + if !s.is_empty() { + panic!(); + } + Invalid + } +} + +impl From for Invalid { + fn from(s: String) -> Invalid { + if !s.is_empty() { + panic!(42); + } else if s.parse::().unwrap() != 42 { + panic!("{:?}", s); + } + Invalid + } +} + +trait ProjStrTrait { + type ProjString; +} +impl ProjStrTrait for Box { + type ProjString = String; +} +impl<'a> From<&'a mut as ProjStrTrait>::ProjString> for Invalid { + fn from(s: &'a mut as ProjStrTrait>::ProjString) -> Invalid { + if s.parse::().ok().unwrap() != 42 { + panic!("{:?}", s); + } + Invalid + } +} + +fn main() {} diff --git a/tests/ui/impl_from_str.stderr b/tests/ui/impl_from_str.stderr new file mode 100644 index 00000000000..b394df153e2 --- /dev/null +++ b/tests/ui/impl_from_str.stderr @@ -0,0 +1,80 @@ +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 +