Add lint to tell about let else pattern #8437

est31 · 2022-02-15T05:43:03Z

Adds a lint to tell the user if the let_else pattern should be used.

~~The PR is blocked probably on rustfmt support, as clippy shouldn't suggest features that aren't yet fully supported by all tools.~~ Edit: I guess adding it as a restriction lint for now is the best option, it can be turned into a style lint later.

changelog: addition of a new lint to check for manual let else

rust-highfive · 2022-02-15T05:43:06Z

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

clippy_lints/src/manual_let_else.rs

clippy_utils/src/msrvs.rs

clippy_lints/src/manual_let_else.rs

joshtriplett · 2022-03-08T17:21:22Z

clippy_lints/src/manual_let_else.rs

+    /// ```
+    #[clippy::version = "1.60.0"]
+    pub MANUAL_LET_ELSE,
+    restriction,


Suggested change

restriction,

style,

As long as this only matches let x = if let, I think it can reasonably be a style lint.

Matching let x = match would lead to false positives, where someone doesn't want to lose the exhaustive matching or the name of the other variant; that should be a separate allow-by-default restriction lint, with an explicit note that it's intended only for identifying possible locations and some suggestions for where it makes sense to make such a change.

Splitting those two cases into separate lints would allow us to enable the more conservative version by default.

I think it can reasonably be a style lint.

I would be more careful, as there is no rustfmt support for it yet.

Matching let x = match would lead to false positives

Note that I have severely restricted the lint for match.

I've been making the assumption that this PR won't get merged until we have full support for let-else. Given that assumption, I think it can be a style lint. I agree that it shouldn't be until then.

@joshtriplett good, we agree then :)

flip1995 · 2022-03-17T12:48:23Z

Generally I like the idea of this lint. But as mentioned earlier, it would be good to wait until rust-lang/rust#93628 is merged. So I marked this as blocked for now.

bors · 2022-04-06T18:22:47Z

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

bors · 2022-06-04T08:18:41Z

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

est31 · 2022-10-16T04:06:43Z

Yeah I could be wrong about the visitors. I was thinking you only need to manually step through control flow things like if/loop and anything else you just stop and check the expr type.

Hmm I still don't understand what you mean by that. But I have refactored the search a little to remove the local mutable variable and use the return types. This has simplified it a little bit I think. One could think about a hybrid that checks the expr's type each time and if it diverges, then early exits.

est31 · 2022-10-16T05:20:53Z

@flip1995 I have changed it back to use the visitor based for_each_expr.

I've also been thinking about the starting lint level of pedantic. Not because I think the lint is bad quality, it's pretty great I think, but because the feature it's endorsing isn't even available on stable yet, just in beta right now, and will be on stable starting November 3rd.

I think there is a non zero population of people who use clippy from nightly but still target stable. For them, it would be bad if clippy started linting about this mid of October. Many projects would probably have an explicit msrv set, but I think most still don't. I think this group of people makes up a large group of people who have turned on pedantic lints.

Therefore, I think we should do one of the following:

Wait with merging this for 6 to 8 weeks so that there has been enough time since the stable release has been disseminated.
Default the MSRV to three versions before the current if not specified. This can either be done centrally, or just for the manual_let_else lint alone
Move the starting level to nursery or restriction

I would prefer the last option, then the second option. What do you think @flip1995 ?

Outside of this question, I think all issues should be resolved.

flip1995

LGTM!

Good point about people using nightly Clippy but targeting stable. This lint will now only get into nightly 1.67. And to be absolutely sure about that, I'll wait with merging this until rust-lang/rust#103337 is merged.

So this will only hit nightly when the feature is already stable. No need to take further action here.

@camsteffen I would merge it now with the visitor. I couldn't find a better way to do this when I looked into it. Let me know if you still think this should be changed.

@est31 Great work on the implementation and great code quality (including comments)!

clippy_lints/src/manual_let_else.rs

bors · 2022-10-23T22:31:13Z

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

…on obvious paths

They aren't perfect but better than nothing

We cannot apply the lint for 3-branches like in the added example.

Sometimes type annotations are needed for type inferrence to work, or because of coercions. We don't know this, and we also don't want users to possibly repeat the entire pattern.

Plus, add some tests for the divergence check.

flip1995 · 2022-10-24T20:21:02Z

Let's ship it 🚢 Thanks again for all the work!

@bors r+

bors · 2022-10-24T20:21:04Z

📌 Commit dcde480 has been approved by flip1995

It is now in the queue for this repository.

bors · 2022-10-24T20:21:10Z

⌛ Testing commit dcde480 with merge b698a15...

bors · 2022-10-24T20:31:48Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing b698a15 to master...

rust-highfive assigned flip1995 Feb 15, 2022

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

