Skip to content

Commit dc6bb6a

Browse files
HMPerson1HMPerson1
committed
Add lint for From<String> 
1 parent 0e489f3 commit dc6bb6a

File tree

4 files changed

+288
-0
lines changed

4 files changed

+288
-0
lines changed

‎clippy_lints/src/impl_from_str.rs

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
use rustc::lint::*;
2+
use rustc::hir;
3+
use rustc::ty;
4+
use syntax_pos::Span;
5+
use utils::{method_chain_args, match_def_path, span_lint_and_then, walk_ptrs_ty};
6+
use utils::paths::{BEGIN_PANIC, BEGIN_PANIC_FMT, FROM_TRAIT, OPTION, RESULT, STRING};
7+
8+
/// **What it does:** Checks for impls of `From<&str>` and `From<String>` that contain `panic!()` or
9+
/// `unwrap()`
10+
///
11+
/// **Why is this bad?** `FromStr` should be used if there's a possibility of failure.
12+
///
13+
/// **Known problems:** None.
14+
///
15+
/// **Example:**
16+
/// ```rust
17+
/// struct Foo(i32);
18+
/// impl From<String> for Foo {
19+
/// fn from(s: String) -> Self {
20+
/// Foo(s.parse().unwrap())
21+
/// }
22+
/// }
23+
/// ```
24+
declare_lint! {
25+
pub IMPL_FROM_STR, Warn,
26+
"Warn on impls of `From<&str>` and `From<String>` that contain `panic!()` or `unwrap()`"
27+
}
28+
29+
pub struct ImplFromStr;
30+
31+
impl LintPass for ImplFromStr {
32+
fn get_lints(&self) -> LintArray {
33+
lint_array!(IMPL_FROM_STR)
34+
}
35+
}
36+
37+
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ImplFromStr {
38+
fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) {
39+
// check for `impl From<???> for ..`
40+
let impl_def_id = cx.tcx.hir.local_def_id(item.id);
41+
if_let_chain!{[
42+
let hir::ItemImpl(.., ref impl_items) = item.node,
43+
let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(impl_def_id),
44+
match_def_path(cx.tcx, impl_trait_ref.def_id, &FROM_TRAIT),
45+
], {
46+
// check if the type parameter is `str` or `String`
47+
let from_ty_param = impl_trait_ref.substs.type_at(1);
48+
let base_from_ty_param =
49+
walk_ptrs_ty(cx.tcx.normalize_associated_type(&from_ty_param));
50+
if base_from_ty_param.sty == ty::TyStr ||
51+
match_type(cx.tcx, base_from_ty_param, &STRING)
52+
{
53+
lint_impl_body(cx, item.span, impl_items);
54+
}
55+
}}
56+
}
57+
}
58+
59+
fn lint_impl_body<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, impl_span: Span, impl_items: &hir::HirVec<hir::ImplItemRef>) {
60+
use rustc::hir::*;
61+
use rustc::hir::intravisit::{self, NestedVisitorMap, Visitor};
62+
63+
struct FindPanicUnwrap<'a, 'tcx: 'a> {
64+
tcx: ty::TyCtxt<'a, 'tcx, 'tcx>,
65+
tables: &'tcx ty::TypeckTables<'tcx>,
66+
result: Vec<Span>,
67+
}
68+
69+
impl<'a, 'tcx: 'a> Visitor<'tcx> for FindPanicUnwrap<'a, 'tcx> {
70+
fn visit_expr(&mut self, expr: &'tcx Expr) {
71+
// check for `begin_panic`
72+
if_let_chain!{[
73+
let ExprCall(ref func_expr, _) = expr.node,
74+
let ExprPath(QPath::Resolved(_, ref path)) = func_expr.node,
75+
match_def_path(self.tcx, path.def.def_id(), &BEGIN_PANIC) ||
76+
match_def_path(self.tcx, path.def.def_id(), &BEGIN_PANIC_FMT),
77+
], {
78+
self.result.push(expr.span);
79+
}}
80+
81+
// check for `unwrap`
82+
if let Some(arglists) = method_chain_args(expr, &["unwrap"]) {
83+
let reciever_ty = walk_ptrs_ty(self.tables.expr_ty(&arglists[0][0]));
84+
if match_type(self.tcx, reciever_ty, &OPTION) ||
85+
match_type(self.tcx, reciever_ty, &RESULT)
86+
{
87+
self.result.push(expr.span);
88+
}
89+
}
90+
91+
// and check sub-expressions
92+
intravisit::walk_expr(self, expr);
93+
}
94+
95+
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
96+
NestedVisitorMap::None
97+
}
98+
}
99+
100+
for impl_item in impl_items {
101+
if_let_chain!{[
102+
impl_item.name == "from",
103+
let ImplItemKind::Method(_, body_id) =
104+
cx.tcx.hir.impl_item(impl_item.id).node,
105+
], {
106+
// check the body for `begin_panic` or `unwrap`
107+
let body = cx.tcx.hir.body(body_id);
108+
let impl_item_def_id = cx.tcx.hir.local_def_id(impl_item.id.node_id);
109+
let mut fpu = FindPanicUnwrap {
110+
tcx: cx.tcx,
111+
tables: cx.tcx.typeck_tables_of(impl_item_def_id),
112+
result: Vec::new(),
113+
};
114+
fpu.visit_expr(&body.value);
115+
116+
// if we've found one, lint
117+
if !fpu.result.is_empty() {
118+
span_lint_and_then(
119+
cx,
120+
IMPL_FROM_STR,
121+
impl_span,
122+
"consider implementing `FromStr` instead",
123+
move |db| {
124+
db.span_note(fpu.result, "potential failure(s)");
125+
});
126+
}
127+
}}
128+
}
129+
}
130+
131+
fn match_type(tcx: ty::TyCtxt, ty: ty::Ty, path: &[&str]) -> bool {
132+
match ty.sty {
133+
ty::TyAdt(adt, _) => match_def_path(tcx, adt.did, path),
134+
_ => false,
135+
}
136+
}

