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

Remove unnecessary match #7040

Closed
SaadiSave opened this issue Apr 6, 2021 · 11 comments · Fixed by #8471
Closed

Remove unnecessary match #7040

SaadiSave opened this issue Apr 6, 2021 · 11 comments · Fixed by #8471
Assignees
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy

Comments

@SaadiSave
Copy link

SaadiSave commented Apr 6, 2021

What it does

What does this lint do?

Remove unnecessary match for error handling when function signatures are the same.

Categories (optional)

  • Kind: style, correctness ?

What is the advantage of the recommended code over the original code

Not sure if there are any performance improvements. The code is more readable and shorter though. This lint also applies for any function pair that returns the same enum, like Option<T> for example.

Drawbacks

None.

Example

// For example, a function has the signature:
fn fname(...) -> Result<T, E> { ... }

// and it is called in another function with the same signature:
fn fname2(...) -> Result<T, E> {
    // This is obviously unnecessary
    match fname(...) {
        Ok(s) => Ok(s),
        Err(e) => Err(e),
    }
}

Could be written as:

// For example, a function has the signature:
fn fname(...) -> Result<T, E> { ... }

// and it is called in another function with the same signature:
fn fname2(...) -> Result<T, E> {
    // Much better
    fname(...)
}

Real world example

async fn destructure_reqwest_response(res: Response) -> Result<Generic, InternalError> {
    ...
}

async fn get_count(...) -> Result<Generic, InternalError> {
    ...
    // Changed this later. Even clippy::pedantic did not detect this, so I assume that a lint is missing.
    match res {
        Ok(r) => match destructure_reqwest_response(r).await {
            Ok(r) => Ok(r),
            Err(e) => Err(e),
        },
        Err(_) => Err(INTERNALERROR),
    }
    ...
}
@SaadiSave SaadiSave added the A-lint Area: New lints label Apr 6, 2021
@giraffate
Copy link
Contributor

It would be better for categories to be style.

@giraffate giraffate added the good-first-issue These issues are a good way to get started with Clippy label Apr 8, 2021
@SaadiSave
Copy link
Author

@giraffate
Correctness is described as code that is completely unnecessary. Don't the examples above have completely unnecessary code? What makes style a better category?

@giraffate
Copy link
Contributor

code that is outright wrong or very useless

correctness is described above in README. The original code isn't wrong and doesn't seems to lead some bugs. So I think style (default warn), not correctness (default deny), is appropriate for this.

@SaadiSave
Copy link
Author

Okay. Will change it

@SaadiSave
Copy link
Author

Can someone estimate how complex implementing this lint is?

@camsteffen
Copy link
Contributor

I think this should be correctness since it is "very useless". It could point out a bug where the coder meant to actually do something instead of nothing.

Name idea: nop_match.

Can someone estimate how complex implementing this lint is?

This should be quite easy to implement. You just need to check that, for each match arm, the body of the arm is the same as the pattern. Not quite as simple as a == b but some simple matches.

@giraffate
Copy link
Contributor

It could point out a bug where the coder meant to actually do something instead of nothing.

Oh, it make sense. correctness also looks good.

@SaadiSave
Copy link
Author

How do I go about implementing this?

@llogiq
Copy link
Contributor

llogiq commented Apr 30, 2021

If I'm not mistaken, correctness lints error instead of warn. We should really add the "suspicious" lint group we suggested earlier.

@camsteffen
Copy link
Contributor

@SaadiSave have you looked at the contributing docs?

@J-ZhengLi
Copy link
Member

Since no one has claim this in almost a year, so I'll give it a try.

@rustbot claim

bors added a commit that referenced this issue Mar 10, 2022
new lint that detects useless match expression

fixes #7040

changelog:
- Register new `correctness` lint called `nop_match`
- add basic test cases
- add logic to check for unnecessary match
- add logic to check for unnecessary if-let expressions, remove some test cases because checking them will take a lot extra effort

TODO:
- ~~detects unnecessary match~~
- ~~detects unnecessary if-let expression that similar to match~~
- modify `question_mark` and `manual_map` to solve possible confliction
@bors bors closed this as completed in 75b616e Mar 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants