Auto merge of rust-lang#5957 - xvschneider:AddUnwrapInsideResultLint, r=yaahc

bors · bors · commit dead45fd5b2f · 2020-08-26T23:38:34.000Z
Adding new lint to prevent usage of 'unwrap' inside functions that re…

### Change
Adding a new lint that will emit a warning when using "unwrap" or "expect" inside a function that returns result.
### Motivation
These functions promote recoverable errors to non-recoverable errors which may be undesirable in code bases which wish to avoid panics.
### Test plan
Running:
`TESTNAME=unwrap_in_result cargo uitest
`---

changelog: none
diff --git 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
@@ -314,6 +314,7 @@ mod unused_io_amount;
 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
@@ -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::<i32>()
+    ///         .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::<i32>()
+    ///         .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<Span>,
+}
+
+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<Self::Map> {
+        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
@@ -2516,6 +2516,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         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
@@ -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<bool, String> {
+        // checks whether a string represents a number divisible by 3
+        let i_result = i_str.parse::<i32>();
+        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<bool, String> {
+        // checks whether a string represents a number divisible by 3
+        let i = i_str.parse::<i32>().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<bool> {
+        let i = i_str.parse::<i32>().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
@@ -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<bool, String> {
+LL | |         // checks whether a string represents a number divisible by 3
+LL | |         let i = i_str.parse::<i32>().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::<i32>().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<bool> {
+LL | |         let i = i_str.parse::<i32>().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::<i32>().expect("not a number");
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+
