Auto merge of #5998 - deg4uss3r:master, r=yaahc

Add map_err_ignore lint

In a large code base a lot of times errors are ignored by using something like:

```rust
foo.map_err(|_| Some::Enum)?;
```

This drops the original error in favor of a enum that will not have the original error's context. This lint helps catch throwing away the original error in favor of an enum without its context.

---

*Please keep the line below*
changelog: Added map_err_ignore lint

8 files changed, 214 insertions, 18 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 285a2ff8060..2aa4991488c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1676,6 +1676,7 @@ Released 2018-09-13
 [`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names
 [`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone
 [`map_entry`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_entry
+[`map_err_ignore`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_err_ignore
 [`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten
 [`map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_identity
 [`map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index c017c5cb5d0..44afd7e82fe 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -231,6 +231,7 @@ mod main_recursion;
 mod manual_async_fn;
 mod manual_non_exhaustive;
 mod map_clone;
+mod map_err_ignore;
 mod map_identity;
 mod map_unit_fn;
 mod match_on_vec_items;
@@ -627,6 +628,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &manual_async_fn::MANUAL_ASYNC_FN,
         &manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
         &map_clone::MAP_CLONE,
+        &map_err_ignore::MAP_ERR_IGNORE,
         &map_identity::MAP_IDENTITY,
         &map_unit_fn::OPTION_MAP_UNIT_FN,
         &map_unit_fn::RESULT_MAP_UNIT_FN,
@@ -920,6 +922,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box implicit_saturating_sub::ImplicitSaturatingSub);
     store.register_late_pass(|| box methods::Methods);
     store.register_late_pass(|| box map_clone::MapClone);
+    store.register_late_pass(|| box map_err_ignore::MapErrIgnore);
     store.register_late_pass(|| box shadow::Shadow);
     store.register_late_pass(|| box types::LetUnitValue);
     store.register_late_pass(|| box types::UnitCmp);
@@ -1186,6 +1189,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::EXPLICIT_INTO_ITER_LOOP),
         LintId::of(&loops::EXPLICIT_ITER_LOOP),
         LintId::of(&macro_use::MACRO_USE_IMPORTS),
+        LintId::of(&map_err_ignore::MAP_ERR_IGNORE),
         LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS),
         LintId::of(&matches::MATCH_BOOL),
         LintId::of(&matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS),
diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs
new file mode 100644
index 00000000000..5298e16a04d
--- /dev/null
+++ b/clippy_lints/src/map_err_ignore.rs
@@ -0,0 +1,147 @@
+use crate::utils::span_lint_and_help;
+
+use rustc_hir::{CaptureBy, Expr, ExprKind, PatKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for instances of `map_err(|_| Some::Enum)`
+    ///
+    /// **Why is this bad?** This map_err throws away the original error rather than allowing the enum to contain and report the cause of the error
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// Before:
+    /// ```rust
+    /// use std::fmt;
+    ///
+    /// #[derive(Debug)]
+    /// enum Error {
+    ///     Indivisible,
+    ///     Remainder(u8),
+    /// }
+    ///
+    /// impl fmt::Display for Error {
+    ///     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+    ///         match self {
+    ///             Error::Indivisible => write!(f, "could not divide input by three"),
+    ///             Error::Remainder(remainder) => write!(
+    ///                 f,
+    ///                 "input is not divisible by three, remainder = {}",
+    ///                 remainder
+    ///             ),
+    ///         }
+    ///     }
+    /// }
+    ///
+    /// impl std::error::Error for Error {}
+    ///
+    /// fn divisible_by_3(input: &str) -> Result<(), Error> {
+    ///     input
+    ///         .parse::<i32>()
+    ///         .map_err(|_| Error::Indivisible)
+    ///         .map(|v| v % 3)
+    ///         .and_then(|remainder| {
+    ///             if remainder == 0 {
+    ///                 Ok(())
+    ///             } else {
+    ///                 Err(Error::Remainder(remainder as u8))
+    ///             }
+    ///         })
+    /// }
+    ///  ```
+    ///
+    ///  After:
+    ///  ```rust
+    /// use std::{fmt, num::ParseIntError};
+    ///
+    /// #[derive(Debug)]
+    /// enum Error {
+    ///     Indivisible(ParseIntError),
+    ///     Remainder(u8),
+    /// }
+    ///
+    /// impl fmt::Display for Error {
+    ///     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+    ///         match self {
+    ///             Error::Indivisible(_) => write!(f, "could not divide input by three"),
+    ///             Error::Remainder(remainder) => write!(
+    ///                 f,
+    ///                 "input is not divisible by three, remainder = {}",
+    ///                 remainder
+    ///             ),
+    ///         }
+    ///     }
+    /// }
+    ///
+    /// impl std::error::Error for Error {
+    ///     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+    ///         match self {
+    ///             Error::Indivisible(source) => Some(source),
+    ///             _ => None,
+    ///         }
+    ///     }
+    /// }
+    ///
+    /// fn divisible_by_3(input: &str) -> Result<(), Error> {
+    ///     input
+    ///         .parse::<i32>()
+    ///         .map_err(Error::Indivisible)
+    ///         .map(|v| v % 3)
+    ///         .and_then(|remainder| {
+    ///             if remainder == 0 {
+    ///                 Ok(())
+    ///             } else {
+    ///                 Err(Error::Remainder(remainder as u8))
+    ///             }
+    ///         })
+    /// }
+    /// ```
+    pub MAP_ERR_IGNORE,
+    pedantic,
+    "`map_err` should not ignore the original error"
+}
+
+declare_lint_pass!(MapErrIgnore => [MAP_ERR_IGNORE]);
+
+impl<'tcx> LateLintPass<'tcx> for MapErrIgnore {
+    // do not try to lint if this is from a macro or desugaring
+    fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) {
+        if e.span.from_expansion() {
+            return;
+        }
+
+        // check if this is a method call (e.g. x.foo())
+        if let ExprKind::MethodCall(ref method, _t_span, ref args, _) = e.kind {
+            // only work if the method name is `map_err` and there are only 2 arguments (e.g. x.map_err(|_|[1]
+            // Enum::Variant[2]))
+            if method.ident.as_str() == "map_err" && args.len() == 2 {
+                // make sure the first argument is a closure, and grab the CaptureRef, body_id, and body_span fields
+                if let ExprKind::Closure(capture, _, body_id, body_span, _) = args[1].kind {
+                    // check if this is by Reference (meaning there's no move statement)
+                    if capture == CaptureBy::Ref {
+                        // Get the closure body to check the parameters and values
+                        let closure_body = cx.tcx.hir().body(body_id);
+                        // make sure there's only one parameter (`|_|`)
+                        if closure_body.params.len() == 1 {
+                            // make sure that parameter is the wild token (`_`)
+                            if let PatKind::Wild = closure_body.params[0].pat.kind {
+                                // span the area of the closure capture and warn that the
+                                // original error will be thrown away
+                                span_lint_and_help(
+                                    cx,
+                                    MAP_ERR_IGNORE,
+                                    body_span,
+                                    "`map_err(|_|...` ignores the original error",
+                                    None,
+                                    "Consider wrapping the error in an enum variant",
+                                );
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
index a7d38c93433..a386cba8fba 100644
--- a/src/lintlist/mod.rs
+++ b/src/lintlist/mod.rs
@@ -1173,6 +1173,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         module: "entry",
     },
     Lint {
+        name: "map_err_ignore",
+        group: "pedantic",
+        desc: "`map_err` should not ignore the original error",
+        deprecation: None,
+        module: "map_err_ignore",
+    },
+    Lint {
         name: "map_flatten",
         group: "pedantic",
         desc: "using combinations of `flatten` and `map` which can usually be written as a single method call",
diff --git a/tests/ui/drop_ref.rs b/tests/ui/drop_ref.rs
index 9181d789d4f..6b5bcdaa78e 100644
--- a/tests/ui/drop_ref.rs
+++ b/tests/ui/drop_ref.rs
@@ -1,5 +1,6 @@
 #![warn(clippy::drop_ref)]
 #![allow(clippy::toplevel_ref_arg)]
+#![allow(clippy::map_err_ignore)]
 
 use std::mem::drop;
 
diff --git a/tests/ui/drop_ref.stderr b/tests/ui/drop_ref.stderr
index 35ae88b78a4..7974bf56d44 100644
--- a/tests/ui/drop_ref.stderr
+++ b/tests/ui/drop_ref.stderr
@@ -1,108 +1,108 @@
 error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
-  --> $DIR/drop_ref.rs:9:5
+  --> $DIR/drop_ref.rs:10:5
    |
 LL |     drop(&SomeStruct);
    |     ^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::drop-ref` implied by `-D warnings`
 note: argument has type `&SomeStruct`
-  --> $DIR/drop_ref.rs:9:10
+  --> $DIR/drop_ref.rs:10:10
    |
 LL |     drop(&SomeStruct);
    |          ^^^^^^^^^^^
 
 error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
-  --> $DIR/drop_ref.rs:12:5
+  --> $DIR/drop_ref.rs:13:5
    |
 LL |     drop(&owned1);
    |     ^^^^^^^^^^^^^
    |
 note: argument has type `&SomeStruct`
-  --> $DIR/drop_ref.rs:12:10
+  --> $DIR/drop_ref.rs:13:10
    |
 LL |     drop(&owned1);
    |          ^^^^^^^
 
 error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
-  --> $DIR/drop_ref.rs:13:5
+  --> $DIR/drop_ref.rs:14:5
    |
 LL |     drop(&&owned1);
    |     ^^^^^^^^^^^^^^
    |
 note: argument has type `&&SomeStruct`
-  --> $DIR/drop_ref.rs:13:10
+  --> $DIR/drop_ref.rs:14:10
    |
 LL |     drop(&&owned1);
    |          ^^^^^^^^
 
 error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
-  --> $DIR/drop_ref.rs:14:5
+  --> $DIR/drop_ref.rs:15:5
    |
 LL |     drop(&mut owned1);
    |     ^^^^^^^^^^^^^^^^^
    |
 note: argument has type `&mut SomeStruct`
-  --> $DIR/drop_ref.rs:14:10
+  --> $DIR/drop_ref.rs:15:10
    |
 LL |     drop(&mut owned1);
    |          ^^^^^^^^^^^
 
 error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
-  --> $DIR/drop_ref.rs:18:5
+  --> $DIR/drop_ref.rs:19:5
    |
 LL |     drop(reference1);
    |     ^^^^^^^^^^^^^^^^
    |
 note: argument has type `&SomeStruct`
-  --> $DIR/drop_ref.rs:18:10
+  --> $DIR/drop_ref.rs:19:10
    |
 LL |     drop(reference1);
    |          ^^^^^^^^^^
 
 error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
-  --> $DIR/drop_ref.rs:21:5
+  --> $DIR/drop_ref.rs:22:5
    |
 LL |     drop(reference2);
    |     ^^^^^^^^^^^^^^^^
    |
 note: argument has type `&mut SomeStruct`
-  --> $DIR/drop_ref.rs:21:10
+  --> $DIR/drop_ref.rs:22:10
    |
 LL |     drop(reference2);
    |          ^^^^^^^^^^
 
 error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
-  --> $DIR/drop_ref.rs:24:5
+  --> $DIR/drop_ref.rs:25:5
    |
 LL |     drop(reference3);
    |     ^^^^^^^^^^^^^^^^
    |
 note: argument has type `&SomeStruct`
-  --> $DIR/drop_ref.rs:24:10
+  --> $DIR/drop_ref.rs:25:10
    |
 LL |     drop(reference3);
    |          ^^^^^^^^^^
 
 error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
-  --> $DIR/drop_ref.rs:29:5
+  --> $DIR/drop_ref.rs:30:5
    |
 LL |     drop(&val);
    |     ^^^^^^^^^^
    |
 note: argument has type `&T`
-  --> $DIR/drop_ref.rs:29:10
+  --> $DIR/drop_ref.rs:30:10
    |
 LL |     drop(&val);
    |          ^^^^
 
 error: calls to `std::mem::drop` with a reference instead of an owned value. Dropping a reference does nothing.
-  --> $DIR/drop_ref.rs:37:5
+  --> $DIR/drop_ref.rs:38:5
    |
 LL |     std::mem::drop(&SomeStruct);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
 note: argument has type `&SomeStruct`
-  --> $DIR/drop_ref.rs:37:20
+  --> $DIR/drop_ref.rs:38:20
    |
 LL |     std::mem::drop(&SomeStruct);
    |                    ^^^^^^^^^^^
diff --git a/tests/ui/map_err.rs b/tests/ui/map_err.rs
new file mode 100644
index 00000000000..617b6422872
--- /dev/null
+++ b/tests/ui/map_err.rs
@@ -0,0 +1,25 @@
+#![warn(clippy::map_err_ignore)]
+use std::convert::TryFrom;
+use std::error::Error;
+use std::fmt;
+
+#[derive(Debug)]
+enum Errors {
+    Ignored,
+}
+
+impl Error for Errors {}
+
+impl fmt::Display for Errors {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "Error")
+    }
+}
+
+fn main() -> Result<(), Errors> {
+    let x = u32::try_from(-123_i32);
+
+    println!("{:?}", x.map_err(|_| Errors::Ignored));
+
+    Ok(())
+}
diff --git a/tests/ui/map_err.stderr b/tests/ui/map_err.stderr
new file mode 100644
index 00000000000..7273f460380
--- /dev/null
+++ b/tests/ui/map_err.stderr
@@ -0,0 +1,11 @@
+error: `map_err(|_|...` ignores the original error
+  --> $DIR/map_err.rs:22:32
+   |
+LL |     println!("{:?}", x.map_err(|_| Errors::Ignored));
+   |                                ^^^
+   |
+   = note: `-D clippy::map-err-ignore` implied by `-D warnings`
+   = help: Consider wrapping the error in an enum variant
+
+error: aborting due to previous error
+