Make map_err_ignore lint more precise

[deg4uss3r]

@lopopolo Noticed the that map_err_ignore lint was not as precise as it should be and gave some good examples in their codebase.

This now checks the body of the closure is a unit enum variant (as the original intent of the lint) and updates the uitest for map_err to make sure we only warn on this case.

I have also tested on their code, and appears good (after removing all #![allow(clippy::map_err_ignore)]s)!

r? @yaahc

---

Please write a short comment explaining your change (or "none" for internal only changes)
changelog: Make map_err_ignore lint more precise

[camsteffen]

I understand that this lint is producing a large amount of unwanted warns, but this is an arbitrary reduction IMHO. A unit enum very well may provide all the context that is needed for a particular error. And with a non-unit-enum, the error is still discarded and context is potentially lost.

Idea: What if we allowed on map_err(|_ignored| ...)? We could add a hint to change the binding to _ignored. You could say that the _ already communicates that the error is ignored, but maybe it should be more explicit in this case?

Or maybe it should just be a restriction lint.

[deg4uss3r]

Idea: What if we allowed on map_err(|_ignored| ...)? We could add a hint to change the binding to _ignored

The lint will indeed pass if you have: .map_err(|_ignored| ...), we could add that as a hint something like:

"Consider wrapping the error in an enum variant, or using a named wildcard (.map_err(|_ignored| ...) to intentionally ignore the error"

[camsteffen]

Yes, I think we should hint to the user that that option available. Though I wonder if we should "officially" only support _ignored? To me, something like _e does not indicate that the programmer intended to ignore the error any more than _.

[ebroto]

IMO we should treat _ and _e equally in this case as both imply that the argument is being ignored. Blessing _ignored in particular seems arbitrary to me.

I would go for moving this lint to restriction as we can't prove there's anything intrinsically bad in the linted code.

[camsteffen]

FWIW, I got the idea from IntelliJ which does not lint on an empty catch block (Java) if the exception is named ignore or ignored.

[ebroto]

FWIW, I got the idea from IntelliJ which does not lint on an empty catch block (Java) if the exception is named ignore or ignored.

I would be personally more open to that if it was a convention, not sure if it is in Java. But to me, identifiers are "very personal territory", i.e. flame war assured :)

[camsteffen]

Well I think it's a convention imposed by IntelliJ...

[yaahc]

IMO we should treat _ and _e equally in this case as both imply that the argument is being ignored. Blessing _ignored in particular seems arbitrary to me.

I would go for moving this lint to restriction as we can't prove there's anything intrinsically bad in the linted code.

There is a difference between _ and _e in rust already. The former explicitly doesn't create a binding and will cause the value associated to be dropped immediately, where as the latter will actually create a binding and keep the type alive until it goes out of scope.

My feeling is that this lint is less useful if it only lints unit enums. My impression of this lint is that it's for users who want to include as much information in their error reports as possible and users who want help avoiding accidentally discarding errors, catching errors that they discarded in the past before when they weren't be careful about error handling. To me using strings as errors and discarding the source falls squarely into the "probably wasn't caring about error handling earlier" case where someone might want to enable this lint to clean up all instances of the old error handling style.

My preference would be to do the following:

• move the lint to restriction because it isn't really a universally best practice kinda thing, there are plenty of legit reasons to discard errors.
• keep the behaviour how it was previously, with the new warning to indicate that any binding other than _ will avoid triggering the lint.

[deg4uss3r]

I definitely agree with @yaahc, if we cannot keep this in pedantic I'd like to go back to the original intent and just let the user know they can ignore the warning with an ignored variable (it doesn't matter what, just so long as it isn't the Wildcard _).

I am going to do this in a new MR since this one's intention will now change.

[camsteffen]

There is a difference between _ and _e in rust already. The former explicitly doesn't create a binding and will cause the value associated to be dropped immediately, where as the latter will actually create a binding and keep the type alive until it goes out of scope.

I'm not sure that supports the way this lint works because that tells me I should prefer _, if anything.

[yaahc]

There is a difference between _ and _e in rust already. The former explicitly doesn't create a binding and will cause the value associated to be dropped immediately, where as the latter will actually create a binding and keep the type alive until it goes out of scope.

I'm not sure that supports the way this lint works because that tells me I should prefer _, if anything.

I don't think it actually matters in function arguments, as far as I can tell arguments don't drop early if not bound the same way they do in let statements.

#[derive(Debug)]
struct DropPrint(&'static str);

impl Drop for DropPrint {
    fn drop(&mut self) {
        println!("dropped {:?}", self)
    }
}

fn main() {
    let _ignored = DropPrint("first");
    let _ = DropPrint("second");
    print_ignore(DropPrint("third"));
    print_nobind(DropPrint("fourth"));
}

fn print_ignore(_ignore: DropPrint) {
    println!("print ignore");
}

fn print_nobind(_: DropPrint) {
    println!("print nobind");
}

Running this prints the following:

dropped DropPrint("second")
print ignore
dropped DropPrint("third")
print nobind
dropped DropPrint("fourth")
dropped DropPrint("first")

So there's no actual difference between them in this context. Imo _ is equivalent to saying "completely discard this" where as _ident is like saying "I'm not gonna use this but don't warn me about it, I know what I'm doing". I think inverting the behaviour would be significantly worse overall than just linting on the _ case, which is the one ppl will likely use if they're trying to just get the error handling done without focusing on it.

[ebroto]

In case someone was subscribed to this PR the new one is here: #6416

[deg4uss3r]

Thanks @ebroto :)