Add map_err_ignore lint

[deg4uss3r]

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

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

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthiaskrgr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[deg4uss3r]

r? @yaahc

[yaahc]

I made a bunch of changes to the example and pushed to your branch so we can just skip ahead of that whole bit, lmk if you have any objections to how I redid things. Everything else looks great, sorry for the late follow up 😅. Once you've had a chance to checkout the changes I made and you're happy it's ready lmk and I'll start bors.

[deg4uss3r]

I made a bunch of changes to the example and pushed to your branch so we can just skip ahead of that whole bit, lmk if you have any objections to how I redid things. Everything else looks great, sorry for the late follow up 😅. Once you've had a chance to checkout the changes I made and you're happy it's ready lmk and I'll start bors.

Super happy with the changes! Sorry, I was not touching a computer over the long holiday weekend for my sanity. I love the new example :D :D :D

[Manishearth]

I think this should not be on by default: probably should be pedantic.

Error handling is a vast space and techniques vary based on codebase and use case. Quite often you want to do this when the error is irrelevant. In such a case you can just map_err with a _e argument, but in general we're wary with on-by-default lints that would fire massively over a codebase where most of the changes would be to just ignore the lint.

Making it a pedantic lint means that people who are using this kind of error handling strategy can use it, but it does not cause problems for people who aren't.

[deg4uss3r]

I think this should not be on by default: probably should be pedantic.

Great catch @Manishearth I completely agree, this is a new space for me so I wasn't sure if style was the right place to make it not on by default etc. I'll fix that soon :)

[yaahc]

LGTM ty again for setting this up @deg4uss3r

@bors r+

[bors]

📌 Commit d719b48 has been approved by yaahc

[bors]

⌛ Testing commit d719b48 with merge f82e84c...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: yaahc
Pushing f82e84c to master...

[lopopolo]

Hi folks, I'd like to echo @Manishearth's concerns about the high blast radius and low signal on this lint.

From the discussion in this PR, the doc comments, and docs -- https://rust-lang.github.io/rust-clippy/master/index.html#map_err_ignore -- it looks like this lint is intending to check whether an error is ignored and replaced with a unit enum variant where that enum variant could instead wrap the error that is being mapped over and ignored.

From a glance at the code, this does not appear to be what the lint checks for. From what I can tell, this lint only looks for closures with a wildcard pattern.

This lint triggers for code which constructs an enum with its own notion of context:

let store = u16::try_from(new)
    .map_err(|_| IncrementLinenoError::Overflow(usize::from(u16::MAX)))?;

Or for this code which maps the error to a ZST:

interp
    .add_fetch_lineno(1)
    .map_err(|_| ParserLineCountError::new())?;

Or for this example which constructs a custom error with a function call:

let revision = config
    .ruby_revision()
    .parse::<Int>()
    .map_err(|_| NotDefinedError::global_constant("RUBY_REVISION"))?;

I like to run clippy in pedantic mode because generally the lints are useful and I try hard to not disable lints.

Is part of rolling out new lints running them against existing code bases to see what churn they suggest? Typically when RuboCop (a Ruby linter) rolls out new lints, they include tests for cases where a lint is expected to fire and when the lint is not expected to fire. This feedback comes from artichoke/artichoke@a993e45.

Given this comment above, would this lint be better placed in the restriction category?

Error handling is a vast space and techniques vary based on codebase and use case. Quite often you want to do this when the error is irrelevant. In such a case you can just map_err with a _e argument, but in general we're wary with on-by-default lints that would fire massively over a codebase where most of the changes would be to just ignore the lint.

[deg4uss3r]

Hi @lopopolo sorry this lint is giving you a bit of trouble!

I should have some time this week to look at only triggering on a unit enum. This looks like it should help you out and do more of what the intention is. :)

[deg4uss3r]

Updated PR in #6416