Auto merge of rust-lang#6157 - rust-lang:result-unit-err, r=ebroto

New lint: result-unit-err

This fixes rust-lang#1721.

- [x] Followed [lint naming conventions][lint_naming]
- [x] Added passing UI tests (including committed `.stderr` file)
- [x] `cargo test` passes locally
- [x] Executed `cargo dev update_lints`
- [x] Added lint documentation
- [x] Run `cargo dev fmt`

[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints

---

changelog: new lint: result-unit-err

CHANGELOG.md

@@ -1918,6 +1918,7 @@ Released 2018-09-13
 [`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
 [`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option
 [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn
+[`result_unit_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_unit_err
 [`reversed_empty_ranges`]: https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges
 [`same_functions_in_if_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_functions_in_if_condition
 [`same_item_push`]: https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push

clippy_lints/src/functions.rs

@@ -1,8 +1,9 @@
 use crate::utils::{
-    attr_by_name, attrs::is_proc_macro, is_must_use_ty, is_trait_impl_item, iter_input_pats, match_def_path,
-    must_use_attr, qpath_res, return_ty, snippet, snippet_opt, span_lint, span_lint_and_help, span_lint_and_then,
-    trait_ref_of_method, type_is_unsafe_function,
+    attr_by_name, attrs::is_proc_macro, is_must_use_ty, is_trait_impl_item, is_type_diagnostic_item, iter_input_pats,
+    last_path_segment, match_def_path, must_use_attr, qpath_res, return_ty, snippet, snippet_opt, span_lint,
+    span_lint_and_help, span_lint_and_then, trait_ref_of_method, type_is_unsafe_function,
 };
+use if_chain::if_chain;
 use rustc_ast::ast::Attribute;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
@@ -16,6 +17,7 @@ use rustc_middle::ty::{self, Ty};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::source_map::Span;
 use rustc_target::spec::abi::Abi;
+use rustc_typeck::hir_ty_to_ty;
 
 declare_clippy_lint! {
     /// **What it does:** Checks for functions with too many parameters.
@@ -169,6 +171,52 @@ declare_clippy_lint! {
     "function or method that could take a `#[must_use]` attribute"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for public functions that return a `Result`
+    /// with an `Err` type of `()`. It suggests using a custom type that
+    /// implements [`std::error::Error`].
+    ///
+    /// **Why is this bad?** Unit does not implement `Error` and carries no
+    /// further information about what went wrong.
+    ///
+    /// **Known problems:** Of course, this lint assumes that `Result` is used
+    /// for a fallible operation (which is after all the intended use). However
+    /// code may opt to (mis)use it as a basic two-variant-enum. In that case,
+    /// the suggestion is misguided, and the code should use a custom enum
+    /// instead.
+    ///
+    /// **Examples:**
+    /// ```rust
+    /// pub fn read_u8() -> Result<u8, ()> { Err(()) }
+    /// ```
+    /// should become
+    /// ```rust,should_panic
+    /// use std::fmt;
+    ///
+    /// #[derive(Debug)]
+    /// pub struct EndOfStream;
+    ///
+    /// impl fmt::Display for EndOfStream {
+    ///     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+    ///         write!(f, "End of Stream")
+    ///     }
+    /// }
+    ///
+    /// impl std::error::Error for EndOfStream { }
+    ///
+    /// pub fn read_u8() -> Result<u8, EndOfStream> { Err(EndOfStream) }
+    ///# fn main() {
+    ///#     read_u8().unwrap();
+    ///# }
+    /// ```
+    ///
+    /// Note that there are crates that simplify creating the error type, e.g.
+    /// [`thiserror`](https://docs.rs/thiserror).
+    pub RESULT_UNIT_ERR,
+    style,
+    "public function returning `Result` with an `Err` type of `()`"
+}
+
 #[derive(Copy, Clone)]
 pub struct Functions {
     threshold: u64,
@@ -188,6 +236,7 @@ impl_lint_pass!(Functions => [
     MUST_USE_UNIT,
     DOUBLE_MUST_USE,
     MUST_USE_CANDIDATE,
+    RESULT_UNIT_ERR,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Functions {
@@ -233,15 +282,16 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
         let attr = must_use_attr(&item.attrs);
         if let hir::ItemKind::Fn(ref sig, ref _generics, ref body_id) = item.kind {
+            let is_public = cx.access_levels.is_exported(item.hir_id);
+            let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
+            if is_public {
+                check_result_unit_err(cx, &sig.decl, item.span, fn_header_span);
+            }
             if let Some(attr) = attr {
-                let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
                 check_needless_must_use(cx, &sig.decl, item.hir_id, item.span, fn_header_span, attr);
                 return;
             }
-            if cx.access_levels.is_exported(item.hir_id)
-                && !is_proc_macro(cx.sess(), &item.attrs)
-                && attr_by_name(&item.attrs, "no_mangle").is_none()
-            {
+            if is_public && !is_proc_macro(cx.sess(), &item.attrs) && attr_by_name(&item.attrs, "no_mangle").is_none() {
                 check_must_use_candidate(
                     cx,
                     &sig.decl,
@@ -257,11 +307,15 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
 
     fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) {
         if let hir::ImplItemKind::Fn(ref sig, ref body_id) = item.kind {
+            let is_public = cx.access_levels.is_exported(item.hir_id);
+            let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
+            if is_public && trait_ref_of_method(cx, item.hir_id).is_none() {
+                check_result_unit_err(cx, &sig.decl, item.span, fn_header_span);
+            }
             let attr = must_use_attr(&item.attrs);
             if let Some(attr) = attr {
-                let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
                 check_needless_must_use(cx, &sig.decl, item.hir_id, item.span, fn_header_span, attr);
-            } else if cx.access_levels.is_exported(item.hir_id)
+            } else if is_public
                 && !is_proc_macro(cx.sess(), &item.attrs)
                 && trait_ref_of_method(cx, item.hir_id).is_none()
             {
@@ -284,18 +338,21 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
             if sig.header.abi == Abi::Rust {
                 self.check_arg_number(cx, &sig.decl, item.span.with_hi(sig.decl.output.span().hi()));
             }
+            let is_public = cx.access_levels.is_exported(item.hir_id);
+            let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
+            if is_public {
+                check_result_unit_err(cx, &sig.decl, item.span, fn_header_span);
+            }
 
             let attr = must_use_attr(&item.attrs);
             if let Some(attr) = attr {
-                let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
                 check_needless_must_use(cx, &sig.decl, item.hir_id, item.span, fn_header_span, attr);
             }
             if let hir::TraitFn::Provided(eid) = *eid {
                 let body = cx.tcx.hir().body(eid);
                 Self::check_raw_ptr(cx, sig.header.unsafety, &sig.decl, body, item.hir_id);
 
-                if attr.is_none() && cx.access_levels.is_exported(item.hir_id) && !is_proc_macro(cx.sess(), &item.attrs)
-                {
+                if attr.is_none() && is_public && !is_proc_macro(cx.sess(), &item.attrs) {
                     check_must_use_candidate(
                         cx,
                         &sig.decl,
@@ -411,6 +468,29 @@ impl<'tcx> Functions {
     }
 }
 
+fn check_result_unit_err(cx: &LateContext<'_>, decl: &hir::FnDecl<'_>, item_span: Span, fn_header_span: Span) {
+    if_chain! {
+        if !in_external_macro(cx.sess(), item_span);
+        if let hir::FnRetTy::Return(ref ty) = decl.output;
+        if let hir::TyKind::Path(ref qpath) = ty.kind;
+        if is_type_diagnostic_item(cx, hir_ty_to_ty(cx.tcx, ty), sym!(result_type));
+        if let Some(ref args) = last_path_segment(qpath).args;
+        if let [_, hir::GenericArg::Type(ref err_ty)] = args.args;
+        if let hir::TyKind::Tup(t) = err_ty.kind;
+        if t.is_empty();
+        then {
+            span_lint_and_help(
+                cx,
+                RESULT_UNIT_ERR,
+                fn_header_span,
+                "this returns a `Result<_, ()>",
+                None,
+                "use a custom Error type instead",
+            );
+        }
+    }
+}
+
 fn check_needless_must_use(
     cx: &LateContext<'_>,
     decl: &hir::FnDecl<'_>,

clippy_lints/src/lib.rs

@@ -582,6 +582,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &functions::MUST_USE_CANDIDATE,
         &functions::MUST_USE_UNIT,
         &functions::NOT_UNSAFE_PTR_ARG_DEREF,
+        &functions::RESULT_UNIT_ERR,
         &functions::TOO_MANY_ARGUMENTS,
         &functions::TOO_MANY_LINES,
         &future_not_send::FUTURE_NOT_SEND,
@@ -1327,6 +1328,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&functions::DOUBLE_MUST_USE),
         LintId::of(&functions::MUST_USE_UNIT),
         LintId::of(&functions::NOT_UNSAFE_PTR_ARG_DEREF),
+        LintId::of(&functions::RESULT_UNIT_ERR),
         LintId::of(&functions::TOO_MANY_ARGUMENTS),
         LintId::of(&get_last_with_len::GET_LAST_WITH_LEN),
         LintId::of(&identity_op::IDENTITY_OP),
@@ -1558,6 +1560,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING),
         LintId::of(&functions::DOUBLE_MUST_USE),
         LintId::of(&functions::MUST_USE_UNIT),
+        LintId::of(&functions::RESULT_UNIT_ERR),
         LintId::of(&if_let_some_result::IF_LET_SOME_RESULT),
         LintId::of(&inherent_to_string::INHERENT_TO_STRING),
         LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY),

src/lintlist/mod.rs

@@ -2005,6 +2005,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         deprecation: None,
         module: "map_unit_fn",
     },
+    Lint {
+        name: "result_unit_err",
+        group: "style",
+        desc: "public function returning `Result` with an `Err` type of `()`",
+        deprecation: None,
+        module: "functions",
+    },
     Lint {
         name: "reversed_empty_ranges",
         group: "correctness",

tests/ui/doc_errors.rs

@@ -1,5 +1,6 @@
 // edition:2018
 #![warn(clippy::missing_errors_doc)]
+#![allow(clippy::result_unit_err)]
 
 use std::io;
 

tests/ui/doc_errors.stderr

@@ -1,5 +1,5 @@
 error: docs for function returning `Result` missing `# Errors` section
-  --> $DIR/doc_errors.rs:6:1
+  --> $DIR/doc_errors.rs:7:1
    |
 LL | / pub fn pub_fn_missing_errors_header() -> Result<(), ()> {
 LL | |     unimplemented!();
@@ -9,47 +9,47 @@ LL | | }
    = note: `-D clippy::missing-errors-doc` implied by `-D warnings`
 
 error: docs for function returning `Result` missing `# Errors` section
-  --> $DIR/doc_errors.rs:10:1
+  --> $DIR/doc_errors.rs:11:1
    |
 LL | / pub async fn async_pub_fn_missing_errors_header() -> Result<(), ()> {
 LL | |     unimplemented!();
 LL | | }
    | |_^
 
 error: docs for function returning `Result` missing `# Errors` section
-  --> $DIR/doc_errors.rs:15:1
+  --> $DIR/doc_errors.rs:16:1
    |
 LL | / pub fn pub_fn_returning_io_result() -> io::Result<()> {
 LL | |     unimplemented!();
 LL | | }
    | |_^
 
 error: docs for function returning `Result` missing `# Errors` section
-  --> $DIR/doc_errors.rs:20:1
+  --> $DIR/doc_errors.rs:21:1
    |
 LL | / pub async fn async_pub_fn_returning_io_result() -> io::Result<()> {
 LL | |     unimplemented!();
 LL | | }
    | |_^
 
 error: docs for function returning `Result` missing `# Errors` section
-  --> $DIR/doc_errors.rs:50:5
+  --> $DIR/doc_errors.rs:51:5
    |
 LL | /     pub fn pub_method_missing_errors_header() -> Result<(), ()> {
 LL | |         unimplemented!();
 LL | |     }
    | |_____^
 
 error: docs for function returning `Result` missing `# Errors` section
-  --> $DIR/doc_errors.rs:55:5
+  --> $DIR/doc_errors.rs:56:5
    |
 LL | /     pub async fn async_pub_method_missing_errors_header() -> Result<(), ()> {
 LL | |         unimplemented!();
 LL | |     }
    | |_____^
 
 error: docs for function returning `Result` missing `# Errors` section
-  --> $DIR/doc_errors.rs:84:5
+  --> $DIR/doc_errors.rs:85:5
    |
 LL |     fn trait_method_missing_errors_header() -> Result<(), ()>;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

tests/ui/double_must_use.rs

@@ -1,4 +1,5 @@
 #![warn(clippy::double_must_use)]
+#![allow(clippy::result_unit_err)]
 
 #[must_use]
 pub fn must_use_result() -> Result<(), ()> {

tests/ui/double_must_use.stderr

@@ -1,5 +1,5 @@
 error: this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`
-  --> $DIR/double_must_use.rs:4:1
+  --> $DIR/double_must_use.rs:5:1
    |
 LL | pub fn must_use_result() -> Result<(), ()> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -8,15 +8,15 @@ LL | pub fn must_use_result() -> Result<(), ()> {
    = help: either add some descriptive text or remove the attribute
 
 error: this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`
-  --> $DIR/double_must_use.rs:9:1
+  --> $DIR/double_must_use.rs:10:1
    |
 LL | pub fn must_use_tuple() -> (Result<(), ()>, u8) {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: either add some descriptive text or remove the attribute
 
 error: this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`
-  --> $DIR/double_must_use.rs:14:1
+  --> $DIR/double_must_use.rs:15:1
    |
 LL | pub fn must_use_array() -> [Result<(), ()>; 1] {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

tests/ui/result_unit_error.rs

@@ -0,0 +1,38 @@
+#[warn(clippy::result_unit_err)]
+#[allow(unused)]
+
+pub fn returns_unit_error() -> Result<u32, ()> {
+    Err(())
+}
+
+fn private_unit_errors() -> Result<String, ()> {
+    Err(())
+}
+
+pub trait HasUnitError {
+    fn get_that_error(&self) -> Result<bool, ()>;
+
+    fn get_this_one_too(&self) -> Result<bool, ()> {
+        Err(())
+    }
+}
+
+impl HasUnitError for () {
+    fn get_that_error(&self) -> Result<bool, ()> {
+        Ok(true)
+    }
+}
+
+trait PrivateUnitError {
+    fn no_problem(&self) -> Result<usize, ()>;
+}
+
+pub struct UnitErrorHolder;
+
+impl UnitErrorHolder {
+    pub fn unit_error(&self) -> Result<usize, ()> {
+        Ok(0)
+    }
+}
+
+fn main() {}

tests/ui/result_unit_error.stderr

@@ -0,0 +1,35 @@
+error: this returns a `Result<_, ()>
+  --> $DIR/result_unit_error.rs:4:1
+   |
+LL | pub fn returns_unit_error() -> Result<u32, ()> {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::result-unit-err` implied by `-D warnings`
+   = help: use a custom Error type instead
+
+error: this returns a `Result<_, ()>
+  --> $DIR/result_unit_error.rs:13:5
+   |
+LL |     fn get_that_error(&self) -> Result<bool, ()>;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: use a custom Error type instead
+
+error: this returns a `Result<_, ()>
+  --> $DIR/result_unit_error.rs:15:5
+   |
+LL |     fn get_this_one_too(&self) -> Result<bool, ()> {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: use a custom Error type instead
+
+error: this returns a `Result<_, ()>
+  --> $DIR/result_unit_error.rs:33:5
+   |
+LL |     pub fn unit_error(&self) -> Result<usize, ()> {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: use a custom Error type instead
+
+error: aborting due to 4 previous errors
+