Lint manual_unwrap_or for it let cases

[lochetti]

This PR modifies manual_unwrap_or to lint for if let cases as well. This effort is part of the fixes desired by #12618

changelog:[manual_unwrap_or]: Lint for if let cases.

[rustbot]

r? @Manishearth

rustbot has assigned @Manishearth.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Manishearth]

r? @y21

might not get time to get around to this in the next few days

[y21]

Do these two lints manual_unwrap_or and manual_unwrap_or_default have overlapping suggestions now or why did it start emitting warnings here?

[lochetti]

@y21 we have this test, that is the one that started to emit the manual_unwrap_or

// Issue #12564  
// No error as &Vec<_> doesn't implement std::default::Default
let mut map = std::collections::HashMap::from([(0, vec![0; 3]), (1, vec![1; 3]), (2, vec![2])]);
let x: &[_] = if let Some(x) = map.get(&0) { x } else { &[] };

Until before this PR it didn't emit the manual_unwrap_or because we didn't have manual_unwrap_or implemented for if let.

But, I think that we already have this problem of emiting both lints for some cases. If we go to master and simply try this code

    let x: Option<i32> = None;
    match x {
        Some(v) => v,
        None => 0,
    };

it will emit both lints. And this is also reaffirmed by the fact that all test cases in tests/ui/manual_unwrap_or_default.rs are not using i32 and 0 for instance, they are using String and String::new(), for instance.

So it look likes we already have a problem right?
If so, and in case we want to avoid linting both lints, maybe we could not lint the manual_unwrap_or if the constant value is the default? But what if the developer had avoided manual_unwrap_or_default?

[y21]

...Ah, okay I see it now. I think the ideal solution would be to unify these two lints and have them live in the same file, because as far as I understand, the only difference between these two lints is the kind of expression in the else case ("if it's equivalent to a default() call, lint manual_unwrap_or_default, otherwise manual_unwrap_or"), all of the additional lint logic seems like it should be exactly the same.

But since this was already an issue before this PR and this is just a side effect of this linting more cases, it doesn't need to be done here in this PR.

But what if the developer had avoided manual_unwrap_or_default?

That's a fair point. We do have the function is_lint_allowed for checking if a lint is #[allow]ed. That would allow us to see if the user explicitly disabled it, and we could still emit this I think.

But as mentioned above, it doesn't really need to be done in this pr.

[lochetti]

Ok! Understood.
After we merge this PR, I will create a new issue for this "merge" of both lints at the same file :)

[y21]

This should check that BindingAnnotation::NONE is used, i.e. avoid linting on an Some(ref x) => x arm because it would otherwise lead to a type error (would need .as_ref())

[y21]

ah, I now realise that this code was just moved around and isn't actually new code.

[lochetti]

And here, I tried to understand, but I also don't know if I need to to anything else 😬

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

• a95824d

[y21]

LGTM, thanks! The other two comments were issues that already existed so I don't think we really need to block this PR on that, it's a good improvement on its own. (I'll get to them if nobody else does)

@bors r+

[bors]

📌 Commit 70ca9a1 has been approved by y21

It is now in the queue for this repository.

[bors]

⌛ Testing commit 70ca9a1 with merge 9e54ff2...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing 9e54ff2 to master...