‎clippy_lints/src/lib.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ pub mod identity_conversion;
100100
pub mod identity_op;
101101
pub mod if_let_redundant_pattern_matching;
102102
pub mod if_not_else;
103+
pub mod impl_from_str;
103104
pub mod infinite_iter;
104105
pub mod int_plus_one;
105106
pub mod invalid_ref;
@@ -341,6 +342,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
341342
reg.register_late_lint_pass(box identity_conversion::IdentityConversion::default());
342343
reg.register_late_lint_pass(box types::ImplicitHasher);
343344
reg.register_early_lint_pass(box const_static_lifetime::StaticConst);
345+
reg.register_late_lint_pass(box impl_from_str::ImplFromStr);
344346

345347
reg.register_lint_group("clippy_restrictions", vec![
346348
arithmetic::FLOAT_ARITHMETIC,
@@ -446,6 +448,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
446448
identity_conversion::IDENTITY_CONVERSION,
447449
identity_op::IDENTITY_OP,
448450
if_let_redundant_pattern_matching::IF_LET_REDUNDANT_PATTERN_MATCHING,
451+
impl_from_str::IMPL_FROM_STR,
449452
infinite_iter::INFINITE_ITER,
450453
invalid_ref::INVALID_REF,
451454
is_unit_expr::UNIT_EXPR,

‎tests/ui/impl_from_str.rs

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// docs example
2+
struct Foo(i32);
3+
impl From<String> for Foo {
4+
fn from(s: String) -> Self {
5+
Foo(s.parse().unwrap())
6+
}
7+
}
8+
9+
10+
struct Valid(Vec<u8>);
11+
12+
impl<'a> From<&'a str> for Valid {
13+
fn from(s: &'a str) -> Valid {
14+
Valid(s.to_owned().into_bytes())
15+
}
16+
}
17+
impl From<String> for Valid {
18+
fn from(s: String) -> Valid {
19+
Valid(s.into_bytes())
20+
}
21+
}
22+
impl From<usize> for Valid {
23+
fn from(i: usize) -> Valid {
24+
if i == 0 {
25+
panic!();
26+
}
27+
Valid(Vec::with_capacity(i))
28+
}
29+
}
30+
31+
32+
struct Invalid;
33+
34+
impl<'a> From<&'a str> for Invalid {
35+
fn from(s: &'a str) -> Invalid {
36+
if !s.is_empty() {
37+
panic!();
38+
}
39+
Invalid
40+
}
41+
}
42+
43+
impl From<String> for Invalid {
44+
fn from(s: String) -> Invalid {
45+
if !s.is_empty() {
46+
panic!(42);
47+
} else if s.parse::<u32>().unwrap() != 42 {
48+
panic!("{:?}", s);
49+
}
50+
Invalid
51+
}
52+
}
53+
54+
trait ProjStrTrait {
55+
type ProjString;
56+
}
57+
impl<T> ProjStrTrait for Box<T> {
58+
type ProjString = String;
59+
}
60+
impl<'a> From<&'a mut <Box<u32> as ProjStrTrait>::ProjString> for Invalid {
61+
fn from(s: &'a mut <Box<u32> as ProjStrTrait>::ProjString) -> Invalid {
62+
if s.parse::<u32>().ok().unwrap() != 42 {
63+
panic!("{:?}", s);
64+
}
65+
Invalid
66+
}
67+
}
68+
69+
fn main() {}

‎tests/ui/impl_from_str.stderr

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
error: consider implementing `FromStr` instead
2+
--> $DIR/impl_from_str.rs:3:1
3+
|
4+
3 | / impl From<String> for Foo {
5+
4 | | fn from(s: String) -> Self {
6+
5 | | Foo(s.parse().unwrap())
7+
6 | | }
8+
7 | | }
9+
| |_^
10+
|
11+
= note: `-D impl-from-str` implied by `-D warnings`
12+
note: potential failure(s)
13+
--> $DIR/impl_from_str.rs:5:13
14+
|
15+
5 | Foo(s.parse().unwrap())
16+
| ^^^^^^^^^^^^^^^^^^
17+
18+
error: consider implementing `FromStr` instead
19+
--> $DIR/impl_from_str.rs:34:1
20+
|
21+
34 | / impl<'a> From<&'a str> for Invalid {
22+
35 | | fn from(s: &'a str) -> Invalid {
23+
36 | | if !s.is_empty() {
24+
37 | | panic!();
25+
... |
26+
40 | | }
27+
41 | | }
28+
| |_^
29+
|
30+
note: potential failure(s)
31+
--> $DIR/impl_from_str.rs:37:13
32+
|
33+
37 | panic!();
34+
| ^^^^^^^^^
35+
= note: this error originates in a macro outside of the current crate
36+
37+
error: consider implementing `FromStr` instead
38+
--> $DIR/impl_from_str.rs:43:1
39+
|
40+
43 | / impl From<String> for Invalid {
41+
44 | | fn from(s: String) -> Invalid {
42+
45 | | if !s.is_empty() {
43+
46 | | panic!(42);
44+
... |
45+
51 | | }
46+
52 | | }
47+
| |_^
48+
|
49+
note: potential failure(s)
50+
--> $DIR/impl_from_str.rs:46:13
51+
|
52+
46 | panic!(42);
53+
| ^^^^^^^^^^^
54+
47 | } else if s.parse::<u32>().unwrap() != 42 {
55+
| ^^^^^^^^^^^^^^^^^^^^^^^^^
56+
48 | panic!("{:?}", s);
57+
| ^^^^^^^^^^^^^^^^^^
58+
= note: this error originates in a macro outside of the current crate
59+
60+
error: consider implementing `FromStr` instead
61+
--> $DIR/impl_from_str.rs:60:1
62+
|
63+
60 | / impl<'a> From<&'a mut <Box<u32> as ProjStrTrait>::ProjString> for Invalid {
64+
61 | | fn from(s: &'a mut <Box<u32> as ProjStrTrait>::ProjString) -> Invalid {
65+
62 | | if s.parse::<u32>().ok().unwrap() != 42 {
66+
63 | | panic!("{:?}", s);
67+
... |
68+
66 | | }
69+
67 | | }
70+
| |_^
71+
|
72+
note: potential failure(s)
73+
--> $DIR/impl_from_str.rs:62:12
74+
|
75+
62 | if s.parse::<u32>().ok().unwrap() != 42 {
76+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
77+
63 | panic!("{:?}", s);
78+
| ^^^^^^^^^^^^^^^^^^
79+
= note: this error originates in a macro outside of the current crate
80+

0 commit comments

Comments
 (0)