Skip to content

Commit cb5bffe

Browse files
committed
Add expect lints
1 parent 98f50fb commit cb5bffe

File tree

2 files changed

+94
-2
lines changed

2 files changed

+94
-2
lines changed

clippy_lints/src/methods/mod.rs

Lines changed: 87 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ declare_clippy_lint! {
7272
/// **Known problems:** None.
7373
///
7474
/// **Example:**
75-
/// Using unwrap on an `Option`:
75+
/// Using unwrap on an `Result`:
7676
///
7777
/// ```rust
7878
/// let res: Result<usize, ()> = Ok(1);
@@ -90,6 +90,63 @@ declare_clippy_lint! {
9090
"using `Result.unwrap()`, which might be better handled"
9191
}
9292

93+
declare_clippy_lint! {
94+
/// **What it does:** Checks for `.expect()` calls on `Option`s.
95+
///
96+
/// **Why is this bad?** Usually it is better to handle the `None` case. Still,
97+
/// for a lot of quick-and-dirty code, `expect` is a good choice, which is why
98+
/// this lint is `Allow` by default.
99+
///
100+
/// **Known problems:** None.
101+
///
102+
/// **Example:**
103+
///
104+
/// Using expect on an `Option`:
105+
///
106+
/// ```rust
107+
/// let opt = Some(1);
108+
/// opt.expect("one");
109+
/// ```
110+
///
111+
/// Better:
112+
///
113+
/// ```rust
114+
/// let opt = Some(1);
115+
/// opt?;
116+
/// ```
117+
pub OPTION_EXPECT_USED,
118+
restriction,
119+
"using `Option.unwrap()`, which should at least get a better message using `expect()`"
120+
}
121+
122+
declare_clippy_lint! {
123+
/// **What it does:** Checks for `.expect()` calls on `Result`s.
124+
///
125+
/// **Why is this bad?** `result.expect()` will let the thread panic on `Err`
126+
/// values. Normally, you want to implement more sophisticated error handling,
127+
/// and propagate errors upwards with `try!`.
128+
///
129+
/// **Known problems:** None.
130+
///
131+
/// **Example:**
132+
/// Using expect on an `Result`:
133+
///
134+
/// ```rust
135+
/// let res: Result<usize, ()> = Ok(1);
136+
/// res.expect("one");
137+
/// ```
138+
///
139+
/// Better:
140+
///
141+
/// ```rust
142+
/// let res: Result<usize, ()> = Ok(1);
143+
/// res?;
144+
/// ```
145+
pub RESULT_EXPECT_USED,
146+
restriction,
147+
"using `Result.expect()`, which might be better handled"
148+
}
149+
93150
declare_clippy_lint! {
94151
/// **What it does:** Checks for methods that should live in a trait
95152
/// implementation of a `std` trait (see [llogiq's blog
@@ -1013,6 +1070,8 @@ declare_clippy_lint! {
10131070
declare_lint_pass!(Methods => [
10141071
OPTION_UNWRAP_USED,
10151072
RESULT_UNWRAP_USED,
1073+
OPTION_EXPECT_USED,
1074+
RESULT_EXPECT_USED,
10161075
SHOULD_IMPLEMENT_TRAIT,
10171076
WRONG_SELF_CONVENTION,
10181077
WRONG_PUB_SELF_CONVENTION,
@@ -1070,6 +1129,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
10701129
["unwrap", "get_mut"] => lint_get_unwrap(cx, expr, arg_lists[1], true),
10711130
["unwrap", ..] => lint_unwrap(cx, expr, arg_lists[0]),
10721131
["expect", "ok"] => lint_ok_expect(cx, expr, arg_lists[1]),
1132+
["expect", ..] => lint_expect(cx, expr, arg_lists[0]),
10731133
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0]),
10741134
["unwrap_or_else", "map"] => lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]),
10751135
["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]),
@@ -2035,6 +2095,31 @@ fn lint_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, unwrap_args: &[hir::E
20352095
}
20362096
}
20372097

2098+
/// lint use of `expect()` for `Option`s and `Result`s
2099+
fn lint_expect(cx: &LateContext<'_, '_>, expr: &hir::Expr, expect_args: &[hir::Expr]) {
2100+
let obj_ty = walk_ptrs_ty(cx.tables.expr_ty(&expect_args[0]));
2101+
2102+
let mess = if match_type(cx, obj_ty, &paths::OPTION) {
2103+
Some((OPTION_EXPECT_USED, "an Option", "None"))
2104+
} else if match_type(cx, obj_ty, &paths::RESULT) {
2105+
Some((RESULT_EXPECT_USED, "a Result", "Err"))
2106+
} else {
2107+
None
2108+
};
2109+
2110+
if let Some((lint, kind, none_value)) = mess {
2111+
span_lint(
2112+
cx,
2113+
lint,
2114+
expr.span,
2115+
&format!(
2116+
"used expect() on {} value. If this value is an {} it will panic",
2117+
kind, none_value
2118+
),
2119+
);
2120+
}
2121+
}
2122+
20382123
/// lint use of `ok().expect()` for `Result`s
20392124
fn lint_ok_expect(cx: &LateContext<'_, '_>, expr: &hir::Expr, ok_args: &[hir::Expr]) {
20402125
if_chain! {
@@ -2044,6 +2129,7 @@ fn lint_ok_expect(cx: &LateContext<'_, '_>, expr: &hir::Expr, ok_args: &[hir::Ex
20442129
if let Some(error_type) = get_error_type(cx, result_type);
20452130
if has_debug_impl(error_type, cx);
20462131

2132+
20472133
then {
20482134
span_lint(
20492135
cx,

tests/ui/methods.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
11
// aux-build:option_helpers.rs
22
// compile-flags: --edition 2018
33

4-
#![warn(clippy::all, clippy::pedantic, clippy::option_unwrap_used)]
4+
#![warn(
5+
clippy::all,
6+
clippy::pedantic,
7+
clippy::option_unwrap_used,
8+
clippy::option_expect_used,
9+
clippy::result_expect_used
10+
)]
511
#![allow(
612
clippy::blacklisted_name,
713
clippy::default_trait_access,

0 commit comments

Comments
 (0)