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

FN single_match_else #3489

Closed
matthiaskrgr opened this issue Dec 3, 2018 · 3 comments · Fixed by #5771
Closed

FN single_match_else #3489

matthiaskrgr opened this issue Dec 3, 2018 · 3 comments · Fixed by #5771
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@matthiaskrgr
Copy link
Member

clippy 0.0.212 (b2601be 2018-11-27)

I had this code which did not trigger any lints:

    let input = match directory {
        Some(value) => value,
        None => {
            return Err((
                ErrorKind::RemoveDirNoArg,
                "No argument assigned to --remove-dir, example: 'git-repos,registry-sources'"
                    .to_string(),
            ))
        }
    };

After applying rustfmt, an additional ; was added:

    let input = match directory {
        Some(value) => value,
        None => {
            return Err((
                ErrorKind::RemoveDirNoArg,
                "No argument assigned to --remove-dir, example: 'git-repos,registry-sources'"
                    .to_string(),
            ));                                   // <--------- HERE
        }
    };

which caused clippy::single_match_else to trigger.

warning: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
   --> src/library.rs:432:17
    |
432 |       let input = match directory {
    |  _________________^
433 | |         Some(value) => value,
434 | |         None => {
435 | |             return Err((
...   |
440 | |         }
441 | |     };
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
help: try this
    |
432 |     let input = if let Some(value) = directory { value } else {
433 |     return Err((
434 |         ErrorKind::RemoveDirNoArg,
435 |         "No argument assigned to --remove-dir, example: 'git-repos,registry-sources'"
436 |             .to_string(),
437 |     ));
  ...

I think the link should also trigger on the unformatted example!

@phansch phansch added the C-bug Category: Clippy is not doing the correct thing label Dec 5, 2018
@ghost
Copy link

ghost commented Jan 8, 2019

I also prefer using ok_or_else and ? to the suggestion.

    let input = directory.ok_or_else(|| Err(
        ErrorKind::RemoveDirNoArg,
        "No argument assigned to --remove-dir, example: 'git-repos,registry-sources'".to_string(),
    ))?;

@ghost
Copy link

ghost commented Jan 9, 2019

Looking at this again

    match Some(1) => {
        Some(_) => {...}
        None => { return 1 },
    }

is equivalent to

    match Some(1) => {
        Some(_) => {...}
        None => return 1,
    }

which is a common pattern and shouldn't be linted IMO.

If anything is changed, I think clippy shouldn't lint for the second pattern either.

@tnielens
Copy link
Contributor

tnielens commented Jul 5, 2020

This lint currently doesn't lint cases where the "else" part of the 2 arm match is a single statement/expression. The bug lies in that { return } is considered as a single expression whereas { return; } isn't.

One approach would be to consider single statement blocks equivalently. That would join @mikerite's last comment. That is, the three following examples wouldn't be linted.

    match Some(1) => {
        Some(_) => {...}
        None => return 1,
    }
    match Some(1) => {
        Some(_) => {...}
        None => { return 1 },
    }

// this case is currently linted

    match Some(1) => {
        Some(_) => {...}
        None => { return 1; },
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants