diff --git a/CHANGELOG.md b/CHANGELOG.md index a5da0f7b767..137b561028a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1778,6 +1778,7 @@ Released 2018-09-13 [`unused_label`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_label [`unused_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_self [`unused_unit`]: https://rust-lang.github.io/rust-clippy/master/index.html#unused_unit +[`unwrap_in_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_in_result [`unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_used [`use_debug`]: https://rust-lang.github.io/rust-clippy/master/index.html#use_debug [`use_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#use_self diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f78de7a175f..577ce6523b4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -314,6 +314,7 @@ macro_rules! declare_clippy_lint { mod unused_self; mod unused_unit; mod unwrap; +mod unwrap_in_result; mod use_self; mod useless_conversion; mod vec; @@ -850,6 +851,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &unused_unit::UNUSED_UNIT, &unwrap::PANICKING_UNWRAP, &unwrap::UNNECESSARY_UNWRAP, + &unwrap_in_result::UNWRAP_IN_RESULT, &use_self::USE_SELF, &useless_conversion::USELESS_CONVERSION, &utils::internal_lints::CLIPPY_LINTS_INTERNAL, @@ -1094,6 +1096,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box pattern_type_mismatch::PatternTypeMismatch); store.register_late_pass(|| box stable_sort_primitive::StableSortPrimitive); store.register_late_pass(|| box repeat_once::RepeatOnce); + store.register_late_pass(|| box unwrap_in_result::UnwrapInResult); store.register_late_pass(|| box self_assignment::SelfAssignment); store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs); @@ -1133,6 +1136,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&shadow::SHADOW_REUSE), LintId::of(&shadow::SHADOW_SAME), LintId::of(&strings::STRING_ADD), + LintId::of(&unwrap_in_result::UNWRAP_IN_RESULT), LintId::of(&verbose_file_reads::VERBOSE_FILE_READS), LintId::of(&write::PRINT_STDOUT), LintId::of(&write::USE_DEBUG), diff --git a/clippy_lints/src/unwrap_in_result.rs b/clippy_lints/src/unwrap_in_result.rs new file mode 100644 index 00000000000..1c7e62ecd3d --- /dev/null +++ b/clippy_lints/src/unwrap_in_result.rs @@ -0,0 +1,140 @@ +use crate::utils::{is_type_diagnostic_item, method_chain_args, return_ty, span_lint_and_then, walk_ptrs_ty}; +use if_chain::if_chain; +use rustc_hir as hir; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::hir::map::Map; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::Span; + +declare_clippy_lint! { + /// **What it does:** Checks for functions of type Result that contain `expect()` or `unwrap()` + /// + /// **Why is this bad?** These functions promote recoverable errors to non-recoverable errors which may be undesirable in code bases which wish to avoid panics. + /// + /// **Known problems:** This can cause false positives in functions that handle both recoverable and non recoverable errors. + /// + /// **Example:** + /// Before: + /// ```rust + /// fn divisible_by_3(i_str: String) -> Result<(), String> { + /// let i = i_str + /// .parse::() + /// .expect("cannot divide the input by three"); + /// + /// if i % 3 != 0 { + /// Err("Number is not divisible by 3")? + /// } + /// + /// Ok(()) + /// } + /// ``` + /// + /// After: + /// ```rust + /// fn divisible_by_3(i_str: String) -> Result<(), String> { + /// let i = i_str + /// .parse::() + /// .map_err(|e| format!("cannot divide the input by three: {}", e))?; + /// + /// if i % 3 != 0 { + /// Err("Number is not divisible by 3")? + /// } + /// + /// Ok(()) + /// } + /// ``` + pub UNWRAP_IN_RESULT, + restriction, + "functions of type `Result<..>` or `Option`<...> that contain `expect()` or `unwrap()`" +} + +declare_lint_pass!(UnwrapInResult=> [UNWRAP_IN_RESULT]); + +impl<'tcx> LateLintPass<'tcx> for UnwrapInResult { + fn check_impl_item(&mut self, cx: &LateContext<'tcx>, impl_item: &'tcx hir::ImplItem<'_>) { + if_chain! { + // first check if it's a method or function + if let hir::ImplItemKind::Fn(ref _signature, _) = impl_item.kind; + // checking if its return type is `result` or `option` + if is_type_diagnostic_item(cx, return_ty(cx, impl_item.hir_id), sym!(result_type)) + || is_type_diagnostic_item(cx, return_ty(cx, impl_item.hir_id), sym!(option_type)); + then { + lint_impl_body(cx, impl_item.span, impl_item); + } + } + } +} + +use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; +use rustc_hir::{Expr, ImplItemKind}; + +struct FindExpectUnwrap<'a, 'tcx> { + lcx: &'a LateContext<'tcx>, + typeck_results: &'tcx ty::TypeckResults<'tcx>, + result: Vec, +} + +impl<'a, 'tcx> Visitor<'tcx> for FindExpectUnwrap<'a, 'tcx> { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + // check for `expect` + if let Some(arglists) = method_chain_args(expr, &["expect"]) { + let reciever_ty = walk_ptrs_ty(self.typeck_results.expr_ty(&arglists[0][0])); + if is_type_diagnostic_item(self.lcx, reciever_ty, sym!(option_type)) + || is_type_diagnostic_item(self.lcx, reciever_ty, sym!(result_type)) + { + self.result.push(expr.span); + } + } + + // check for `unwrap` + if let Some(arglists) = method_chain_args(expr, &["unwrap"]) { + let reciever_ty = walk_ptrs_ty(self.typeck_results.expr_ty(&arglists[0][0])); + if is_type_diagnostic_item(self.lcx, reciever_ty, sym!(option_type)) + || is_type_diagnostic_item(self.lcx, reciever_ty, sym!(result_type)) + { + self.result.push(expr.span); + } + } + + // and check sub-expressions + intravisit::walk_expr(self, expr); + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } +} + +fn lint_impl_body<'tcx>(cx: &LateContext<'tcx>, impl_span: Span, impl_item: &'tcx hir::ImplItem<'_>) { + if_chain! { + + if let ImplItemKind::Fn(_, body_id) = impl_item.kind; + then { + let body = cx.tcx.hir().body(body_id); + let impl_item_def_id = cx.tcx.hir().local_def_id(impl_item.hir_id); + let mut fpu = FindExpectUnwrap { + lcx: cx, + typeck_results: cx.tcx.typeck(impl_item_def_id), + result: Vec::new(), + }; + fpu.visit_expr(&body.value); + + // if we've found one, lint + if !fpu.result.is_empty() { + span_lint_and_then( + cx, + UNWRAP_IN_RESULT, + impl_span, + "used unwrap or expect in a function that returns result or option", + move |diag| { + diag.help( + "unwrap and expect should not be used in a function that returns result or option" ); + diag.span_note(fpu.result, "potential non-recoverable error(s)"); + }); + } + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index eadd2621a40..687fac7baa8 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2516,6 +2516,13 @@ deprecation: None, module: "unused_unit", }, + Lint { + name: "unwrap_in_result", + group: "restriction", + desc: "functions of type `Result<..>` or `Option`<...> that contain `expect()` or `unwrap()`", + deprecation: None, + module: "unwrap_in_result", + }, Lint { name: "unwrap_used", group: "restriction", diff --git a/tests/ui/unwrap_in_result.rs b/tests/ui/unwrap_in_result.rs new file mode 100644 index 00000000000..2aa842adc85 --- /dev/null +++ b/tests/ui/unwrap_in_result.rs @@ -0,0 +1,44 @@ +#![warn(clippy::unwrap_in_result)] + +struct A; + +impl A { + // should not be detected + fn good_divisible_by_3(i_str: String) -> Result { + // checks whether a string represents a number divisible by 3 + let i_result = i_str.parse::(); + match i_result { + Err(_e) => Err("Not a number".to_string()), + Ok(i) => { + if i % 3 == 0 { + return Ok(true); + } + Err("Number is not divisible by 3".to_string()) + }, + } + } + + // should be detected + fn bad_divisible_by_3(i_str: String) -> Result { + // checks whether a string represents a number divisible by 3 + let i = i_str.parse::().unwrap(); + if i % 3 == 0 { + Ok(true) + } else { + Err("Number is not divisible by 3".to_string()) + } + } + + fn example_option_expect(i_str: String) -> Option { + let i = i_str.parse::().expect("not a number"); + if i % 3 == 0 { + return Some(true); + } + None + } +} + +fn main() { + A::bad_divisible_by_3("3".to_string()); + A::good_divisible_by_3("3".to_string()); +} diff --git a/tests/ui/unwrap_in_result.stderr b/tests/ui/unwrap_in_result.stderr new file mode 100644 index 00000000000..56bc2f2d1c0 --- /dev/null +++ b/tests/ui/unwrap_in_result.stderr @@ -0,0 +1,41 @@ +error: used unwrap or expect in a function that returns result or option + --> $DIR/unwrap_in_result.rs:22:5 + | +LL | / fn bad_divisible_by_3(i_str: String) -> Result { +LL | | // checks whether a string represents a number divisible by 3 +LL | | let i = i_str.parse::().unwrap(); +LL | | if i % 3 == 0 { +... | +LL | | } +LL | | } + | |_____^ + | + = note: `-D clippy::unwrap-in-result` implied by `-D warnings` + = help: unwrap and expect should not be used in a function that returns result or option +note: potential non-recoverable error(s) + --> $DIR/unwrap_in_result.rs:24:17 + | +LL | let i = i_str.parse::().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: used unwrap or expect in a function that returns result or option + --> $DIR/unwrap_in_result.rs:32:5 + | +LL | / fn example_option_expect(i_str: String) -> Option { +LL | | let i = i_str.parse::().expect("not a number"); +LL | | if i % 3 == 0 { +LL | | return Some(true); +LL | | } +LL | | None +LL | | } + | |_____^ + | + = help: unwrap and expect should not be used in a function that returns result or option +note: potential non-recoverable error(s) + --> $DIR/unwrap_in_result.rs:33:17 + | +LL | let i = i_str.parse::().expect("not a number"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 2 previous errors +