Use original variable name in the suggestion #10175

koka831 · 2023-01-07T17:38:26Z

changelog: Sugg: [manual_let_else]: Now suggest the correct variable name
#10175

rustbot · 2023-01-07T17:38:32Z

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

giraffate · 2023-01-10T01:37:02Z

tests/ui/manual_let_else_match.stderr

@@ -43,7 +43,7 @@ LL | /     let v = match f() {
 LL | |         Ok(v) => v,
 LL | |         Err(_) => return,
 LL | |     };
-   | |______^ help: consider writing: `let Ok(v) = f() else { return };`
+   | |______^ help: consider writing: `let Some(v) = f() else { return };`


This suggestion gives the compile error because of mismatched types.

Thank you for quick review, and I realized I've misunderstood the problem...
I'm sorry for bothering you, since my approach cannot handle cases like that so may I close the PR for now?

let v = match build_enum() { _ => continue, Variant::Bar(v) | Variant::Baz(v) => v, };

Okay, no worries! Anyway, thanks for your try to improve Clippy!

Yeah pattern editing is tough. rust-analyzer has library functionality that can edit patterns, and it uses that library functionality for its "replace manual let else with let-else" tool. It's pretty cool.

For | patterns, the additional challenge is that they have to be surrounded by (...).

Btw, if you want to fix this PR, you should remove "renamings of the bindings" from the list in the comments.

Since let_else feature has been stable in 1.65.0, it's now unnecessary

koka831 · 2023-02-01T10:35:32Z

clippy_lints/src/manual_let_else.rs

+                PatKind::TupleStruct(ref w, ..) => {
+                    let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
+                    let (sn_inner, _) = snippet_with_context(cx, local.span, span.ctxt(), "", &mut app);
+                    format!("{sn_wrapper}({sn_inner})")
+                },


@giraffate @est31 Thank you for your advice:)
Finally, it's addressed by separating the PatKind::Or case from the PatKind::TupleStruct case, which we should fixed in the PR.

Could you review it again? thanks

The code is absolutely not enough but it will improve the situation at least for the Some and Ok cases. The biggest issue I think is that tuple structs with multiple arguments are not supported, as in:

let foo = if let SomeEnum::Val(v, 0) = e() { v } else { _ };

Will be turned into:

let SomeEnum::Val(v) = e() else { _ };

But I think the PR should be accepted. 👍

@est31 Thank you for the quick review!
I see the example above should be handled like below:

let SomeEnum::Val(foo, 0) = e() { v } else { ... }; // ^ replaced ideally

In this PR, I made a change to keep left if TupleStruct has more than two arguments. ref
I hope it's enough for now:pray:

koka831 · 2023-02-01T10:36:18Z

clippy_lints/src/manual_let_else.rs

