Skip to content

Commit

Permalink
Add expect
Browse files Browse the repository at this point in the history
Co-Authored-By: Philipp Krones <hello@philkrones.com>
  • Loading branch information
Licenser and flip1995 committed Oct 17, 2019
1 parent d895183 commit 57dcc0d
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 3 deletions.
87 changes: 86 additions & 1 deletion clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ declare_clippy_lint! {
/// **Known problems:** None.
///
/// **Example:**
/// Using unwrap on an `Option`:
/// Using unwrap on an `Result`:
///
/// ```rust
/// let res: Result<usize, ()> = Ok(1);
Expand All @@ -90,6 +90,63 @@ declare_clippy_lint! {
"using `Result.unwrap()`, which might be better handled"
}

declare_clippy_lint! {
/// **What it does:** Checks for `.expect()` calls on `Option`s.
///
/// **Why is this bad?** Usually it is better to handle the `None` case. Still,
/// for a lot of quick-and-dirty code, `expect` is a good choice, which is why
/// this lint is `Allow` by default.
///
/// **Known problems:** None.
///
/// **Example:**
///
/// Using expect on an `Option`:
///
/// ```rust
/// let opt = Some(1);
/// opt.expect("one");
/// ```
///
/// Better:
///
/// ```rust
/// let opt = Some(1);
/// opt?;
/// ```
pub OPTION_EXPECT_USED,
restriction,
"using `Option.expect()`, which might be better handled"
}

declare_clippy_lint! {
/// **What it does:** Checks for `.expect()` calls on `Result`s.
///
/// **Why is this bad?** `result.expect()` will let the thread panic on `Err`
/// values. Normally, you want to implement more sophisticated error handling,
/// and propagate errors upwards with `try!`.
///
/// **Known problems:** None.
///
/// **Example:**
/// Using expect on an `Result`:
///
/// ```rust
/// let res: Result<usize, ()> = Ok(1);
/// res.expect("one");
/// ```
///
/// Better:
///
/// ```rust
/// let res: Result<usize, ()> = Ok(1);
/// res?;
/// ```
pub RESULT_EXPECT_USED,
restriction,
"using `Result.expect()`, which might be better handled"
}

declare_clippy_lint! {
/// **What it does:** Checks for methods that should live in a trait
/// implementation of a `std` trait (see [llogiq's blog
Expand Down Expand Up @@ -1013,6 +1070,8 @@ declare_clippy_lint! {
declare_lint_pass!(Methods => [
OPTION_UNWRAP_USED,
RESULT_UNWRAP_USED,
OPTION_EXPECT_USED,
RESULT_EXPECT_USED,
SHOULD_IMPLEMENT_TRAIT,
WRONG_SELF_CONVENTION,
WRONG_PUB_SELF_CONVENTION,
Expand Down Expand Up @@ -1070,6 +1129,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
["unwrap", "get_mut"] => lint_get_unwrap(cx, expr, arg_lists[1], true),
["unwrap", ..] => lint_unwrap(cx, expr, arg_lists[0]),
["expect", "ok"] => lint_ok_expect(cx, expr, arg_lists[1]),
["expect", ..] => lint_expect(cx, expr, arg_lists[0]),
["unwrap_or", "map"] => option_map_unwrap_or::lint(cx, expr, arg_lists[1], arg_lists[0]),
["unwrap_or_else", "map"] => lint_map_unwrap_or_else(cx, expr, arg_lists[1], arg_lists[0]),
["map_or", ..] => lint_map_or_none(cx, expr, arg_lists[0]),
Expand Down Expand Up @@ -2035,6 +2095,31 @@ fn lint_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, unwrap_args: &[hir::E
}
}

/// lint use of `expect()` for `Option`s and `Result`s
fn lint_expect(cx: &LateContext<'_, '_>, expr: &hir::Expr, expect_args: &[hir::Expr]) {
let obj_ty = walk_ptrs_ty(cx.tables.expr_ty(&expect_args[0]));

let mess = if match_type(cx, obj_ty, &paths::OPTION) {
Some((OPTION_EXPECT_USED, "an Option", "None"))
} else if match_type(cx, obj_ty, &paths::RESULT) {
Some((RESULT_EXPECT_USED, "a Result", "Err"))
} else {
None
};

if let Some((lint, kind, none_value)) = mess {
span_lint(
cx,
lint,
expr.span,
&format!(
"used expect() on {} value. If this value is an {} it will panic",
kind, none_value
),
);
}
}

/// lint use of `ok().expect()` for `Result`s
fn lint_ok_expect(cx: &LateContext<'_, '_>, expr: &hir::Expr, ok_args: &[hir::Expr]) {
if_chain! {
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/panic_unimplemented.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ declare_clippy_lint! {
declare_clippy_lint! {
/// **What it does:** Checks for usage of `unreachable!`.
///
/// **Why is this bad?** This macro can cause cause code to panics
/// **Why is this bad?** This macro can cause code to panic
///
/// **Known problems:** None.
///
Expand Down
8 changes: 7 additions & 1 deletion tests/ui/methods.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
// aux-build:option_helpers.rs
// compile-flags: --edition 2018

#![warn(clippy::all, clippy::pedantic, clippy::option_unwrap_used)]
#![warn(
clippy::all,
clippy::pedantic,
clippy::option_unwrap_used,
clippy::option_expect_used,
clippy::result_expect_used
)]
#![allow(
clippy::blacklisted_name,
clippy::default_trait_access,
Expand Down

0 comments on commit 57dcc0d

Please sign in to comment.