New lint [let_else_on_result_ok]

[Centri3]

Closes #9792.

changelog: New lint [let_else_on_result_ok]

[rustbot]

r? @giraffate

(rustbot has picked a reviewer for you, use r? to override)

[bors]

☔ The latest upstream changes (presumably #10951) made this pull request unmergeable. Please resolve the merge conflicts.

[Alexendoo]

You explicitly do have to handle the case where there's an error with let/else already, what's the scenario this lint is expected to benefit?

[Centri3]

what's the scenario this lint is expected to benefit?

This tweet easily explains it best: https://twitter.com/nick_r_cameron/status/1588224853336793089
If you want to actually ignore the Err case's contents, you can use .ok, but which is a bit clunky.

(though the lint description is incorrect, I wrote that pretty late :D)

[Alexendoo]

Looking through some examples it seems like this would be almost entirely false positives, meaning most of the changes happening would be making code worse to satisfy the lint by adding .ok() or similar

[Centri3]

We could probably limit it to ones that return, rather than continue, since almost all of which come from that. Majority of the rest seem to be worth linting (like returning None from Ok, or constructing an entirely new Err that doesn't include the old Err's information)

[Alexendoo]

There's nothing wrong with those cases though, e.g. plenty of them are handling SendError or similar which are pointless to include in the error information

A specific lint that suggests if let Ok(v) = x else { return None }; -> let v = x.ok()?; would be reasonable since it's more concise, but there's nothing particularly wrong with the former

[Centri3]

e.g. plenty of them are handling SendError

I only see a select few that do that. Majority of the others are ignoring an anyhow::Result or some custom error type, that does include error information. (I mean, one even ignores a serde_json::Error which is very silly)

There really isn't any particularly good reason to remove such information in those cases, at worst you can just print to stderr instead before dropping it

[Alexendoo]

I went through a few pages, ignoring any continues/breaks/return None the majority were still false positives which is far too high

[Centri3]

That's fair. I do agree that there are many cases where this isn't applicable so I'll see if there's some way to make it more specialized.

[Centri3]

So, I've made a couple changes to it. It now only lints if the following are true:

• The containing function returns Result
• There's a return in the else block
• The Result type returned from the called function (or just the type of the pattern, really) has a non-zero number of fields in the Err variant. That way, something like SendError isn't linted (e: It now also lints non_exhaustive structs/enums)

Note that the final one would not account for functions that only ever return one variant, like let's say

enum MyErrorType {
	A,
	B(u32),
}

fn my_error() -> Result<(), MyErrorType> {
	Err(MyErrorType::A)
}

Will still be linted. But I think this is fine.

I think that should be good now. Is there anything else it should ignore?

[bors]

☔ The latest upstream changes (presumably #10994) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Hey this is triage, I'm closing this due to inactivity. Currently, @Centri3 sadly doesn't have the time to continue this implementation. If anyone is interested in continuing this PR, you're more than welcome to create a new PR and push it over the finish line. :D

Thank you to @Centri3 and the reviewers for the time, that you already put into this!

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review