hellow554 reviewed Feb 15, 2022

View reviewed changes

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

flip1995 reviewed Feb 15, 2022

View reviewed changes

clippy_utils/src/msrvs.rs Outdated Show resolved Hide resolved

est31 force-pushed the let_else_lint branch 12 times, most recently from dc3602d to c33031a Compare February 19, 2022 23:55

joshtriplett reviewed Mar 8, 2022

View reviewed changes

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

joshtriplett reviewed Mar 8, 2022

View reviewed changes

est31 force-pushed the let_else_lint branch from c33031a to dd83257 Compare March 9, 2022 00:50

est31 mentioned this pull request Mar 14, 2022

Stabilize let_chains in Rust 1.64 rust-lang/rust#94927

Merged

flip1995 added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 17, 2022

est31 force-pushed the let_else_lint branch from dd83257 to 44771f5 Compare April 12, 2022 11:25

est31 mentioned this pull request Apr 27, 2022

suggest replacing let else with ? statement where applicable #8755

Closed

est31 force-pushed the let_else_lint branch from 44771f5 to d545737 Compare May 22, 2022 21:38

est31 force-pushed the let_else_lint branch from d545737 to 6692803 Compare September 18, 2022 04:10

est31 force-pushed the let_else_lint branch 2 times, most recently from 383a44f to 0e6dc72 Compare October 16, 2022 03:00

est31 force-pushed the let_else_lint branch 2 times, most recently from 7f96771 to 8ac1bae Compare October 16, 2022 05:18

flip1995 approved these changes Oct 23, 2022

View reviewed changes

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

est31 force-pushed the let_else_lint branch from 8ac1bae to c47d699 Compare October 24, 2022 20:01

est31 and others added 11 commits October 24, 2022 22:05

Add lint to tell about let else pattern

f827be9

Also support linting for match

2e01e6b

Support tuples

c5a7696

Don't suggest let else in match if the else arm explicitly mentions n…

5da7a17

…on obvious paths

Replace from_different_macros with equivalent and simpler check

173a8e0

Make the match checking configurable

9bd70db

Make an attempt of creating suggestions

a1db931

They aren't perfect but better than nothing

Don't lint if the let is already a let-else

01e651f

We cannot apply the lint for 3-branches like in the added example.

Don't fire the lint if there is a type annotation

748169d

Sometimes type annotations are needed for type inferrence to work, or because of coercions. We don't know this, and we also don't want users to possibly repeat the entire pattern.

Fix dogfooding

96ea5b2

Also consider match guards for divergence check

dcde480

Plus, add some tests for the divergence check.

est31 force-pushed the let_else_lint branch from c47d699 to dcde480 Compare October 24, 2022 20:06

bors merged commit b698a15 into rust-lang:master Oct 24, 2022

This was referenced Oct 29, 2022

Add let else statements rust-lang/style-team#165

Closed

Suggest let...else with return over match with return #9749

Closed

kangalio mentioned this pull request Nov 3, 2022

Suggest rewriting applicable match or if-let statements to new let-else syntax #7540

Closed

est31 mentioned this pull request Nov 10, 2022

TESTNAME should be in CONTRIBUTING.md #9825

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint to tell about let else pattern #8437

Add lint to tell about let else pattern #8437

est31 commented Feb 15, 2022 •

edited

Loading

rust-highfive commented Feb 15, 2022

joshtriplett Mar 8, 2022 •

edited

Loading

est31 Mar 8, 2022

joshtriplett Mar 8, 2022 •

edited

Loading

est31 Mar 8, 2022

flip1995 commented Mar 17, 2022

bors commented Apr 6, 2022

bors commented Jun 4, 2022

est31 commented Oct 16, 2022

est31 commented Oct 16, 2022

flip1995 left a comment

bors commented Oct 23, 2022

flip1995 commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

Add lint to tell about let else pattern #8437

Add lint to tell about let else pattern #8437

Conversation

est31 commented Feb 15, 2022 • edited Loading

rust-highfive commented Feb 15, 2022

joshtriplett Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

est31 Mar 8, 2022

Choose a reason for hiding this comment

joshtriplett Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

est31 Mar 8, 2022

Choose a reason for hiding this comment

flip1995 commented Mar 17, 2022

bors commented Apr 6, 2022

bors commented Jun 4, 2022

est31 commented Oct 16, 2022

est31 commented Oct 16, 2022

flip1995 left a comment

Choose a reason for hiding this comment

bors commented Oct 23, 2022

flip1995 commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

bors commented Oct 24, 2022

est31 commented Feb 15, 2022 •

edited

Loading

joshtriplett Mar 8, 2022 •

edited

Loading

joshtriplett Mar 8, 2022 •

edited

Loading