Simplify if let statements #8356

J-ZhengLi · 2022-01-27T08:12:00Z

changelog: Allowing [qustion_mark] lint to check if let expressions that immediatly return unwrapped value

rust-highfive · 2022-01-27T08:12:03Z

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

Please see the contribution instructions for more information.

tests/ui/question_mark.rs

clippy_lints/src/question_mark.rs

J-ZhengLi · 2022-01-30T09:18:22Z

Upon attempting to fix this issue, I found a new problem here, maybe I'm just a little bit confused.

After removing this line below

if path_to_local_id(peel_blocks(if_then), bind_id);

This lint will also triggered by:

fn check_option(x: Option<i32>) -> Option<i32> {
    do_something();
    if let Some(a) = x {
        Some(a)
    } else {
        None
    }
}

which is already triggering [`manual_map`], and offered a suggestion of replacing the if let with x.map(|a| a)

shouldn't a x? be more intuitive in this case?

camsteffen · 2022-02-04T21:55:04Z

@J-ZhengLi Sorry for the delay. I'll take a look at this soon. It looks like some internal lints are failing. Try running cargo test --features internal.

camsteffen · 2022-02-04T22:00:22Z

shouldn't a x? be more intuitive in this case?

That is a case where the match doesn't do anything. There is a suggested lint for that at #7040. For now, I would just let it be linted by manual_map only.

J-ZhengLi · 2022-02-14T11:26:07Z

@camsteffen I hope it's good now, let me know if anything comes up

tests/ui/question_mark.rs

clippy_lints/src/question_mark.rs

camsteffen · 2022-02-16T22:40:37Z

clippy_lints/src/question_mark.rs

+        nested_expr: &Expr<'_>,
+        pat_symbol: Option<Symbol>,
+    ) -> bool {
+        Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr, pat_symbol)


I think we could use some refactoring to avoid code repetition in a lot of these utils. For example, don't get the expression type in all the is_* functions. And just look for ExprKind::Ret in one place rather than having a bunch of *_and_early_return utils. Maybe you can have a refactor commit before other changes?

I tried refactoring it, and it still looks like speghatti unfortunatly @camsteffen

Instead of is_early_return(..) || is_early_return(..), try doing the following. I think that will make things a lot simpler.

if let Some(adt) = caller_ty.ty_adt_ty(); if let Some(name) = cx.tcx.get_diagnostic_name(adt.did); if matches!(name, sym::Option | sym::Result);

Thanks for that suggestion, it looks pretty handy. However, don't I still need to separate Option and Result as different cases for return check to prevent things like this from happening:

if let Ok(a) = result { Some(a) } else { None }

edit: I think I know how to do the refractor now nvm, I forgor what I had in mind

bors · 2022-03-13T16:33:47Z

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

bors · 2022-04-06T17:39:34Z

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

dswij · 2022-06-12T06:51:04Z

r? @dswij

dswij

Apologies that this got sidelined for a while. Thanks for the PR and for being patient!

The change itself looks great to me, just some small comments and I after that I think it's ready to be merged

dswij · 2022-06-22T13:12:54Z

clippy_lints/src/question_mark.rs

