Don't lint manual_let_else in cases where ? would work #10924

est31 · 2023-06-11T04:43:47Z

Don't lint manual_let_else where the question mark operator ? would be sufficient, that is, mostly in cases like:

let v = if let Some(v) = ex { v } else { return None };

Also, this PR emits the question_mark lint for let...else patterns that could be written with ? (also, only return None like cases).

changelog: [`manual_let_else`]: don't lint in cases where question_mark already lints
changelog: [`question_mark`]: lint for `let Some(...) = ex else { return None };`

Fixes #8755

rustbot · 2023-06-11T04:43:51Z

r? @Manishearth

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

Manishearth · 2023-06-11T09:39:59Z

additional r? @Centri3

rustbot · 2023-06-11T09:40:01Z

Failed to set assignee to Centri3: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

Centri3 · 2023-06-11T09:41:38Z

Sure, will take a look in a bit!

Centri3

This looks good to me, great additions to the tests. Can you move pat_and_expr_can_be_question_mark to clippy_utils? Thanks!

Centri3 · 2023-06-11T09:59:53Z

clippy_lints/src/manual_let_else.rs

+/// We output `Some(a)` in the first instance, and `Some(FooBar { a, b })` in the second, because
+/// the question mark operator is applicable here. Callers have to check whether we are in a
+/// constant or not.
+pub fn pat_and_expr_can_be_question_mark<'a, 'hir>(


This should go in clippy_utils imo.

It feels a little bit specific to question mark/manual_let_else to me. Should I still move it?

I mean, the reason why I put it into a separate function and why it's shared by question_mark and manual_let_else is to make sure that there is definitely no overlap between the two. I don't think it's something that's supposed to be generally usable/useful, compared to say a function that checks if a pattern is refutable or not.

As it stands currently it's definitely only useful for those two lints, but this sort of stuff should go in clippy_utils regardless. In this case, you give a Pat and an Expr and get whether it can be represented as a ?; that, imo, is general enough for it. Even if it's never really used elsewhere it could potentially save someone the effort of reimplementing it, then getting told it was already implemented in another lint

Done, I've moved it.

Manishearth · 2023-06-12T04:15:03Z

clippy_lints/src/manual_let_else.rs

+    else_body: &Expr<'_>,
+) -> Option<&'a Pat<'hir>> {
+    if let PatKind::TupleStruct(pat_path, [inner_pat], _) = pat.kind &&
+        is_res_lang_ctor(cx, cx.qpath_res(&pat_path, pat.hir_id), OptionSome) &&


suggestion: probably could also handle Result here?

Result has the issue that ? does Err conversion while the else block has no access to the err at all. So it is required to come up with something else, say either some manual bail!() invocation, or constructing the Err(Something) manually. That's why I think it has to be restricted to options.

makes sense

Hmm thinking of it, one could lint for cases like:

let res = something_that_returns_result(); let Ok(val) = res else { return res };

or

let res = something_that_returns_result(); let Ok(val) = res else { return Err(res.unwrap_err()) };

It's I think way more rare to encounter that though.

yeah that makes sense

flip1995 · 2023-06-12T07:54:31Z

clippy_lints/src/manual_let_else.rs

+/// We output `Some(a)` in the first instance, and `Some(FooBar { a, b })` in the second, because
+/// the question mark operator is applicable here. Callers have to check whether we are in a
+/// constant or not.
+pub fn pat_and_expr_can_be_question_mark<'a, 'hir>(


The linting logic for let ... else -> ? is now called twice for 2 different lints. Usually we handle this by running those "conflicting" lints in the same pass and only check for the superseded lint, if the other lint does not trigger. You can see a lot of examples of that in the methods module.

Do you think merging those two files into on pass and then applying the same approach would work for those lints?

The question_mark lint is implemented right now as a visitor on expressions, while the manual_let_else lint looks for statements. It makes a lot of sense for both to do it this way, because let v = if let Some(v) = ex { v } else { return None }; is a statement while for question_mark it looks for stuff like if let Some(v) = ex { v } else { return None }, which is an expression, and it doesn't matter if that expression is put into a local or is the start of a method chain, directly a function parameter, etc.

So while it's possible to merge the two lints into one pass, likely that would just similarly call the same function two times, just once through a check on statements, once through a check on expressions.

The only way I could think of unifying is that one changes the manual_let_else lint to look for expressions, and then make it obtain the parent node, and if the parent node is a let statement, it would continue from there.

Do you think this is preferable to having two passes?

I would do this way simpler:

fn check_let_some_else_return_none(..) -> bool

should return a bool whether the lint was triggered.

in the check_stmt part of the unified pass, you would then write the code like this:

if !check_let_some_else_return_none(...) { check_manual_let_else(...); }

And in check_manual_let_else you put the code that currently is in the LateLintPass impl of check_stmt of the manual_let_else lint.

Maybe it makes sense to include those lints in the matches module. But I'm not 100% convinced if this is the best location for it.

The check_let_some_else_return_none function does only check for let...else, while the check_manual_let_else function checks for let foo = if let () ... patterns (and the match equivalent). Therefore, there is no overlap here. The overlap with the question mark lint would happen with the <QuestionMark as LateLintPass>::check_expr function, which takes an expression instead of a statement.

I have pushed a commit to put the two things into one pass. I feel it's better without this merging, but maybe you like it more than I do.

bors · 2023-06-28T18:59:25Z

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

…would work Also, lint question_mark for `let...else` clauses that can be simplified to use `?`. This lint isn't perfect as it doesn't support the unstable try blocks.

Manishearth · 2023-07-01T19:55:15Z

@bors delegate=centri3

(please approve this PR when ready)

bors · 2023-07-01T19:55:17Z

✌️ @Centri3, you can now approve this pull request!

If @Manishearth told you to "r=me" after making some further change, please make that change, then do @bors r=@Manishearth

Centri3

I'm happy with this version, just one minor nit

clippy_lints/src/manual_let_else.rs

If question_mark is allowed, there is no overlap any more, so we can just not suppress it.

Centri3 · 2023-07-03T14:18:25Z

I think this is done, I took another look and couldn't see anything. thanks!

@bors r=Centri3,flip1995,Manishearth

bors · 2023-07-03T14:18:27Z

📌 Commit d80581c has been approved by Centri3,flip1995,Manishearth

It is now in the queue for this repository.

bors · 2023-07-03T14:18:34Z

⌛ Testing commit d80581c with merge 3f17c5c...

bors · 2023-07-03T14:33:48Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3,flip1995,Manishearth
Pushing 3f17c5c to master...

est31 · 2023-07-03T14:35:32Z

@Centri3 @Manishearth thanks for the review!

rustbot assigned Manishearth Jun 11, 2023

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

est31 force-pushed the manual_let_else_question_mark branch 2 times, most recently from 6b657ce to 37ad84d Compare June 11, 2023 05:51

Centri3 reviewed Jun 11, 2023

View reviewed changes

Manishearth reviewed Jun 12, 2023

View reviewed changes

flip1995 reviewed Jun 12, 2023

View reviewed changes

est31 added 2 commits June 30, 2023 18:01

Don't lint manual_let_else in cases where the question mark operator …

86391ab

…would work Also, lint question_mark for `let...else` clauses that can be simplified to use `?`. This lint isn't perfect as it doesn't support the unstable try blocks.

Fix the now stricter lint in manual_rem_euclid.rs

c6be621

est31 force-pushed the manual_let_else_question_mark branch from 37ad84d to c6be621 Compare June 30, 2023 16:01

Put into one pass

20dfaba

est31 force-pushed the manual_let_else_question_mark branch from e37727a to 20dfaba Compare June 30, 2023 17:47

Centri3 approved these changes Jul 1, 2023

View reviewed changes

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

est31 force-pushed the manual_let_else_question_mark branch from 7086172 to 4dd78d3 Compare July 2, 2023 02:29

est31 added 2 commits July 3, 2023 09:42

Don't suppress manual_let_else if question_mark is allowed

6990eaa

If question_mark is allowed, there is no overlap any more, so we can just not suppress it.

Move pat_and_expr_can_be_question_mark into clippy_utils

d80581c

est31 force-pushed the manual_let_else_question_mark branch from 4dd78d3 to d80581c Compare July 3, 2023 07:43

bors merged commit 3f17c5c into rust-lang:master Jul 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't lint manual_let_else in cases where ? would work #10924

Don't lint manual_let_else in cases where ? would work #10924

est31 commented Jun 11, 2023

rustbot commented Jun 11, 2023

Manishearth commented Jun 11, 2023

rustbot commented Jun 11, 2023

Centri3 commented Jun 11, 2023

Centri3 left a comment

Centri3 Jun 11, 2023

Manishearth Jun 12, 2023

est31 Jun 30, 2023

est31 Jun 30, 2023

Centri3 Jul 1, 2023

est31 Jul 2, 2023

Manishearth Jun 12, 2023

est31 Jun 12, 2023

Manishearth Jun 12, 2023

est31 Jun 12, 2023

Manishearth Jun 12, 2023

flip1995 Jun 12, 2023

Manishearth Jun 12, 2023

est31 Jun 12, 2023

flip1995 Jun 12, 2023

est31 Jun 30, 2023

est31 Jun 30, 2023

bors commented Jun 28, 2023

Manishearth commented Jul 1, 2023

bors commented Jul 1, 2023

Centri3 left a comment

Centri3 commented Jul 3, 2023

bors commented Jul 3, 2023

bors commented Jul 3, 2023

bors commented Jul 3, 2023

est31 commented Jul 3, 2023

Don't lint manual_let_else in cases where ? would work #10924

Don't lint manual_let_else in cases where ? would work #10924

Conversation

est31 commented Jun 11, 2023

rustbot commented Jun 11, 2023

Manishearth commented Jun 11, 2023

rustbot commented Jun 11, 2023

Centri3 commented Jun 11, 2023

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

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

bors commented Jun 28, 2023

Manishearth commented Jul 1, 2023

bors commented Jul 1, 2023

Centri3 left a comment

Choose a reason for hiding this comment

Centri3 commented Jul 3, 2023

bors commented Jul 3, 2023

bors commented Jul 3, 2023

bors commented Jul 3, 2023

est31 commented Jul 3, 2023