add lint manual_unwrap_or

[tnielens]

Implements partially #5923.

changelog: add lint manual_unwrap_or

[rust-highfive]

r? @matthiaskrgr

(rust_highfive has picked a reviewer for you, use r? to override)

[ebroto]

Left some thoughts

[tnielens]

@ebroto all your remarks should be addressed.

[ebroto]

Just some very minor nits. Also we should rebase to get rid of the merge commits.

[tnielens]

Last comments were addressed.

[ebroto]

So, I've been playing with the lint a bit and I think I found a problematic case.

The unwrap_or/else methods take self by value, while match does not need to move the scrutinee. If the scrutinee is mutable and is modified in the None arm, after applying the lint we get a use-after-move error:

struct NonCopyable;

fn main() {
    let mut option: Option<NonCopyable> = None;
    match option {
        Some(x) => x,
        None => {
            option = Some(NonCopyable);
            // some more code ...
            option.unwrap()
        },
    };
}

(playground)

is turned into:

struct NonCopyable;

fn main() {
    let mut option: Option<NonCopyable> = None;
    option.unwrap_or_else(|| {
        option = Some(NonCopyable);
        // some more code ...
        option.unwrap()
    });
}

(playground)

I see at least two options:

1. We check that the T in Option<T> is Copy, otherwise we bail out
2. We check that the None arm is not modifying the scrutinee.

I'm more hesitant about making the lint enabled by default for now, I think we might be missing other stuff when turning an arm value into a closure. What do you think?

[tnielens]

Great catch! @ebroto

Here is another problem related to ownership:

fn main() {
    let option: Option<&str> = None;
    match option {
        Some(s) => s,
        None => &format!("{} {}!", "hello", "world"),
    };
    
    option.unwrap_or_else(|| {
        // compilation error here, it can't return a reference to a value owned by the closure body
        &format!("{} {}!", "hello", "world")
    });
}

Indeed, the risk of hitting an ownership bug is high no matter what exception rules we put in place.
Should we stick to constant none arms only and only suggest the unwrap_or variant?

[ebroto]

Oh, right, that one is more probable :)

Indeed, let's be safe and lint only the unwrap_or case 👍

[tnielens]

The unwrap_or_else suggestions are removed.

[ebroto]

@bors r+

Thanks!

[bors]

📌 Commit 690a6a6 has been approved by ebroto

[bors]

⌛ Testing commit 690a6a6 with merge 4e83a38...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 4e83a38 to master...