-            _ => false,
-        }
+fn offer_suggestion(cx: &LateContext<'_>, expr: &Expr<'_>, suggestion: String, applicability: Applicability) {
+    if !suggestion.is_empty() {


Let's use Option<String> rather than check for emptiness here.

I think we can even remove this check, I don't see a scenario where suggestion should be empty.

clippy_lints/src/question_mark.rs

…f-let` expressions

dswij · 2022-07-08T16:11:55Z

Thanks so much for this amazing work and refactors!
@bors r+ squash

bors · 2022-07-08T16:11:56Z

📌 Commit 76a0553 has been approved by dswij

It is now in the queue for this repository.

bors · 2022-07-08T16:12:03Z

⌛ Testing commit 76a0553 with merge 49f73e1...

Simplify if let statements fixes: #8288 --- changelog: Allowing [`qustion_mark`] lint to check `if let` expressions that immediatly return unwrapped value

bors · 2022-07-08T16:32:27Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 49f73e1 to master...

xFrednet · 2022-07-08T17:04:14Z

It looks like bors simply forgot to merge/close this PR. There is a commit for this on master and the issue has been closed. I'll try to ping bors and otherwise close this PR manually. Thank you for the contribution 🙃

@bors ping

bors · 2022-07-08T17:04:16Z

😪 I'm awake I'm awake

rust-highfive assigned camsteffen Jan 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 27, 2022

dswij reviewed Jan 27, 2022

View reviewed changes

tests/ui/question_mark.rs Outdated Show resolved Hide resolved

tests/ui/question_mark.rs Show resolved Hide resolved

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

J-ZhengLi changed the title ~~attemp to fix #8288~~ #8288 simple if_let as [question_mark] extension Jan 28, 2022

J-ZhengLi changed the title ~~#8288 simple if_let as [question_mark] extension~~ #8288 simple if_let as `[question_mark]` extension Jan 28, 2022

J-ZhengLi changed the title ~~#8288 simple if_let as `[question_mark]` extension~~ #8288 simple if_let as [question_mark] extension Jan 28, 2022

J-ZhengLi changed the title ~~#8288 simple if_let as [question_mark] extension~~ #8288 simple if_let as question_mark extension Jan 28, 2022

J-ZhengLi marked this pull request as ready for review January 28, 2022 08:23

J-ZhengLi force-pushed the master-issue8288 branch from 7c034ec to 03b2fdc Compare January 28, 2022 08:34

J-ZhengLi closed this Jan 28, 2022

J-ZhengLi force-pushed the master-issue8288 branch from 59863c8 to ea4db3a Compare January 28, 2022 09:49

J-ZhengLi reopened this Jan 29, 2022

J-ZhengLi marked this pull request as draft January 29, 2022 01:12

J-ZhengLi force-pushed the master-issue8288 branch 3 times, most recently from ef4c941 to 5d73a2b Compare February 10, 2022 10:02

J-ZhengLi marked this pull request as ready for review February 10, 2022 10:14

J-ZhengLi changed the title ~~#8288 simple if_let as question_mark extension~~ Simplify if let statements Feb 16, 2022

camsteffen suggested changes Feb 16, 2022

View reviewed changes

J-ZhengLi force-pushed the master-issue8288 branch from f6ac5a5 to 7d9eb34 Compare February 21, 2022 10:57

J-ZhengLi marked this pull request as draft February 21, 2022 12:29

J-ZhengLi force-pushed the master-issue8288 branch 2 times, most recently from 7aed5f0 to 2c98b63 Compare February 21, 2022 13:11

J-ZhengLi marked this pull request as ready for review February 23, 2022 04:19

J-ZhengLi force-pushed the master-issue8288 branch from 0ed2eda to 10f0b21 Compare March 9, 2022 02:31

J-ZhengLi closed this Mar 29, 2022

J-ZhengLi force-pushed the master-issue8288 branch from 10f0b21 to fb94992 Compare March 29, 2022 08:47

J-ZhengLi reopened this Mar 29, 2022

rust-highfive assigned dswij and unassigned camsteffen Jun 12, 2022

J-ZhengLi force-pushed the master-issue8288 branch 2 times, most recently from c35130a to 1462f84 Compare June 13, 2022 08:43

dswij reviewed Jun 22, 2022

View reviewed changes

J-ZhengLi added 2 commits July 2, 2022 18:52

expend [question_mark] lint to suggest for early early returning `i…

1e64193

…f-let` expressions

fix confliction with needless_match

bc11103

J-ZhengLi force-pushed the master-issue8288 branch from 1462f84 to bc11103 Compare July 2, 2022 10:53

some more code refractoring

76a0553

bors added a commit that referenced this pull request Jul 8, 2022

Auto merge of #8356 - J-ZhengLi:master-issue8288, r=dswij

49f73e1

Simplify if let statements fixes: #8288 --- changelog: Allowing [`qustion_mark`] lint to check `if let` expressions that immediatly return unwrapped value

xFrednet closed this Jul 8, 2022

Alexendoo mentioned this pull request Jul 9, 2022

Read authorship information from last commit when squashing rust-lang/homu#152

Open

Jarcho mentioned this pull request Aug 11, 2022

Fix false positives of needless_match #9092

Merged

J-ZhengLi deleted the master-issue8288 branch February 8, 2023 01:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify if let statements #8356

Simplify if let statements #8356

J-ZhengLi commented Jan 27, 2022 •

edited

Loading

rust-highfive commented Jan 27, 2022

J-ZhengLi commented Jan 30, 2022

camsteffen commented Feb 4, 2022

camsteffen commented Feb 4, 2022

J-ZhengLi commented Feb 14, 2022 •

edited

Loading

camsteffen Feb 16, 2022

J-ZhengLi Feb 24, 2022 •

edited

Loading

camsteffen Feb 25, 2022

J-ZhengLi Feb 26, 2022 •

edited

Loading

bors commented Mar 13, 2022

bors commented Apr 6, 2022

dswij commented Jun 12, 2022

dswij left a comment

dswij Jun 22, 2022

dswij Jun 22, 2022

dswij commented Jul 8, 2022

bors commented Jul 8, 2022

bors commented Jul 8, 2022

bors commented Jul 8, 2022

xFrednet commented Jul 8, 2022 •

edited

Loading

bors commented Jul 8, 2022

Simplify if let statements #8356

Simplify if let statements #8356

Conversation

J-ZhengLi commented Jan 27, 2022 • edited Loading

rust-highfive commented Jan 27, 2022

J-ZhengLi commented Jan 30, 2022

camsteffen commented Feb 4, 2022

camsteffen commented Feb 4, 2022

J-ZhengLi commented Feb 14, 2022 • edited Loading

camsteffen Feb 16, 2022

Choose a reason for hiding this comment

J-ZhengLi Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

camsteffen Feb 25, 2022

Choose a reason for hiding this comment

J-ZhengLi Feb 26, 2022 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 13, 2022

bors commented Apr 6, 2022

dswij commented Jun 12, 2022

dswij left a comment

Choose a reason for hiding this comment

dswij Jun 22, 2022

Choose a reason for hiding this comment

dswij Jun 22, 2022

Choose a reason for hiding this comment

dswij commented Jul 8, 2022

bors commented Jul 8, 2022

bors commented Jul 8, 2022

bors commented Jul 8, 2022

xFrednet commented Jul 8, 2022 • edited Loading

bors commented Jul 8, 2022

J-ZhengLi commented Jan 27, 2022 •

edited

Loading

J-ZhengLi commented Feb 14, 2022 •

edited

Loading

J-ZhengLi Feb 24, 2022 •

edited

Loading

J-ZhengLi Feb 26, 2022 •

edited

Loading

xFrednet commented Jul 8, 2022 •

edited

Loading