Add ignored_result_err lint to detect discarded Result error variants

[scuzzycheese]

View all comments

changelog: new lint: [ignored_result_err]: Add ignored_result_err lint to detect discarded Result error variants. fixes #16107

[rustbot]

Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @dswij (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 8 candidates
• 8 candidates expanded to 8 candidates
• Random selection from Jarcho, dswij, llogiq, samueltardieu

[github-actions]

Lintcheck changes for bf50705

Lint	Added	Removed	Changed
clippy::ignored_result_err	309	0	0

This comment will be updated if you push new changes

[dswij]

r? clippy

[scuzzycheese]

Re-opening PR, I made mistake by resetting my origin.

[scuzzycheese]

@rustbot ready

[scuzzycheese]

Hi @flip1995
Just bumping this since I accidentally closed and reopened it a few days ago, which threw off the bot status.
Whenever you have some free time, I’d love to get your feedback on the implementation!

[scuzzycheese]

@rustbot ready

[scuzzycheese]

@rustbot ready

[scuzzycheese]

Hi @flip1995
Just bumping this again. Just did a rebase. Hoping someone can review this soon.
Thanks.

[scuzzycheese]

Hi @flip1995
Just bumping this again. Just did a rebase. Hoping someone can review this soon.
Thanks.

[scuzzycheese]

Hi @flip1995
Just bumping this again. Just did a rebase. Hoping someone can review this soon.
Thanks.

[flip1995]

Implementation, tests and lint category mostly LGTM. Some small suggestions to think about

Sorry for taking so long with the review. Thanks for sticking with it and bumping it!

View changes since this review

[flip1995]

One more idea for a test: else if Err(e) = some_call()

This should not lint, as the error is not discarded.

Before trying to detect this in this lint, please check if there is already a lint for this pattern (also in pedantic/restriction) . If there is, the lint added in this PR doesn't need to also cover it.

[scuzzycheese]

I just want to clarify, do you mean adding a test like this?

// Should NOT lint — `if let Err(e)` binds the error, it is not discarded
if let Err(e) = some_call() {
    println!("failed: {e}");
}

Or do you mean specifically on conjunction with the Ok Branch?

    if let Ok(res) = some_call() {
        //~^ ignored_result_err
        println!("{res}");
    } else if let Err(e) = some_call() {
        println!("failed: {e}");
    }

My assumption is that those are two separate calls to some_call(), and the error for the first call is discarded, but not discarded for the second call.
I'm going to assume you're talking about the first example which shouldn't lint, and the second one should still lint.

Unless I'm completely misunderstanding what you mean. :D

[flip1995]

I was talking about the second example.. kinda. The first is also a good test to add though.

Small modification for the second example: only call some_call once:

    let sc = some_call()
    if let Ok(res) = sc {
        //~^ ignored_result_err
        println!("{res}");
    } else if let Err(e) = sc {
        println!("failed: {e}");
    }

Please check if another lint also detects this pattern and if so, no need to spend time on making this lint detect it.

[scuzzycheese]

I couldn't find any lints that detect this pattern.

The ones that are a closest match are:

• redundant_pattern_matching
  • This detects if let Ok(_) = x where the binding is unused → .is_ok()
  • However our arms bind and use res/e
• needless_match
  • This detects if-let/match that reconstructs the same value (Ok(v) => Ok(v), Some(v)=>Some(v)). A no-op
  • My arms don't reconstruct; they consume the values
• manual_ok_err
  • This detect arms that map to Some(v)/None → .ok()/.err()
  • Our arms produce side effects (println!), not Option
• option_if_let_else
  • This detects if let Some(v) = opt {…} else {…} → map_or
  • Option only, and single if let/else, not Ok + else if let Err

Anyway, I've updated this to detect that pattern, and it catches your use case. Nice catch! Thank you!

[scuzzycheese]

Updated the code. I'll amend the commit. Hopefully this is good.

View changes since this review

[rustbot]

This lint has been nominated for inclusion.

A FCP topic has been created on Zulip.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.