Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking Issue: lints that lint against match and if-lets should be more consistent #12618

Open
J-ZhengLi opened this issue Apr 2, 2024 · 3 comments
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages C-tracking-issue Category: Tracking Issue good-first-issue These issues are a good way to get started with Clippy

Comments

@J-ZhengLi
Copy link
Member

J-ZhengLi commented Apr 2, 2024

Description

there are lints that check (or at least should be checking) for match or if-let on Option or Result, such as [question_mark], which checks all combinations now.

But there are some lints that either checks only match exprs on Option and Result, or checks match and if-let on Options, which does not seems consistent, consider the same rules could be applied for both.

Here is a short list of lints that I found which has this problem (I didn't went into it enough so it's highly possible there are missing some lint, ping me to add them in):

Lint Name What's Missing PR # Finished
manual_unwrap_or linting for if-let expressions
manual_unwrap_or_default linting for Result type
manual_let_else (somewhat belongs to this list... I think) linting for Result type
(The pat_and_expr_can_be_question_mark could use some improvement, since not only Options can use question mark, any types that impls Try trait can use it, like Result ofc)

Not in the list but could someone plz confirm that the problem which [if_let_mutex] trying to prevent can or cannot happen with match?

Version

No response

Additional Labels

@rustbot label +I-false-nagative +C-enhancement +good-first-issue

@J-ZhengLi J-ZhengLi added good-first-issue These issues are a good way to get started with Clippy C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages I-false-negative Issue: The lint should have been triggered on code, but wasn't labels Apr 2, 2024
@J-ZhengLi
Copy link
Member Author

(somewhat) related to the discussion in #12610

@J-ZhengLi J-ZhengLi added C-tracking-issue Category: Tracking Issue and removed I-false-negative Issue: The lint should have been triggered on code, but wasn't labels Apr 2, 2024
@rufevean
Copy link

rufevean commented Apr 8, 2024

Am i reproducing this issue correctly?

fn manual_unwrap_or_example(opt: option<i32>) -> i32 {
    if let some(val) = opt {
        val
    } else {
        0 
    }
}

fn main() {
    let opt_none: option<i32> = none;
    println!("{}", manual_unwrap_or_example(opt_none));
}

if we use

cargo clippy -- -D clippy::manual_unwrap_or

on this code, it shows no warning or error while it should be suggesting us to use unwrap_or () .

@J-ZhengLi
Copy link
Member Author

Am i reproducing this issue correctly?

fn manual_unwrap_or_example(opt: option<i32>) -> i32 {
    if let some(val) = opt {
        val
    } else {
        0 
    }
}

fn main() {
    let opt_none: option<i32> = none;
    println!("{}", manual_unwrap_or_example(opt_none));
}

if we use

cargo clippy -- -D clippy::manual_unwrap_or

on this code, it shows no warning or error while it should be suggesting us to use unwrap_or () .

Yeah, except it should be some other value except 0 in the else branch. (This repro triggers manual_unwrap_or_default flawlessly, but changing the number doesn't triggers manual_unwrap_or, which is why I think they are inconsistent 😄 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages C-tracking-issue Category: Tracking Issue good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

No branches or pull requests

2 participants