new lint: and_then_then_some #12981

lolbinarycat · 2024-06-23T05:38:31Z

changelog: [and_then_then_some]: added

rustbot · 2024-06-23T05:38:35Z

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

lolbinarycat · 2024-06-23T07:11:19Z

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

lolbinarycat · 2024-06-23T08:16:54Z

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

clippy_lints/src/and_then_then_some.rs

J-ZhengLi

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

J-ZhengLi · 2024-06-27T03:22:43Z

clippy_lints/src/and_then_then_some.rs

+    match local.kind {
+        ExprKind::Path(QPath::Resolved(
+            _,
+            Path {
+                res: Res::Local(local_hid),
+                ..
+            },
+        )) => {


looks like clippy_utils::path_to_local, but I'm not sure if this can be replaced

i'm much more familiar with the hir and mir types than all the clippy utils, and yeah, you can't use function calls in patterns.

It could be extracted from the match expression then in the _ case you put that in the other branch. It'd be cleaner imo too

Actually, one step further, rather than doing an if let as I suggested above, do smth with Option::map_or_else then is_some_and(identity) since the true case is only when path_to_local would return Some.

lolbinarycat · 2024-06-27T06:56:40Z

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 · 2024-06-27T07:04:15Z

@rustbot review

Centri3

There's a few things I don't fully get, other than that just nits and lack of tests. Thanks! :3

clippy_lints/src/and_then_then_some.rs

Centri3 · 2024-06-27T07:28:39Z

clippy_lints/src/and_then_then_some.rs

+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        match expr.kind {
+            ExprKind::MethodCall(_, selfarg, [arg], _) | ExprKind::Call(_, [selfarg, arg]) => {
+                // TODO: check if type of reciever is diagnostic item Option?


Yeah this should check if the type is Option then check the name of the call

we're comparing the path later on, and the only way it could be a method call of one of Option's methods is if the receiver is Option, or if it's a function call that will fail typecheck, so checking the reciever type feels redundant.

originally i wanted it to still work even if the method was imported under a different name, but it doesn't seem like you can do that anyways.

It's not redundant. The reason why paths are discouraged more often than not is because they aren't as stable as diagnostic items, reexports are essentially stripped after the resolver so if Option is moved to, say, std::option_private::Option and reexported in Option, it'll no longer work.

Diagnostic item check + method name is the preferred way to do this

clippy_lints/src/and_then_then_some.rs

Centri3 · 2024-06-27T07:41:55Z

clippy_lints/src/and_then_then_some.rs

+    }
+}
+
+fn show_sugg(cx: &LateContext<'_>, span: Span, selfarg: &Expr<'_>, closure_args: Span, predicate: &Expr<'_>) {


Suggested change

fn show_sugg(cx: &LateContext<'_>, span: Span, selfarg: &Expr<'_>, closure_args: Span, predicate: &Expr<'_>) {

fn show_sugg(cx: &LateContext<'_>, span: Span, recv_or_self: &Expr<'_>, closure_args: Span, predicate: &Expr<'_>) {

Centri3 · 2024-06-27T07:42:52Z

clippy_lints/src/and_then_then_some.rs

+
+fn is_then_some(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    if let Some(def_id) = fn_def_id(cx, expr) {
+        match_def_path(cx, def_id, &["core", "bool", "<impl bool>", "then_some"])


Same here. Let's match the type's diagnostic item to bool then check the method name

Centri3 · 2024-06-27T07:43:26Z

tests/ui/and_then_then_some.rs

Would like some tests with external and procedural macros

This also needs to call in_external_macro and is_from_proc_macro. Make sure is_from_proc_macro is called last before linting

Also could do with some tests regarding your blocks handling, also if it's an explicit return instead

do you mean returning early if those functions are true? it seems like order wouldn't matter in that case, so obviously i'm missing something.

Early returning from in_external_macro. The reasoning behind calling is_from_proc_macro is because it's quite costly. We tend to call it last

Centri3 · 2024-06-27T07:48:05Z

clippy_lints/src/and_then_then_some.rs

+            // FIXME: this is the best way i could find to compare if
+            // a local refers to a specific closure argument.
+            //
+            // it breaks if the closure argument has an explicitly declared type,
+            // since the spans only align for TyKind::Infer


Issue: You shouldn't use the FnDecl for this. The Body contains more parameter info, mostly importantly the HirId

See Map::body, instead of getting the span of the body get the body itself :P

the documentation on hir::Body seems to suggest it only contains argument patterns when used with a function, not a closure?

It does for both cases. That documentation seems misleading tbh, I think they meant function as in a method call, function, or closure, since constant values wouldn't be included

Centri3 · 2024-06-27T08:31:45Z

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

J-ZhengLi · 2024-06-27T12:03:10Z

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

lolbinarycat · 2024-06-27T12:55:31Z

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).

Centri3 · 2024-06-28T03:14:19Z

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.

lolbinarycat · 2024-06-28T17:36:00Z

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

bors · 2024-07-04T05:52:48Z

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

Co-authored-by: John Arundel <[email protected]>

Co-authored-by: Catherine Flores <[email protected]>

lolbinarycat · 2024-07-05T17:38:23Z

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.

xFrednet · 2024-08-03T08:29:37Z

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

r? clippy

blyxyas · 2024-08-05T00:17:09Z

Rerolling these, I'm currently focused on our project's goal, see #13154

r? clippy

Manishearth · 2024-08-05T03:48:21Z

r? clippy

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

bors · 2024-08-28T08:08:48Z

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

blyxyas · 2024-09-09T22:14:08Z

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

lolbinarycat · 2024-09-09T23:42:52Z

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

rustbot assigned Centri3 Jun 23, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 23, 2024

bitfield reviewed Jun 23, 2024

View reviewed changes

clippy_lints/src/and_then_then_some.rs Outdated Show resolved Hide resolved

J-ZhengLi reviewed Jun 27, 2024

View reviewed changes

Centri3 reviewed Jun 27, 2024

View reviewed changes

Centri3 mentioned this pull request Jun 27, 2024

hir::Body's documentation incorrectly suggests it doesn't contain parmaeter info for closures rust-lang/rust#127031

Open

lolbinarycat and others added 13 commits July 5, 2024 12:08

WIP: add and_then_then_some lint

1d78783

saftey commit: it kinda works for the basic case

4bb4f13

handle another edge case

5b520b6

i think i have everything working

37e7058

cover most edge cases

3b9bba4

working ui test

7a0adc4

rustfix and rustfmt

88ec537

cleanup and refactor

5654d30

remove wildcard import

7ea79c0

fix typo

2a6f6d0

Co-authored-by: John Arundel <[email protected]>

add backticks around identifiers

360cf82

Co-authored-by: Catherine Flores <[email protected]>

adjust variable names

2ccce8d

Co-authored-by: Catherine Flores <[email protected]>

make comment more clear

b528cb7

Co-authored-by: Catherine Flores <[email protected]>

lolbinarycat and others added 10 commits July 5, 2024 12:08

reword documentation

da89528

Co-authored-by: Catherine Flores <[email protected]>

run autoformatting

8e9fa96

fix variable name

13c7361

saftey commit

2f343a0

fix another edge case and add more unit tests

4e8fe6c

add and test another edge case (explicit return)

1090186

autoformat

de9881f

remove unused imports

015822d

update test output

be39ee0

add checks for macros

016653f

lolbinarycat force-pushed the and_then_then_some branch from 8bd91d7 to 016653f Compare July 5, 2024 16:12

WIP: add another check to prevent false positives

6c0607d

lolbinarycat added 4 commits July 5, 2024 13:48

add more checks

f03e8d6

add extra unit test for the Copy case

bb0e265

run autoformatting

c842e4b

remove glob import

122cd11

rustbot assigned blyxyas and unassigned Centri3 Aug 3, 2024

rustbot assigned Manishearth and unassigned blyxyas Aug 5, 2024

rustbot assigned flip1995 and unassigned Manishearth Aug 5, 2024

	fn show_sugg(cx: &LateContext<'_>, span: Span, selfarg: &Expr<'_>, closure_args: Span, predicate: &Expr<'_>) {
	fn show_sugg(cx: &LateContext<'_>, span: Span, recv_or_self: &Expr<'_>, closure_args: Span, predicate: &Expr<'_>) {

new lint: and_then_then_some #12981

Are you sure you want to change the base?

new lint: and_then_then_some #12981

Conversation

lolbinarycat commented Jun 23, 2024

rustbot commented Jun 23, 2024

lolbinarycat commented Jun 23, 2024

lolbinarycat commented Jun 23, 2024

J-ZhengLi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolbinarycat commented Jun 27, 2024 • edited Loading

lolbinarycat commented Jun 27, 2024

Centri3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centri3 Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Centri3 commented Jun 27, 2024

J-ZhengLi commented Jun 27, 2024

lolbinarycat commented Jun 27, 2024

Centri3 commented Jun 28, 2024 • edited Loading

lolbinarycat commented Jun 28, 2024

bors commented Jul 4, 2024

lolbinarycat commented Jul 5, 2024

xFrednet commented Aug 3, 2024

blyxyas commented Aug 5, 2024

Manishearth commented Aug 5, 2024

bors commented Aug 28, 2024

blyxyas commented Sep 9, 2024

lolbinarycat commented Sep 9, 2024

lolbinarycat commented Jun 27, 2024 •

edited

Loading

Centri3 Jun 27, 2024 •

edited

Loading

Centri3 commented Jun 28, 2024 •

edited

Loading