add [manual_find] lint for function return case

[ebobrow]

part of the implementation discussed in #7143

changelog: add [manual_find] lint for function return case

[rust-highfive]

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

[xFrednet]

r? @Alexendoo (I'll keep @flip1995 assigned, as either one of us might do the final review 🙃)

[Alexendoo]

Hi, thanks for the PR!

So far I've mostly been looking at the pattern/ident stuff, I think it could do with being more conservative to deal with all the wild patterns out there, say for where PAT binds binding

for PAT in input {
    if COND {
        return Some(binding);
    }
    None
}

default to the assumption we're going to suggest

input.[into_iter/iter/etc]().map(|PAT| binding).find(|&binding| COND)

Then add more focused variants, like if PAT is already a PatKind::Binding omit the .map(), or if it's a single ref to a binding use .copied() instead

[xFrednet]

Hey @ebobrow, this is a ping from triage, can take a look at the review comments and address them? 🙃

@rustbot author

[ebobrow]

Hey, sorry about that! I've been really busy lately but I'll see if I can work on it this weekend.

[ebobrow]

Thanks for the review @Alexendoo and apologies for the delay! I think this should address your comments.

[ebobrow]

Thanks for the review! This should address your comments.

[Alexendoo]

I have found quite the spanner to throw in the works:

fn vec_string(strings: Vec<String>) -> Option<String> {
    for s in strings {
        if s == String::new() {
            return Some(s);
        }
    }
    None
}

both |s| and |&s| would result in errors:

error[E0277]: can't compare `&std::string::String` with `std::string::String`
  --> t.rs:25:36
   |
25 |     strings.into_iter().find(|s| s == String::new())
   |                                    ^^ no implementation for `&std::string::String == std::string::String`
   |
   = help: the trait `std::cmp::PartialEq<std::string::String>` is not implemented for `&std::string::String`

error[E0507]: cannot move out of a shared reference
  --> t.rs:25:31
   |
25 |     strings.into_iter().find(|&s| s == String::new())
   |                               ^-
   |                               ||
   |                               |data moved here
   |                               |move occurs because `s` has type `std::string::String`, which does not implement the `Copy` trait
   |                               help: consider removing the `&`: `s`

I believe the only way to create a correct suggestion here would be

strings.into_iter().find(|s| *s == String::new())

[Alexendoo]

One thing that you could do is check if inner_ret is Copy, in that case you can add the extra borrow unconditionally and the body of find can be unchanged

For the non Copy case it's more tricky, I think turning every use of ident into (*ident) would be correct, though it would be a fair bit of work creating such a snippet. I'd need to think a bit longer on the cases where you can leave it as ident

Another option would be to have an Applicability::HasPlaceholders suggestion for the non-Copy case, e.g. for the strings case suggesting

strings.into_iter().find(|s| { ... })

[ebobrow]

I went with the Applicability::HasPlaceholders implementation for now, but I can change it to dereference each ident. The only way I can think to do that would be to walk the tree and add partial snippets with asterisks and parentheses in between.

As for the cases that don't have to be dereferenced, all I can think of so far is a method called on ident or maybe if PartialEq is implemented for a struct and a reference to the struct (so we can call &Data(1) == Data(1)). I'm probably missing some.

[flip1995]

Thanks for the review @Alexendoo!

I left some more comments. Their mostly about how to improve the suggestion a bit further and making the code more maintainable by adding a few more comments.

[ebobrow]

Thanks for the review @flip1995! I've applied your suggestions. I didn't end up using strip_return because I already had to do most of the pattern matching there to check if the last statement was a return, but let me know if I'm missing something and should use it.

[flip1995]

Thanks for addressing this so quickly! I have one more question left.

[flip1995]

Why can't you do this in the find(|(x, _)| ...) closure?

[ebobrow]

If we did that, the function would return the tuple instead of just the x. In the imperative version, the code would look something like

for (x, _) in arr {
    if /* ... */ {
        return Some(x);
    }
}

so it only returns the x and ignores the rest of the pattern, but in a find the elements in the iterator remain as the entire tuple.

[ebobrow]

Hey @flip1995, just checking in! Are you going to review this or have you been waiting for me to do something? (Apologies if so!)

[flip1995]

I think you answered my question. I'll have another look on the PR soon and will then merge it. I don't think there's anything left for you to do.

Sorry for taking so long. My real life is a bit busy recently, so open source suffers... :/

[ebobrow]

No worries! I'm in no rush, just wanted to make sure this hadn't been lost.

[flip1995]

Thanks for the patience again!

@bors r+

[bors]

📌 Commit 430575b has been approved by flip1995

[bors]

⌛ Testing commit 430575b with merge fdd0e72...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing fdd0e72 to master...