rust-clippy icon indicating copy to clipboard operation
rust-clippy copied to clipboard

Published 20 hours ago • verified publisher icon rust-lang

new lint: and_then_then_some

Open lolbinarycat opened this issue 1 year ago • 16 comments

fixes #12978

changelog: [and_then_then_some]: added

lolbinarycat avatar Jun 23 '24 05:06 lolbinarycat

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Centri3 (or someone else) some time within the next two weeks.

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

rustbot avatar Jun 23 '24 05:06 rustbot

unable to reproduce formatting errors locally, cargo dev fmt --check runs fine

lolbinarycat avatar Jun 23 '24 07:06 lolbinarycat

ok apparently it just started working again, no idea why that happened.

lolbinarycat avatar Jun 23 '24 08:06 lolbinarycat

Maybe this could be merge to the existing lint clippy::manual_filter?

that lint isn't machine applicable. additionally, it's not a nursury lint.

lolbinarycat avatar Jun 27 '24 06:06 lolbinarycat

@rustbot review

lolbinarycat avatar Jun 27 '24 07:06 lolbinarycat

Maybe this could be merge to the existing lint clippy::manual_filter?

that lint isn't machine applicable. additionally, it's not a nursury lint.

Two suggestions can have differing applicability. Also imo this shouldn't be nursery, it seems completely fine once the issue regarding FnDecl is resolved

Centri3 avatar Jun 27 '24 08:06 Centri3

ah just got back from work, looks like a lot discussion has happened so I'm gonna clear something really quick then throw myself out :D

that lint isn't machine applicable.

I think it is? The applicability was initialized as MachineApplicable in manual_utils.rs, later passed to manual_filter via SuggInfo. You may keep this lint as a separated one consider how complicated the other one looks, plus this one checks different thing anyway.

But just keep in mind about putting lints in 'nursery', they would receive less testing and not being as useful as the other lints.

I've ran lintcheck on top 200 popular crate with this lint, unfortunately I didn't see any thing related, putting it in 'nursery' is just further limiting it's useages, so agree with Centri3 that this shouldn't be a 'nursery' lint, at least for now

J-ZhengLi avatar Jun 27 '24 12:06 J-ZhengLi

I've ran lintcheck on top 200 popular crate with this lint, unfortunately I didn't see any thing related, putting it in 'nursery' is just further limiting it's useages, so agree with Centri3 that this shouldn't be a 'nursery' lint, at least for now

the problem is false positives, since the pre and post transformation code actually has different signatures (goes from by value to by reference).

lolbinarycat avatar Jun 27 '24 12:06 lolbinarycat

Hmm I see. we can fix that by adding a & to filter's signature if it's Copy, check clippy_utils::ty::is_copy. Otherwise it should be non-applicable if it's !Copy yet warn-by-default

Also, if the original type is a reference then it's Copy automatically so that's another case that's very nicely handled.

Centri3 avatar Jun 28 '24 03:06 Centri3

also, if the argument variable only appears in autoreferencing method calls (like in the current tests) it should be possible.

lolbinarycat avatar Jun 28 '24 17:06 lolbinarycat

:umbrella: The latest upstream changes (presumably #12873) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Jul 04 '24 05:07 bors

Hmm I see. we can fix that by adding a & to filter's signature if it's Copy, check clippy_utils::ty::is_copy. Otherwise it should be non-applicable if it's !Copy yet warn-by-default

Also, if the original type is a reference then it's Copy automatically so that's another case that's very nicely handled.

I think this false positive I'm worried about might actually be mathematically impossible, since it requires a non-copy value to be moved in the predicate, then moved into the argument of then_some, which would fail borrow checking.

lolbinarycat avatar Jul 05 '24 17:07 lolbinarycat

Hey, this is triage. It looks like @Centri3 is currently busy. Let's pick another reviewer:

r? clippy

xFrednet avatar Aug 03 '24 08:08 xFrednet

Rerolling these, I'm currently focused on our project's goal, see https://github.com/rust-lang/rust-clippy/pull/13154

r? clippy

blyxyas avatar Aug 05 '24 00:08 blyxyas

r? clippy

rerolling again, I'm wrapping up vacation and will be on airplanes for a while

Manishearth avatar Aug 05 '24 03:08 Manishearth

:umbrella: The latest upstream changes (presumably #11476) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Aug 28 '24 08:08 bors

I'll get to review this, seems that it's been kinda forgotten by the team. We should have a review soon.

blyxyas avatar Sep 09 '24 22:09 blyxyas

i've kinda abandoned it since it seems like there might already be a lint for this?

lolbinarycat avatar Sep 09 '24 23:09 lolbinarycat

:umbrella: The latest upstream changes (possibly d28d2344d000aa96bef729cf408731f952f71fb0) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot avatar Mar 31 '25 23:03 rustbot

Ping @lolbinarycat from triage. I think this is waiting on a review, but do you plan to come back to working on this?

Jarcho avatar Sep 16 '25 21:09 Jarcho