Suggest `Option::map_or`(_else) for `if let Some { y } else { x }` #5301

JarredAllen · 2020-03-11T15:57:02Z

There are two issues with this code that I have noticed:

Use of Option::map_or causes it to always evaluate the code in the else block. If that block is computationally expensive or if it updates some state (such as getting the next value from an iterator), then this change would cause the code to behave differently. In either of those circumstances, it should suggest Option::map_or_else, which takes both cases as a closure and runs one. However, I don't know how to check if the expression would change some state, so I left the lint's applicability as MaybeIncorrect.
There are lints which can trigger on specific sub-cases of this lint (if_let_some_result, question_mark, and while_let_loop) and suggest different changes (usually better ones because they're more specific). Is this acceptable for clippy to give multiple suggestions, or should I have the code check if those other lints trigger and then not trigger this lint if they do?

changelog: Add lint [option_if_let_else]

flip1995 · 2020-03-11T16:37:45Z

For your first point, there's a pretty simple solution: If the block has statements (besides the returning expression), then you should suggest map_or_else(|| { /* else block */ }, ...). If the else block only contains a returning expression, you can suggest map_or(expr, ...). If this expression is then a function call, that can have side effects, the or_fun_call lint will take care of this afterwards.

For your second point. The if_let_some_result lint and this lint should not trigger at the same time, but the if_let_some_result should be prioritized. Since both lints address if let Some(..) = expr constructs, maybe you can reuse some code of this lint and the if_let_some_result lint and don't trigger this lint, if the if_let_some_result is triggered.

For the other two: This new lint should never trigger, if there is a break, continue, or return statement in one of the if/else blocks, since using a closure for this won't work. (Well, maybe it works for break/continue, but I'm not sure how they behave in a closure. I think break/continue are at least confusing in a closure, if they aren't in a loop inside the closure)

bors · 2020-03-23T20:56:12Z

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

bors · 2020-03-30T20:12:37Z

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

bors · 2020-04-08T14:33:48Z

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

JarredAllen · 2020-04-10T01:38:07Z

I pulled the changes which happened in the master branch while I was gone (sorry for the delay, I had things to deal with involving COVID-19, but everything's fine now), and now, when I try to build, it complains:

error[E0463]: can't find crate for rustc_hir_pretty

I've tried everything I can think of to make it appear, but nothing seems to make it work. If it matters, here's the output of running cargo --version and rustc --version on my machine:

cargo 1.43.0-nightly (e57bd0299 2020-02-21)

rustc 1.43.0-nightly (beac68a88 2020-03-01)

Is there anything that I need to do to make that crate appear?

JarredAllen · 2020-04-14T23:06:18Z

Failed tests are ui/author/blocks.rs and ui/swap.rs, and I didn't mess with either of those lints, so I think this is ready to merge.

flip1995

Sorry for taking so long for the review. I don't have an excuse, just wasn't really motivated to review PRs recently.

clippy_lints/src/option_if_let_else.rs

tests/ui/author/blocks.rs

clippy_lints/src/option_if_let_else.rs

tests/ui/option_if_let_else.rs

clippy_lints/src/option_if_let_else.rs

flip1995 · 2020-04-15T16:14:03Z

To get the latest rustc toolchain, you should run sh setup-toolchain.sh, which will install the current rustc master toolchain for you.

clippy_lints/src/option_if_let_else.rs

tests/ui/swap.rs

flip1995 · 2020-04-16T18:43:48Z

After undoing the change to tests/ui/swap.rs and a rebase(+squash) to get rid of the merge commit and this is ready to go.

bors · 2020-04-17T16:21:06Z

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

flip1995 · 2020-04-17T16:42:27Z

@bors r+

Thanks!

bors · 2020-04-17T16:42:28Z

📌 Commit ff4da34 has been approved by flip1995

bors · 2020-04-17T16:43:39Z

⌛ Testing commit ff4da34 with merge ed748d3...

Suggest `Option::map_or`(_else) for `if let Some { y } else { x }` Fixes #5203 There are two issues with this code that I have noticed: - Use of `Option::map_or` causes it to always evaluate the code in the else block. If that block is computationally expensive or if it updates some state (such as getting the next value from an iterator), then this change would cause the code to behave differently. In either of those circumstances, it should suggest Option::map_or_else, which takes both cases as a closure and runs one. However, I don't know how to check if the expression would change some state, so I left the lint's applicability as MaybeIncorrect. - There are lints which can trigger on specific sub-cases of this lint (`if_let_some_result`, `question_mark`, and `while_let_loop`) and suggest different changes (usually better ones because they're more specific). Is this acceptable for clippy to give multiple suggestions, or should I have the code check if those other lints trigger and then not trigger this lint if they do?

bors · 2020-04-17T16:44:09Z

💔 Test failed - checks-action_test

flip1995 · 2020-04-17T19:15:15Z

There are a lot of findings in the Clippy codebase itself. You should be able to use cargo clippy --fix -Zunstable-options to fix this. This will do the job (under Linux):

$ cargo +master install --path . --force  # Install Clippy
$ cp ~/.cargo/bin/*clippy* ~/.rustup/toolchains/master/bin  # Make Clippy available in the master toolchain
$ export LD_LIBRARY_PATH=$(rustc +master --print sysroot)/lib  # Set library path
$ cd clippy_lints
$ cargo +master clippy --fix -Zunstable-options  # Fix findings with the previously installed clippy
$ cd -
$ cargo dev fmt  # reformat codebase

bors · 2020-06-30T14:44:43Z

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

flip1995 · 2020-07-03T10:33:16Z

Thanks for rebasing! There's again some fallout of the rebase (cx.tables->cx.tables()). I want to prioritize merging this PR, since it modifies multiple files across the codebase, so please ping me after you addressed the fallout (force pushes tend to not show up in GH notifications).

bors · 2020-07-03T15:12:19Z

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

JarredAllen · 2020-07-04T04:37:42Z

@flip1995 Sorry for the delay. I had to force-push without it quite working yet because something broke on my machine that causes it to randomly fail a test in the middle, so using the automatic Travis testing was the only way I could actually test it. But it passes all the tests now, so it should finally be good to merge.

flip1995 · 2020-07-06T15:56:46Z

@bors r+ p=10

Thanks!

bors · 2020-07-06T15:56:48Z

📌 Commit c8f700e has been approved by flip1995

bors · 2020-07-06T15:56:58Z

⌛ Testing commit c8f700e with merge ac85692...

bors · 2020-07-06T16:21:54Z

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

Arnavion · 2020-07-19T07:12:11Z

FYI, issues arising from this lint:

option_if_let_else suggests map_or instead of map_or_else even if else branch contains a function call #5821 - The expectation that this lint would be okay to suggest o.map_or(g(), |i| f(i)) because or_fun_call would then suggest o.map_or_else(|| g(), |i| f(i)) doesn't actually hold. (Should've added a test for it :) )
option_if_let_else suggestion inapplicable when mutable borrow or owned value is used in both branches #5822 - The lint suggests o.map_or(expr_that_uses_foo, |_| expr_that_uses_foo) which cannot compile because foo can only be used once, such as when foo is a &mut _ or is consumed by expr_that_uses_foo

flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Mar 11, 2020

JarredAllen force-pushed the option_if_let_else branch from ff0e564 to 038ab65 Compare April 14, 2020 20:30

JarredAllen changed the title ~~(WIP) Suggest Option::map_or(_else) for if let Some { y } else { x }~~ Suggest Option::map_or(_else) for if let Some { y } else { x } Apr 14, 2020

flip1995 self-requested a review April 15, 2020 13:39

flip1995 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 15, 2020

flip1995 requested changes Apr 15, 2020

View reviewed changes

flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Apr 15, 2020

flip1995 requested changes Apr 16, 2020

View reviewed changes

flip1995 reviewed Apr 16, 2020

View reviewed changes

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

flip1995 approved these changes Apr 16, 2020

View reviewed changes

JarredAllen force-pushed the option_if_let_else branch 2 times, most recently from d074cdd to f958879 Compare April 16, 2020 20:22

JarredAllen force-pushed the option_if_let_else branch from f958879 to ff4da34 Compare April 17, 2020 16:41

JarredAllen force-pushed the option_if_let_else branch from 890e4b6 to d9fe3b0 Compare July 1, 2020 18:37

JarredAllen added 13 commits July 3, 2020 16:47

Lint for if let Some(x) = ... instead of Option::map_or

bf48a2d

Stop linting on macros and correctly use braces for constructs

82f8d4d

Properly parenthesize to avoid operator precedence errors

b85796f

Handle ref, mut, &, and &mut on the option

88c8afd

Refactoring

f73b455

Refactoring pt. 2

7c4de9d

Don't lint if contains a macro

5e20475

Used clippy to clean itself

5150277

Last few tweaks

93f0f5d

Fix compile error from library change

ccb9998

Clean existing lint code to match new lint

6ce9812

Update compile-test to follow new lint

6e2d55c

Formatted updates to lints

1c32263

JarredAllen force-pushed the option_if_let_else branch from 72e88af to 1c32263 Compare July 4, 2020 03:28

Fixed compile errors

c8f700e

JarredAllen force-pushed the option_if_let_else branch from 020cc8c to c8f700e Compare July 4, 2020 04:22

bors merged commit ac85692 into rust-lang:master Jul 6, 2020

This was referenced Jul 6, 2020

Move range_minus_one to pedantic #5752

Merged

new lint: match_like_matches_macro #5769

Merged

Arnavion mentioned this pull request Jul 19, 2020

option_if_let_else suggests map_or instead of map_or_else even if else branch contains a function call #5821

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest `Option::map_or`(_else) for `if let Some { y } else { x }` #5301

Suggest `Option::map_or`(_else) for `if let Some { y } else { x }` #5301

JarredAllen commented Mar 11, 2020 •

edited by flip1995

Loading

flip1995 commented Mar 11, 2020

bors commented Mar 23, 2020

bors commented Mar 30, 2020

bors commented Apr 8, 2020

JarredAllen commented Apr 10, 2020

JarredAllen commented Apr 14, 2020

flip1995 left a comment

flip1995 commented Apr 15, 2020

flip1995 commented Apr 16, 2020

bors commented Apr 17, 2020

flip1995 commented Apr 17, 2020

bors commented Apr 17, 2020

bors commented Apr 17, 2020

bors commented Apr 17, 2020

flip1995 commented Apr 17, 2020 •

edited

Loading

bors commented Jun 30, 2020

flip1995 commented Jul 3, 2020

bors commented Jul 3, 2020

JarredAllen commented Jul 4, 2020

flip1995 commented Jul 6, 2020

bors commented Jul 6, 2020

bors commented Jul 6, 2020

bors commented Jul 6, 2020

Arnavion commented Jul 19, 2020

Suggest Option::map_or(_else) for if let Some { y } else { x } #5301

Suggest Option::map_or(_else) for if let Some { y } else { x } #5301

Conversation

JarredAllen commented Mar 11, 2020 • edited by flip1995 Loading

flip1995 commented Mar 11, 2020

bors commented Mar 23, 2020

bors commented Mar 30, 2020

bors commented Apr 8, 2020

JarredAllen commented Apr 10, 2020

JarredAllen commented Apr 14, 2020

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Apr 15, 2020

flip1995 commented Apr 16, 2020

bors commented Apr 17, 2020

flip1995 commented Apr 17, 2020

bors commented Apr 17, 2020

bors commented Apr 17, 2020

bors commented Apr 17, 2020

flip1995 commented Apr 17, 2020 • edited Loading

bors commented Jun 30, 2020

flip1995 commented Jul 3, 2020

bors commented Jul 3, 2020

JarredAllen commented Jul 4, 2020

flip1995 commented Jul 6, 2020

bors commented Jul 6, 2020

bors commented Jul 6, 2020

bors commented Jul 6, 2020

Arnavion commented Jul 19, 2020

Suggest `Option::map_or`(_else) for `if let Some { y } else { x }` #5301

Suggest `Option::map_or`(_else) for `if let Some { y } else { x }` #5301

JarredAllen commented Mar 11, 2020 •

edited by flip1995

Loading

flip1995 commented Apr 17, 2020 •

edited

Loading