Skip to content

Commit f82e84c

Browse files
borsbors
committed
Auto merge of #5998 - deg4uss3r:master, r=yaahc
Add map_err_ignore lint In a large code base a lot of times errors are ignored by using something like: ```rust foo.map_err(|_| Some::Enum)?; ``` This drops the original error in favor of a enum that will not have the original error's context. This lint helps catch throwing away the original error in favor of an enum without its context. --- *Please keep the line below* changelog: Added map_err_ignore lint
2 parents 231444d + d719b48 commit f82e84c

File tree

8 files changed

+214
-18
lines changed

8 files changed

+214
-18
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1676,6 +1676,7 @@ Released 2018-09-13
16761676
[`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names
16771677
[`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone
16781678
[`map_entry`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_entry
1679+
[`map_err_ignore`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_err_ignore
16791680
[`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten
16801681
[`map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_identity
16811682
[`map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or

‎clippy_lints/src/lib.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,7 @@ mod main_recursion;
231231
mod manual_async_fn;
232232
mod manual_non_exhaustive;
233233
mod map_clone;
234+
mod map_err_ignore;
234235
mod map_identity;
235236
mod map_unit_fn;
236237
mod match_on_vec_items;
@@ -627,6 +628,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
627628
&manual_async_fn::MANUAL_ASYNC_FN,
628629
&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
629630
&map_clone::MAP_CLONE,
631+
&map_err_ignore::MAP_ERR_IGNORE,
630632
&map_identity::MAP_IDENTITY,
631633
&map_unit_fn::OPTION_MAP_UNIT_FN,
632634
&map_unit_fn::RESULT_MAP_UNIT_FN,
@@ -920,6 +922,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
920922
store.register_late_pass(|| box implicit_saturating_sub::ImplicitSaturatingSub);
921923
store.register_late_pass(|| box methods::Methods);
922924
store.register_late_pass(|| box map_clone::MapClone);
925+
store.register_late_pass(|| box map_err_ignore::MapErrIgnore);
923926
store.register_late_pass(|| box shadow::Shadow);
924927
store.register_late_pass(|| box types::LetUnitValue);
925928
store.register_late_pass(|| box types::UnitCmp);
@@ -1186,6 +1189,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
11861189
LintId::of(&loops::EXPLICIT_INTO_ITER_LOOP),
11871190
LintId::of(&loops::EXPLICIT_ITER_LOOP),
11881191
LintId::of(&macro_use::MACRO_USE_IMPORTS),
1192+
LintId::of(&map_err_ignore::MAP_ERR_IGNORE),
11891193
LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS),
11901194
LintId::of(&matches::MATCH_BOOL),
11911195
LintId::of(&matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS),

‎clippy_lints/src/map_err_ignore.rs

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
use crate::utils::span_lint_and_help;
2+
3+
use rustc_hir::{CaptureBy, Expr, ExprKind, PatKind};
4+
use rustc_lint::{LateContext, LateLintPass};
5+
use rustc_session::{declare_lint_pass, declare_tool_lint};
6+
7+
declare_clippy_lint! {
8+
/// **What it does:** Checks for instances of `map_err(|_| Some::Enum)`
9+
///
10+
/// **Why is this bad?** This map_err throws away the original error rather than allowing the enum to contain and report the cause of the error
11+
///
12+
/// **Known problems:** None.
13+
///
14+
/// **Example:**
15+
/// Before:
16+
/// ```rust
17+
/// use std::fmt;
18+
///
19+
/// #[derive(Debug)]
20+
/// enum Error {
21+
/// Indivisible,
22+
/// Remainder(u8),
23+
/// }
24+
///
25+
/// impl fmt::Display for Error {
26+
/// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
27+
/// match self {
28+
/// Error::Indivisible => write!(f, "could not divide input by three"),
29+
/// Error::Remainder(remainder) => write!(
30+
/// f,
31+
/// "input is not divisible by three, remainder = {}",
32+
/// remainder
33+
/// ),
34+
/// }
35+
/// }
36+
/// }
37+
///
38+
/// impl std::error::Error for Error {}
39+
///
40+
/// fn divisible_by_3(input: &str) -> Result<(), Error> {
41+
/// input
42+
/// .parse::<i32>()
43+
/// .map_err(|_| Error::Indivisible)
44+
/// .map(|v| v % 3)
45+
/// .and_then(|remainder| {
46+
/// if remainder == 0 {
47+
/// Ok(())
48+
/// } else {
49+
/// Err(Error::Remainder(remainder as u8))
50+
/// }
51+
/// })
52+
/// }
53+
/// ```
54+
///
55+
/// After:
56+
/// ```rust
57+
/// use std::{fmt, num::ParseIntError};
58+
///
59+
/// #[derive(Debug)]
60+
/// enum Error {
61+
/// Indivisible(ParseIntError),
62+
/// Remainder(u8),
63+
/// }
64+
///
65+
/// impl fmt::Display for Error {
66+
/// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
67+
/// match self {
68+
/// Error::Indivisible(_) => write!(f, "could not divide input by three"),
69+
/// Error::Remainder(remainder) => write!(
70+
/// f,
71+
/// "input is not divisible by three, remainder = {}",
72+
/// remainder
73+
/// ),
74+
/// }
75+
/// }
76+
/// }
77+
///
78+
/// impl std::error::Error for Error {
79+
/// fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
80+
/// match self {
81+
/// Error::Indivisible(source) => Some(source),
82+
/// _ => None,
83+
/// }
84+
/// }
85+
/// }
86+
///
87+
/// fn divisible_by_3(input: &str) -> Result<(), Error> {
88+
/// input
89+
/// .parse::<i32>()
90+
/// .map_err(Error::Indivisible)
91+
/// .map(|v| v % 3)
92+
/// .and_then(|remainder| {
93+
/// if remainder == 0 {
94+
/// Ok(())
95+
/// } else {
96+
/// Err(Error::Remainder(remainder as u8))
97+
/// }
98+
/// })
99+
/// }
100+
/// ```
101+
pub MAP_ERR_IGNORE,
102+
pedantic,
103+
"`map_err` should not ignore the original error"
104+
}
105+
106+
declare_lint_pass!(MapErrIgnore => [MAP_ERR_IGNORE]);
107+
108+
impl<'tcx> LateLintPass<'tcx> for MapErrIgnore {
109+
// do not try to lint if this is from a macro or desugaring
110+
fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) {
111+
if e.span.from_expansion() {
112+
return;
113+
}
114+
115+
// check if this is a method call (e.g. x.foo())
116+
if let ExprKind::MethodCall(ref method, _t_span, ref args, _) = e.kind {
117+
// only work if the method name is `map_err` and there are only 2 arguments (e.g. x.map_err(|_|[1]
118+
// Enum::Variant[2]))
119+
if method.ident.as_str() == "map_err" && args.len() == 2 {
120+
// make sure the first argument is a closure, and grab the CaptureRef, body_id, and body_span fields
121+
if let ExprKind::Closure(capture, _, body_id, body_span, _) = args[1].kind {
122+
// check if this is by Reference (meaning there's no move statement)
123+
if capture == CaptureBy::Ref {
124+
// Get the closure body to check the parameters and values
125+
let closure_body = cx.tcx.hir().body(body_id);
126+
// make sure there's only one parameter (`|_|`)
127+
if closure_body.params.len() == 1 {
128+
// make sure that parameter is the wild token (`_`)
129+
if let PatKind::Wild = closure_body.params[0].pat.kind {
130+
// span the area of the closure capture and warn that the
131+
// original error will be thrown away
132+
span_lint_and_help(
133+
cx,
134+
MAP_ERR_IGNORE,
135+
body_span,
136+
"`map_err(|_|...` ignores the original error",
137+
None,
138+
"Consider wrapping the error in an enum variant",
139+
);
140+
}
141+
}
142+
}
143+
}
144+
}
145+
}
146+
}
147+
}

‎src/lintlist/mod.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1172,6 +1172,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
11721172
deprecation: None,
11731173
module: "entry",
11741174
},
1175+
Lint {
1176+
name: "map_err_ignore",
1177+
group: "pedantic",
1178+
desc: "`map_err` should not ignore the original error",
1179+
deprecation: None,
1180+
module: "map_err_ignore",
1181+
},
11751182
Lint {
11761183
name: "map_flatten",
11771184
group: "pedantic",

‎tests/ui/drop_ref.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#![warn(clippy::drop_ref)]
22
#![allow(clippy::toplevel_ref_arg)]
3+
#![allow(clippy::map_err_ignore)]
34

45
use std::mem::drop;
56

‎tests/ui/drop_ref.stderr

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,108 +1,108 @@
11
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
2-
--> $DIR/drop_ref.rs:9:5
2+
--> $DIR/drop_ref.rs:10:5
33
|
44
LL | drop(&SomeStruct);
55
| ^^^^^^^^^^^^^^^^^
66
|
77
= note: `-D clippy::drop-ref` implied by `-D warnings`
88
note: argument has type `&SomeStruct`
9-
--> $DIR/drop_ref.rs:9:10
9+
--> $DIR/drop_ref.rs:10:10
1010
|
1111
LL | drop(&SomeStruct);
1212
| ^^^^^^^^^^^
1313

1414
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
15-
--> $DIR/drop_ref.rs:12:5
15+
--> $DIR/drop_ref.rs:13:5
1616
|
1717
LL | drop(&owned1);
1818
| ^^^^^^^^^^^^^
1919
|
2020
note: argument has type `&SomeStruct`
21-
--> $DIR/drop_ref.rs:12:10
21+
--> $DIR/drop_ref.rs:13:10
2222
|
2323
LL | drop(&owned1);
2424
| ^^^^^^^
2525

2626
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
27-
--> $DIR/drop_ref.rs:13:5
27+
--> $DIR/drop_ref.rs:14:5
2828
|
2929
LL | drop(&&owned1);
3030
| ^^^^^^^^^^^^^^
3131
|
3232
note: argument has type `&&SomeStruct`
33-
--> $DIR/drop_ref.rs:13:10
33+
--> $DIR/drop_ref.rs:14:10
3434
|
3535
LL | drop(&&owned1);
3636
| ^^^^^^^^
3737

3838
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
39-
--> $DIR/drop_ref.rs:14:5
39+
--> $DIR/drop_ref.rs:15:5
4040
|
4141
LL | drop(&mut owned1);
4242
| ^^^^^^^^^^^^^^^^^
4343
|
4444
note: argument has type `&mut SomeStruct`
45-
--> $DIR/drop_ref.rs:14:10
45+
--> $DIR/drop_ref.rs:15:10
4646
|
4747
LL | drop(&mut owned1);
4848
| ^^^^^^^^^^^
4949

5050
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
51-
--> $DIR/drop_ref.rs:18:5
51+
--> $DIR/drop_ref.rs:19:5
5252
|
5353
LL | drop(reference1);
5454
| ^^^^^^^^^^^^^^^^
5555
|
5656
note: argument has type `&SomeStruct`
57-
--> $DIR/drop_ref.rs:18:10
57+
--> $DIR/drop_ref.rs:19:10
5858
|
5959
LL | drop(reference1);
6060
| ^^^^^^^^^^
6161

6262
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
63-
--> $DIR/drop_ref.rs:21:5
63+
--> $DIR/drop_ref.rs:22:5
6464
|
6565
LL | drop(reference2);
6666
| ^^^^^^^^^^^^^^^^
6767
|
6868
note: argument has type `&mut SomeStruct`
69-
--> $DIR/drop_ref.rs:21:10
69+
--> $DIR/drop_ref.rs:22:10
7070
|
7171
LL | drop(reference2);
7272
| ^^^^^^^^^^
7373

7474
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
75-
--> $DIR/drop_ref.rs:24:5
75+
--> $DIR/drop_ref.rs:25:5
7676
|
7777
LL | drop(reference3);
7878
| ^^^^^^^^^^^^^^^^
7979
|
8080
note: argument has type `&SomeStruct`
81-
--> $DIR/drop_ref.rs:24:10
81+
--> $DIR/drop_ref.rs:25:10
8282
|
8383
LL | drop(reference3);
8484
| ^^^^^^^^^^
8585

8686
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
87-
--> $DIR/drop_ref.rs:29:5
87+
--> $DIR/drop_ref.rs:30:5
8888
|
8989
LL | drop(&val);
9090
| ^^^^^^^^^^
9191
|
9292
note: argument has type `&T`
93-
--> $DIR/drop_ref.rs:29:10
93+
--> $DIR/drop_ref.rs:30:10
9494
|
9595
LL | drop(&val);
9696
| ^^^^
9797

9898
error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
99-
--> $DIR/drop_ref.rs:37:5
99+
--> $DIR/drop_ref.rs:38:5
100100
|
101101
LL | std::mem::drop(&SomeStruct);
102102
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
103103
|
104104
note: argument has type `&SomeStruct`
105-
--> $DIR/drop_ref.rs:37:20
105+
--> $DIR/drop_ref.rs:38:20
106106
|
107107
LL | std::mem::drop(&SomeStruct);
108108
| ^^^^^^^^^^^

‎tests/ui/map_err.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
#![warn(clippy::map_err_ignore)]
2+
use std::convert::TryFrom;
3+
use std::error::Error;
4+
use std::fmt;
5+
6+
#[derive(Debug)]
7+
enum Errors {
8+
Ignored,
9+
}
10+
11+
impl Error for Errors {}
12+
13+
impl fmt::Display for Errors {
14+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
15+
write!(f, "Error")
16+
}
17+
}
18+
19+
fn main() -> Result<(), Errors> {
20+
let x = u32::try_from(-123_i32);
21+
22+
println!("{:?}", x.map_err(|_| Errors::Ignored));
23+
24+
Ok(())
25+
}

‎tests/ui/map_err.stderr

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
error: `map_err(|_|...` ignores the original error
2+
--> $DIR/map_err.rs:22:32
3+
|
4+
LL | println!("{:?}", x.map_err(|_| Errors::Ignored));
5+
| ^^^
6+
|
7+
= note: `-D clippy::map-err-ignore` implied by `-D warnings`
8+
= help: Consider wrapping the error in an enum variant
9+
10+
error: aborting due to previous error
11+

0 commit comments

Comments
 (0)