@@ -37,7 +37,6 @@ declare_clippy_lint! {
    /// Could be written:
    ///
    /// ```rust
-    /// # #![feature(let_else)]


IMHO let_else has been stable since 1.65 so it can be removed?

Absolutely, yes.

est31 · 2023-02-01T13:01:31Z

tests/ui/manual_let_else.rs

@@ -248,4 +254,13 @@ fn not_fire() {
        Some(value) => value,
        _ => macro_call!(),
    };
+
+    fn e() -> Variant {


Just a note: this is inside a function named not_fire. At least if you follow how I have originally designed the file,
the test should be moved to the end of the function do_fire (as should the Issue 9440 snippet above).

Maybe clippy conventions are different and this design is unique among clippy, idk. I did it back then because it seemed to me that this design makes it very easy to verify that the not_fire section does indeed not fire, you just look at the last emitted lint's line number and see that it is prior to the line number of the not_fire function. I'd have mixed firing and not firing had clippy included ERROR lines as the rustc testsuite does (which is IMO a good idea in general because in clippy you often have // should fire or // should not fire comments that do the same but are not machine verified).

I like your design. (Sorry for bothering, I've just overlooked it, I usually add new specs at the end of files.)
I edited the last commit and moved them into fire().
@est31 Thank you for your review:)

update spec fix: move specs in fire

est31 · 2023-03-01T13:02:12Z

Friendly ping @giraffate -- This is ready for review (and IMO good to be merged)

giraffate · 2023-03-01T13:06:13Z

Thanks for pinging! I'll review this in a few days.

giraffate

Overall looks good, thanks!

I made one comment for formatting.

giraffate · 2023-03-03T13:25:31Z

clippy_lints/src/manual_let_else.rs

+            local.els.is_none() &&
+            local.ty.is_none() &&
+            init.span.ctxt() == stmt.span.ctxt() &&
+            let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init) {


I don't know why rustfmt doesn't work for this case, but it would be better to deepen the indent.

Suggested change

let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init) {

let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init)

{

match if_let_or_match {

rustfmt doesn't support let chains right now: rust-lang/rustfmt#5203

@koka831 Sorry for the my late reviewing. Can this be addressed?

est31 · 2023-05-16T06:05:33Z

I think it's not being formatted by rustfmt due to rust-lang/rustfmt#5203

@giraffate given how @koka831's github activity is quite interrupted (I dearly hope that nothing bad has happened to them), maybe it would be the best to press the "commit suggestion" button and then approve the PR? Alternatively, merge it with this formatting issue and then fix it in a later PR. Edit: I offer to file that PR.

giraffate · 2023-05-17T00:28:03Z

@est31 Thanks for the reminder!

@giraffate given how @koka831's github activity is quite interrupted (I dearly hope that nothing bad has happened to them), maybe it would be the best to press the "commit suggestion" button and then approve the PR? Alternatively, merge it with this formatting issue and then fix it in a later PR. Edit: I offer to file that PR.

I'll fix it later. Thanks!

giraffate · 2023-05-17T00:28:24Z

@bors r+

bors · 2023-05-17T00:28:26Z

📌 Commit 07c8c50 has been approved by giraffate

It is now in the queue for this repository.

bors · 2023-05-18T01:21:30Z

⌛ Testing commit 07c8c50 with merge 7e18451...

bors · 2023-05-18T01:34:16Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 7e18451 to master...

koka831 · 2023-09-25T12:49:12Z

@est31 @giraffate I'm sorry for not being able to respond for a while due to health issues. Now that I have recovered, I hope to be active again:pray:

est31 · 2023-09-25T14:49:26Z

@koka831 thanks for the update! In the meantime, I have made #10797 where I addressed the one concern for this PR and generally greatly improved the state of patterns in the PR and the follow up work.

est31 · 2023-09-25T18:45:35Z

(Also great to hear that you are doing better now)

giraffate · 2023-09-26T01:08:32Z

@koka831 No worries! I'm glad you're feeling better!

rustbot assigned giraffate Jan 7, 2023

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

giraffate suggested changes Jan 10, 2023

View reviewed changes

koka831 added 3 commits February 1, 2023 19:17

Suggest the correct variable name for manual_let_else

a408e76

Split if conditions by its semantics

4931f45

Remove old feature flag in code comment

20338fd

Since let_else feature has been stable in 1.65.0, it's now unnecessary

koka831 force-pushed the fix/10171 branch from aea0082 to 20338fd Compare February 1, 2023 10:31

koka831 commented Feb 1, 2023

View reviewed changes

koka831 requested a review from giraffate February 1, 2023 10:36

koka831 force-pushed the fix/10171 branch from 0a6f3e1 to 326124b Compare February 1, 2023 11:58

est31 reviewed Feb 1, 2023

View reviewed changes

est31 approved these changes Feb 1, 2023

View reviewed changes

Avoid renaming for TupleStruct with multiple arguments

07c8c50

update spec fix: move specs in fire

koka831 force-pushed the fix/10171 branch from 326124b to 07c8c50 Compare February 1, 2023 13:14

est31 mentioned this pull request Mar 1, 2023

[manual_let_else] does not take binding variable name into account #10424

Closed

giraffate reviewed Mar 3, 2023

View reviewed changes

giraffate approved these changes May 17, 2023

View reviewed changes

bors merged commit 7e18451 into rust-lang:master May 18, 2023

est31 mentioned this pull request May 18, 2023

[manual_let_else] does not copy mut modifier #10431

Closed

koka831 deleted the fix/10171 branch September 25, 2023 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use original variable name in the suggestion #10175

Use original variable name in the suggestion #10175

koka831 commented Jan 7, 2023 •

edited by xFrednet

Loading

rustbot commented Jan 7, 2023

giraffate Jan 10, 2023

koka831 Jan 10, 2023 •

edited

Loading

giraffate Jan 11, 2023

est31 Jan 11, 2023

koka831 Feb 1, 2023

est31 Feb 1, 2023

est31 Feb 1, 2023

koka831 Feb 1, 2023 •

edited

Loading

koka831 Feb 1, 2023

est31 Feb 1, 2023

est31 Feb 1, 2023

koka831 Feb 1, 2023 •

edited

Loading

est31 commented Mar 1, 2023

giraffate commented Mar 1, 2023

giraffate left a comment

giraffate Mar 3, 2023

est31 Mar 4, 2023

giraffate Mar 15, 2023

est31 commented May 16, 2023 •

edited

Loading

giraffate commented May 17, 2023

giraffate commented May 17, 2023

bors commented May 17, 2023

bors commented May 18, 2023

bors commented May 18, 2023

koka831 commented Sep 25, 2023

est31 commented Sep 25, 2023

est31 commented Sep 25, 2023

giraffate commented Sep 26, 2023

Use original variable name in the suggestion #10175

Use original variable name in the suggestion #10175

Conversation

koka831 commented Jan 7, 2023 • edited by xFrednet Loading

rustbot commented Jan 7, 2023

Choose a reason for hiding this comment

koka831 Jan 10, 2023 • edited Loading

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

koka831 Feb 1, 2023 • edited Loading

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

koka831 Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

est31 commented Mar 1, 2023

giraffate commented Mar 1, 2023

giraffate 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

est31 commented May 16, 2023 • edited Loading

giraffate commented May 17, 2023

giraffate commented May 17, 2023

bors commented May 17, 2023

bors commented May 18, 2023

bors commented May 18, 2023

koka831 commented Sep 25, 2023

est31 commented Sep 25, 2023

est31 commented Sep 25, 2023

giraffate commented Sep 26, 2023

koka831 commented Jan 7, 2023 •

edited by xFrednet

Loading

koka831 Jan 10, 2023 •

edited

Loading

koka831 Feb 1, 2023 •

edited

Loading

koka831 Feb 1, 2023 •

edited

Loading

est31 commented May 16, 2023 •

edited

Loading