Auto merge of #5977 - xvschneider:AddLintPanicInResult, r=matthiaskrgr

Add lint panic in result

### Change
Adding a new "restriction" lint that will emit a warning when using "panic", "unimplemented" or "unreachable" in a function of type option/result.

### Motivation
Some codebases must avoid crashes at all costs, and hence functions of type option/result must return an error instead of crashing.

### Test plan
Running:
TESTNAME=panic_in_result cargo uitest ---

changelog: none

CHANGELOG.md

@@ -1755,6 +1755,7 @@ Released 2018-09-13
 [`out_of_bounds_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#out_of_bounds_indexing
 [`overflow_check_conditional`]: https://rust-lang.github.io/rust-clippy/master/index.html#overflow_check_conditional
 [`panic`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic
+[`panic_in_result_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_in_result_fn
 [`panic_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_params
 [`panicking_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#panicking_unwrap
 [`partialeq_ne_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_ne_impl

clippy_lints/src/lib.rs

@@ -269,6 +269,7 @@ mod open_options;
 mod option_env_unwrap;
 mod option_if_let_else;
 mod overflow_check_conditional;
+mod panic_in_result_fn;
 mod panic_unimplemented;
 mod partialeq_ne_impl;
 mod path_buf_push_overwrite;
@@ -751,6 +752,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &option_env_unwrap::OPTION_ENV_UNWRAP,
         &option_if_let_else::OPTION_IF_LET_ELSE,
         &overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL,
+        &panic_in_result_fn::PANIC_IN_RESULT_FN,
         &panic_unimplemented::PANIC,
         &panic_unimplemented::PANIC_PARAMS,
         &panic_unimplemented::TODO,
@@ -1091,6 +1093,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box manual_async_fn::ManualAsyncFn);
     store.register_early_pass(|| box redundant_field_names::RedundantFieldNames);
     store.register_late_pass(|| box vec_resize_to_zero::VecResizeToZero);
+    store.register_late_pass(|| box panic_in_result_fn::PanicInResultFn);
+
     let single_char_binding_names_threshold = conf.single_char_binding_names_threshold;
     store.register_early_pass(move || box non_expressive_names::NonExpressiveNames {
         single_char_binding_names_threshold,
@@ -1135,6 +1139,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS),
         LintId::of(&missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS),
         LintId::of(&modulo_arithmetic::MODULO_ARITHMETIC),
+        LintId::of(&panic_in_result_fn::PANIC_IN_RESULT_FN),
         LintId::of(&panic_unimplemented::PANIC),
         LintId::of(&panic_unimplemented::TODO),
         LintId::of(&panic_unimplemented::UNIMPLEMENTED),

clippy_lints/src/panic_in_result_fn.rs

@@ -0,0 +1,90 @@
+use crate::utils::{is_expn_of, is_type_diagnostic_item, return_ty, span_lint_and_then};
+use rustc_hir as hir;
+use rustc_hir::intravisit::{self, FnKind, NestedVisitorMap, Visitor};
+use rustc_hir::Expr;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::hir::map::Map;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::Span;
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `panic!`, `unimplemented!`, `todo!` or `unreachable!` in a function of type result.
+    ///
+    /// **Why is this bad?** For some codebases, it is desirable for functions of type result to return an error instead of crashing. Hence unimplemented, panic and unreachable should be avoided.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// fn result_with_panic() -> Result<bool, String>
+    /// {
+    ///     panic!("error");
+    /// }
+    /// ```
+    pub PANIC_IN_RESULT_FN,
+    restriction,
+    "functions of type `Result<..>` that contain `panic!()`, `todo!()` or `unreachable()` or `unimplemented()` "
+}
+
+declare_lint_pass!(PanicInResultFn  => [PANIC_IN_RESULT_FN]);
+
+impl<'tcx> LateLintPass<'tcx> for PanicInResultFn {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        fn_kind: FnKind<'tcx>,
+        _: &'tcx hir::FnDecl<'tcx>,
+        body: &'tcx hir::Body<'tcx>,
+        span: Span,
+        hir_id: hir::HirId,
+    ) {
+        if !matches!(fn_kind, FnKind::Closure(_))
+            && is_type_diagnostic_item(cx, return_ty(cx, hir_id), sym!(result_type))
+        {
+            lint_impl_body(cx, span, body);
+        }
+    }
+}
+
+struct FindPanicUnimplementedUnreachable {
+    result: Vec<Span>,
+}
+
+impl<'tcx> Visitor<'tcx> for FindPanicUnimplementedUnreachable {
+    type Map = Map<'tcx>;
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+        if ["unimplemented", "unreachable", "panic", "todo"]
+            .iter()
+            .any(|fun| is_expn_of(expr.span, fun).is_some())
+        {
+            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, body: &'tcx hir::Body<'tcx>) {
+    let mut panics = FindPanicUnimplementedUnreachable { result: Vec::new() };
+    panics.visit_expr(&body.value);
+    if !panics.result.is_empty() {
+        span_lint_and_then(
+            cx,
+            PANIC_IN_RESULT_FN,
+            impl_span,
+            "used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`",
+            move |diag| {
+                diag.help(
+                    "`unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing",
+                );
+                diag.span_note(panics.result, "return Err() instead of panicking");
+            },
+        );
+    }
+}

src/lintlist/mod.rs

@@ -1718,6 +1718,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "panic_unimplemented",
     },
+    Lint {
+        name: "panic_in_result_fn",
+        group: "restriction",
+        desc: "functions of type `Result<..>` that contain `panic!()`, `todo!()` or `unreachable()` or `unimplemented()` ",
+        deprecation: None,
+        module: "panic_in_result_fn",
+    },
     Lint {
         name: "panic_params",
         group: "style",

tests/ui/panic_in_result_fn.rs

@@ -0,0 +1,70 @@
+#![warn(clippy::panic_in_result_fn)]
+
+struct A;
+
+impl A {
+    fn result_with_panic() -> Result<bool, String> // should emit lint
+    {
+        panic!("error");
+    }
+
+    fn result_with_unimplemented() -> Result<bool, String> // should emit lint
+    {
+        unimplemented!();
+    }
+
+    fn result_with_unreachable() -> Result<bool, String> // should emit lint
+    {
+        unreachable!();
+    }
+
+    fn result_with_todo() -> Result<bool, String> // should emit lint
+    {
+        todo!("Finish this");
+    }
+
+    fn other_with_panic() // should not emit lint
+    {
+        panic!("");
+    }
+
+    fn other_with_unreachable() // should not emit lint
+    {
+        unreachable!();
+    }
+
+    fn other_with_unimplemented() // should not emit lint
+    {
+        unimplemented!();
+    }
+
+    fn other_with_todo() // should not emit lint
+    {
+        todo!("finish this")
+    }
+
+    fn result_without_banned_functions() -> Result<bool, String> // should not emit lint
+    {
+        Ok(true)
+    }
+}
+
+fn function_result_with_panic() -> Result<bool, String> // should emit lint
+{
+    panic!("error");
+}
+
+fn todo() {
+    println!("something");
+}
+
+fn function_result_with_custom_todo() -> Result<bool, String> // should not emit lint
+{
+    todo();
+    Ok(true)
+}
+
+fn main() -> Result<(), String> {
+    todo!("finish main method");
+    Ok(())
+}

tests/ui/panic_in_result_fn.stderr

@@ -0,0 +1,105 @@
+error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
+  --> $DIR/panic_in_result_fn.rs:6:5
+   |
+LL | /     fn result_with_panic() -> Result<bool, String> // should emit lint
+LL | |     {
+LL | |         panic!("error");
+LL | |     }
+   | |_____^
+   |
+   = note: `-D clippy::panic-in-result-fn` implied by `-D warnings`
+   = help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
+note: return Err() instead of panicking
+  --> $DIR/panic_in_result_fn.rs:8:9
+   |
+LL |         panic!("error");
+   |         ^^^^^^^^^^^^^^^^
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
+  --> $DIR/panic_in_result_fn.rs:11:5
+   |
+LL | /     fn result_with_unimplemented() -> Result<bool, String> // should emit lint
+LL | |     {
+LL | |         unimplemented!();
+LL | |     }
+   | |_____^
+   |
+   = help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
+note: return Err() instead of panicking
+  --> $DIR/panic_in_result_fn.rs:13:9
+   |
+LL |         unimplemented!();
+   |         ^^^^^^^^^^^^^^^^^
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
+  --> $DIR/panic_in_result_fn.rs:16:5
+   |
+LL | /     fn result_with_unreachable() -> Result<bool, String> // should emit lint
+LL | |     {
+LL | |         unreachable!();
+LL | |     }
+   | |_____^
+   |
+   = help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
+note: return Err() instead of panicking
+  --> $DIR/panic_in_result_fn.rs:18:9
+   |
+LL |         unreachable!();
+   |         ^^^^^^^^^^^^^^^
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
+  --> $DIR/panic_in_result_fn.rs:21:5
+   |
+LL | /     fn result_with_todo() -> Result<bool, String> // should emit lint
+LL | |     {
+LL | |         todo!("Finish this");
+LL | |     }
+   | |_____^
+   |
+   = help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
+note: return Err() instead of panicking
+  --> $DIR/panic_in_result_fn.rs:23:9
+   |
+LL |         todo!("Finish this");
+   |         ^^^^^^^^^^^^^^^^^^^^^
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
+  --> $DIR/panic_in_result_fn.rs:52:1
+   |
+LL | / fn function_result_with_panic() -> Result<bool, String> // should emit lint
+LL | | {
+LL | |     panic!("error");
+LL | | }
+   | |_^
+   |
+   = help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
+note: return Err() instead of panicking
+  --> $DIR/panic_in_result_fn.rs:54:5
+   |
+LL |     panic!("error");
+   |     ^^^^^^^^^^^^^^^^
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: used `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` in a function that returns `Result`
+  --> $DIR/panic_in_result_fn.rs:67:1
+   |
+LL | / fn main() -> Result<(), String> {
+LL | |     todo!("finish main method");
+LL | |     Ok(())
+LL | | }
+   | |_^
+   |
+   = help: `unimplemented!()`, `unreachable!()`, `todo!()` or `panic!()` should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing
+note: return Err() instead of panicking
+  --> $DIR/panic_in_result_fn.rs:68:5
+   |
+LL |     todo!("finish main method");
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 6 previous errors
+