From a7ad78f3eb2886a942c077461f6e205ca72f9cb2 Mon Sep 17 00:00:00 2001 From: "Heinz N. Gies" Date: Tue, 15 Oct 2019 21:33:07 +0200 Subject: [PATCH] Add expect Co-Authored-By: Philipp Krones --- clippy_lints/src/methods/mod.rs | 87 ++++++++++++++++++++++++- clippy_lints/src/panic_unimplemented.rs | 2 +- tests/ui/methods.rs | 8 ++- 3 files changed, 94 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index efa283b823d..ac8a1dbdac3 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -73,7 +73,7 @@ /// **Known problems:** None. /// /// **Example:** - /// Using unwrap on an `Option`: + /// Using unwrap on an `Result`: /// /// ```rust /// let res: Result = Ok(1); @@ -91,6 +91,63 @@ "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 = Ok(1); + /// res.expect("one"); + /// ``` + /// + /// Better: + /// + /// ```rust + /// let res: Result = 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 @@ -1037,6 +1094,8 @@ 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, @@ -1095,6 +1154,7 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) { ["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]), @@ -2063,6 +2123,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! { diff --git a/clippy_lints/src/panic_unimplemented.rs b/clippy_lints/src/panic_unimplemented.rs index 6981ecff0d0..94efb5acc00 100644 --- a/clippy_lints/src/panic_unimplemented.rs +++ b/clippy_lints/src/panic_unimplemented.rs @@ -76,7 +76,7 @@ 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. /// diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 54a58e0c86a..ca8358754ee 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -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, -- 2.44.0