Add lint for unused_result_ok

[ithinuel]

This PR adds a lint to capture the use of expr.ok(); when the result is not really used.

This could be interpreted as the result being checked (like it is with unwrap() or expect) but
it actually only ignores the result.

let _ = expr; expresses that intent better.

This was also mentionned in #8994 (although not being the main topic of that issue).

changelog: [misleading_use_of_ok]: Add new lint to capture .ok(); when the result is not really used.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[y21]

What does the trimming here do?

[ithinuel]

I'm not 100% sure.
I used matching_result_ok as an example.

IIUC it removes the extra spaces on cases like line 14 of the UI test sample.

[y21]

🤔 That's odd, I'm not sure what's going on in match_result_ok. The span of the receiver of the .ok() method call should always be

    x   .   parse::<i32>()   .   ok   ();
    ^^^^^^^^^^^^^^^^^^^^^^

I tried removing the trimming in both this lint and in match_result_ok and in both cases it seems to pass all tests just fine.
@flip1995 seeing that you reviewed #5032, which is where that was introduced, do you know why this was necessary?
Specifically referring to this line

[flip1995]

No idea anymore. My suspicion is, that back then I/we cared more about well formatted suggestions. Nowadays, we don't really care about that so much, as we assume that everyone uses rustfmt anyway and Clippy suggestion formatting is not important.

But that is just a wild guess.

[ithinuel]

Is there any compatibility requirement with previous version of the compiler?

As far as I understand, _ = is a destructuring assignment introduced in 1.59.

[flip1995]

Do you mean in the Clippy codebase or in the suggestions made by Clippy?

In the former case: no

In the latter case: Yes. If you want to add a suggestion that suggests _ = ..., it should probably be gated by an MSRV check for 1.59.0 where that feature was added. There's a lot of examples in the code of how to do that, should it be required here.

[ithinuel]

I meant the suggestion. So yes, let’s keep it as let _ = ….
I’ll let you resolve this conversation if it the required changes have been met.

[ithinuel]

I discovered today that _ = … is a valid alternative to let _ = ….
Personally, I tend to prefer the lighter _ = …. However, I can see that the current code base, particularly the UI tests, heavily uses let _ = ….
I’m open to either approach, so please let me know if there is a preferred option.

[dswij]

FME, let _ = ... is a much more popular approach. But again, it might be only on the codebases that I've seen, and I don't have real data to back it up. IMO, It's fine to use let _ = ... for now. wdyt? @ithinuel

[ithinuel]

I’ll keep it as let _ = .

I don’t think having a different recommendation based on the compiler version in used is a great things. The noise caused would outweigh the subjective readability benefits.

[y21]

I agree with @dswij, and FWIW, the rustc lint unused_must_use also suggests let _ = ..., so having this lint use it as well sounds good?
IMO the lint name is a bit unclear about what exactly it's looking for. What about unused_result_ok? That better captures the pattern and makes it easier if in the future we want more lints for other misleading uses of .ok(). It's also similar to match_result_ok.

[ithinuel]

@y21 & @flip1995 is there anything blocking the PR from making it through?

[y21]

Sorry for the long wait. Implementation LGTM, I just have some small documentation nits, then IMO it's merge ready? @dswij do you have something else, since you're assigned?

[y21]

We could also just add #[must_use] to the ok method in the standard library and that would cause unused_must_use to start linting on .ok(); but that might be too opinionated for unused_must_use.
I could see that leading to people who disagree with it to allow unused_must_use entirely, which would be worse because they'd miss warnings for definitely-wrong code, like string.trim(); being a no-op.

So, probably better to have it as a clippy lint instead.

[Jarcho]

This definitely looks like a case for #[must_use]. An unused result is either an obfuscated drop (x.ok(); will drop x), or a useless method call (foo().ok() does nothing foo() doesn't). In both cases there's a more idiomatic form to use to accomplish the goal.

[xFrednet]

Hey @y21 and @Jarcho, it looks like this PR has been approved, but there was a following discussion. Could you take a look at it?

@rustbot ready

[y21]

It probably makes sense to open a discussion for this on zulip. I view rustc's unused_must_use lint almost as a correctness lint since it's typically applied to things where it really doesn't make any sense to ignore it and it must be a bug (for example, ignoring futures without doing something with them is a no-op, calling .trim() on a &str without using it is a no-op), whereas using .ok() on a Result to suppress the lint is more of a style thing that people might disagree with. To give one concrete example, the dotenv crate has dotenv().ok(); in its example on the readme.

I think adding #[must_use] to ok would also lead to the suggestion let _ = result.ok(); when it could have just been let _ = result;, so a clippy lint allows for better diagnostics, though #[must_use] allows providing a message so that could be easily mitigated.

[J-ZhengLi]

Well, it has been another 2 months~

so, pinging the most recent people who replies (how you don't mind): @y21 @xFrednet, anyone wanna have the privilege to open a discussion/FCP on zulip ? 😄

At the mean time, @ithinuel, sorry for the long wait, do you mind applying those suggestions made by y21 on February?

[xFrednet]

I can give this another look, if @y21 doesn't get to it during the weekend :)

[y21]

Sorry, missed this. Implementation wise this all still looks good to me. There's still the question if we want this in the first place.
FCP: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/FCP.3A.20.60unused_result_ok.60

[Alexendoo]

See also rust-lang/rust#66116

[xFrednet]

Hey, this is a ping from triage. @y21 I think the FCP period should be done. Can you summarize the results and move this forward?

@rustbot ready

[y21]

Looks like people are generally on board with having this be a clippy lint, based on the FCP thread. The discussion on the issue that Alexendoo linked also shows that it is a pattern that people write and would probably be too strong for rustc to warn on as part of unused_must_use for now, and it's also been tried before in rust-lang/rust#79006 and got rejected.

@flip1995 on Zulip even went as far as wanting this in the restriction category (ie allow-by-default, not style) because it's so common. I think I can get behind this, because it looks like people are still figuring out what the ideal solution for this would be and there doesn't appear to be a very clear answer on what is generally considered more idiomatic to the point where we should start pushing it onto people, but ultimately I don't have a strong preference here.
I'll post a poll soon just to get more opinions on the category

(There's also the concern that this lint will need to be deprecated when that method gets marked as #[must_use] but I don't think it's worth blocking it over that -- we had to do the same just recently with maybe_misused_cfg when check-cfg got stabilized)

[y21]

Alright, the poll didn't bring in a lot more opinions but we did get another vote for restriction, so that sounds like we should go with that.
@ithinuel could you make this last change? I believe that was the last "concern" here and should be ready to go with the category changed. (And also squashing the commits would be nice)

[ithinuel]

That should do the trick (I also rebased on top of master).

[y21]

There's still a bunch of commits that don't seem meaningful outside of this PR (eg "apply review feedback", "remove trim", "rename and add tests", ...). Can you squash those also?

[y21]

Also @dswij , I forgot that you're still also assigned to this PR. Did you have anything else?

[ithinuel]

@y21, I squashed the commits together and kept the relevant commit title.

[y21]

I think @dswij is busy, so I'm going to just go ahead and skip that to avoid having to block this for even longer.

Thank you for the effort you put into this @ithinuel !

@bors r+

[bors]

📌 Commit 182c268 has been approved by y21

It is now in the queue for this repository.

[bors]

⌛ Testing commit 182c268 with merge 5ead90f...

[bors]

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