Lint `Option.map(f)` where f returns unit #1467

philipturnbull · 2017-01-23T02:52:34Z

This fixes #1352.

The logic for finding instances is relatively easy. Most of the code is dealing with generating nice, valid suggestions.

killercup

Sorry, I'm not a regular Clippy contributor or anything, but I just wanted to mention that this construct is not called "nil function" in Rust – as Rust doesn't have a nil value! :)

Functions without return values return an empty tuple (()), called Unit. (To add to the confusion, this is also where you'd say a function returns void in C, but the common definition of Void is that it describes a diverging function, e.g. an endless loop, or a process abort.)

killercup · 2017-01-23T07:08:49Z

clippy_lints/src/map_nil_fn.rs

+pub struct Pass;
+
+/// **What it does:** Checks for usage of `Option.map(f)` where f is a nil
+/// function or closure


In Rust, it's not called "nil function". It's actually a function that returns Unit (())

Given that this code also handles uninhabited/never types, it's not just unit, so we might want to keep the function names the same. However, the user-facing lint name and documentation should probably just handle unit; using a diverging expression in map sounds like a very rare use case.

Whoops, thanks for the explanation. I will tidy up the user facing portions to consistently use unit. I added the divergent support just for completeness and I couldn't think of any usecases apart from maybe _.map(std::process::exit). I can remove it to simplify the code slightly

killercup · 2017-01-23T07:09:23Z

clippy_lints/src/map_nil_fn.rs

+/// x.map(|msg| log_err_msg(format_msg(msg)))
+/// ```
+/// The correct use would be:
+/// ```rust


I'd put blank lines around code blocks

killercup · 2017-01-23T07:09:57Z

clippy_lints/src/map_nil_fn.rs

+// The expression inside a closure may or may not have surrounding braces and
+// semicolons, which causes problems when generating a suggestion. Given an
+// expression that evaluates to '()' or '!', recursively remove useless braces
+// and semi-colons until is suitable for including in the suggestion template


could probably be a doc comment with three slashes

Manishearth

This is good! Minor issues, then we can land this

Manishearth · 2017-01-23T07:14:44Z

CHANGELOG.md

@@ -374,6 +374,7 @@ All notable changes to this project will be documented in this file.
 [`nonsensical_open_options`]: https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options
 [`not_unsafe_ptr_arg_deref`]: https://github.com/Manishearth/rust-clippy/wiki#not_unsafe_ptr_arg_deref
 [`ok_expect`]: https://github.com/Manishearth/rust-clippy/wiki#ok_expect
+[`option_map_nil_fn`]: https://github.com/Manishearth/rust-clippy/wiki#option_map_nil_fn


It's called unit.

Manishearth · 2017-01-23T07:14:57Z

README.md

@@ -295,6 +295,7 @@ name
 [nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options)                   | warn    | nonsensical combination of options for opening a file
 [not_unsafe_ptr_arg_deref](https://github.com/Manishearth/rust-clippy/wiki#not_unsafe_ptr_arg_deref)                   | warn    | public functions dereferencing raw pointer arguments but not marked `unsafe`
 [ok_expect](https://github.com/Manishearth/rust-clippy/wiki#ok_expect)                                                 | warn    | using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result
+[option_map_nil_fn](https://github.com/Manishearth/rust-clippy/wiki#option_map_nil_fn)                                 | allow   | using `Option.map(f)`, where f is a nil function or closure


"Function or closure returning unit"

Manishearth · 2017-01-23T07:17:38Z

clippy_lints/src/map_nil_fn.rs

+/// ```
+declare_lint! {
+    pub OPTION_MAP_NIL_FN,
+    Allow,


Can this be Warn? I'm not sure, since .map() is also a rather common construct even with unit functs, and I'm not sure if if let really is unambiguously better. I personally use if let as much as possible, though it took me some time to switch over to that (and you'll see a lot of pre-2014 code using it).

thoughts @llogiq @mcarton

I use map when I have a ready-made function that fits. If I needed to insert a closure that returns unit, I'd pick if let.

I agree that for the fn case that if let isn't obviously better so it should stay Deny. But I do think if let is clearer for closures though. Maybe we could split it into two lints:

OPTION_MAP_UNIT_FN which is Allow

OPTION_MAP_UNIT_CLOSURE which is Deny

I wouldn't Deny a lint unless we can be reasonably sure to have found an error. Warn should be preferred here IMHO.

Manishearth · 2017-01-23T07:19:43Z

clippy_lints/src/map_nil_fn.rs

+#[derive(Clone)]
+pub struct Pass;
+
+/// **What it does:** Checks for usage of `Option.map(f)` where f is a nil


What if we instead lint for Option.map(f) where the result isn't used? Covers non-unit functions too. The code can be similar to the must_use lint from rustc.
Lint is fine as is too though.

https://github.com/rust-lang/rust/blob/master/src/librustc_lint/unused.rs#L110

Manishearth · 2017-01-23T07:23:26Z

clippy_lints/src/map_nil_fn.rs

+// expression that evaluates to '()' or '!', recursively remove useless braces
+// and semi-colons until is suitable for including in the suggestion template
+fn reduce_nil_expression<'a>(cx: &LateContext, expr: &'a hir::Expr) -> Option<Span> {
+    if !is_nil_expression(cx, expr) {


I wonder if the bulk of this function could be moved into utils as a general-purpose closure-reducing function.

Manishearth · 2017-01-23T07:25:01Z

clippy_lints/src/map_nil_fn.rs

+pub struct Pass;
+
+/// **What it does:** Checks for usage of `Option.map(f)` where f is a nil
+/// function or closure


Given that this code also handles uninhabited/never types, it's not just unit, so we might want to keep the function names the same. However, the user-facing lint name and documentation should probably just handle unit; using a diverging expression in map sounds like a very rare use case.

Manishearth · 2017-01-23T07:36:41Z

@killercup

Sorry, I'm not a regular Clippy contributor or anything,

Feel free to review clippy PRs 😄

oli-obk · 2017-03-24T09:26:16Z

@philipturnbull are you planning to continue this PR or do you want someone to take over?

oli-obk · 2017-06-09T18:08:30Z

Friendly ping @philipturnbull

philipturnbull · 2017-06-09T18:29:55Z

So sorry 😞 I missed your first ping. I haven't got time to work on this atm, so someone else can take over this PR.

One problem I had was some "analysis paralysis" over classifying things as Allow/Warn/Deny. As discussed in other comments there doesn't seem to be one canonical style so it isn't obvious what the default values should be.

oli-obk · 2017-06-09T18:46:58Z

OK. Don't worry about classifications. Usually we make stuff warn by default as long as they don't have false positives. Deny by default is reserved for bad bugs that eat laudry. Even if we get it wrong, it can quickly be fixed later

phansch · 2018-04-07T12:51:25Z

@oli-obk I would like to finish this one up. I have a rebased branch ready but should I open a new PR or continue (force) pushing to philipturnbull:option_map_nil_fn?

oli-obk · 2018-04-07T13:07:11Z

@phansch feel free to force-push here, that way all the discussion stays in one place.

oli-obk · 2018-04-09T16:47:27Z

tests/ui/option_map_unit_fn.stderr

+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Some(value) = x.field { ... }`
+


Is it possible to make this suggestion contain the expression?

Yup, looks like it's possible. Tomorrow I want to add a bunch more test cases with multiple statements and expressions and see how those behave.

Okay, I have to take that back as I'm currently stuck here.
The culprit is this match block:

https://github.com/philipturnbull/rust-clippy/blob/85ad541747039733332ce47a586203506fc11efe/clippy_lints/src/option_map_unit_fn.rs#L90-L103

Currently it only covers two cases:

no statement, one expression

one statement, no expression

For any other case it returns None; resulting in the placeholder ....

The cases not covered are blocks that:

contain both statements and expressions { foo; bar; "baz" }

contain multiple statements { abc; def; }

span multiple lines (these get turned into a single-line suggestion as it is currently)

Is there a way to get the full span of inside a block, without the braces directly, without doing all the reduction? I had a look at the utils, but there was only remove_blocks that returns the input block expression if it contains any statements.

I don't think we have anything for that. For now, just make the None case use span_lint_approximate

oli-obk · 2018-04-09T16:49:07Z

tests/ui/option_map_unit_fn.stderr

+35 |     x.field.map(do_nothing);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^-
+   |     |
+   |     help: try this: `if let Some(...) = x.field { do_nothing(...) }`


Maybe we could generate a variable name? The name could be taken from the variable that is matched on.

Something like x_field or field in this case? I guess it could cause problems (with rustfix?) if there is already a variable with the same name as the generated one?

There could be, but let's do it anyway and mark the suggestion as approximate

oli-obk · 2018-04-09T16:49:56Z

clippy_lints/src/option_map_unit_fn.rs

+                        hir::StmtDecl(ref d, _) => Some(d.span),
+                        hir::StmtExpr(ref e, _) => Some(e.span),
+                        hir::StmtSemi(ref e, _) => {
+                            if is_unit_expression(cx, e) {


Any particular reason we don't just keep the semicolon always?

I pushed 85ad541 to remove it and it seems fine. The new stderr output doesn't show any weird changes, so I guess it was just an optical thing.

phansch · 2018-04-10T08:27:18Z

@oli-obk I will have a look at the suggestion improvements later today, thanks! Do you think it also makes sense to expand this to Result.map() since it's the same API?

oli-obk · 2018-04-10T08:27:56Z

Yes that makes sense

`reduce_nil_closure` mixed together a) 'is this a nil closure?' and b) 'can it be reduced to a simple expression?'. Split the logic into two functions so we can still generate a basic warning when the closure can't be simplified.

This was a bit messed up after a bigger rebase.

Rust does not have nil.

Given a map call like `x.field.map ...` the suggestion will contain: `if let Some(x_field) ...` Given a map call like `x.map ...` the suggestion will contain: `if let Some(_x) ...` Otherwise it will suggest: `if let Some(_) ...`

phansch · 2018-04-15T12:14:49Z

Ok, the last three commits contain the (basic) let binding variable suggestions,make two of the three suggestions approximate and also add the lint for Result.map ✨

If the diff is too big, I can also open a separate PR for the Result.map lint.

oli-obk

Just some nits.

oli-obk · 2018-04-15T12:24:04Z

clippy_lints/src/map_unit_fn.rs

+#[derive(Clone)]
+pub struct Pass;
+
+/// **What it does:** Checks for usage of `Option.map(f)` where f is a function


Write Option in lowercase

oli-obk · 2018-04-15T12:24:29Z

clippy_lints/src/map_unit_fn.rs

+/// or closure that returns the unit type.
+///
+/// **Why is this bad?** Readability, this can be written more clearly with
+/// an if statement


an if let statement

oli-obk · 2018-04-15T12:25:37Z

clippy_lints/src/map_unit_fn.rs

+    "using `Option.map(f)`, where f is a function or closure that returns ()"
+}
+
+/// **What it does:** Checks for usage of `Result.map(f)` where f is a function


lowercase Result

oli-obk · 2018-04-15T12:25:47Z

clippy_lints/src/map_unit_fn.rs

+/// or closure that returns the unit type.
+///
+/// **Why is this bad?** Readability, this can be written more clearly with
+/// an if statement


also if let

oli-obk · 2018-04-15T12:26:06Z

clippy_lints/src/map_unit_fn.rs

+declare_clippy_lint! {
+    pub RESULT_MAP_UNIT_FN,
+    complexity,
+    "using `Result.map(f)`, where f is a function or closure that returns ()"


lowercase Result

oli-obk · 2018-04-15T12:29:49Z

clippy_lints/src/map_unit_fn.rs

+            Approx(String)
+        }
+
+        let suggestion = if let Some(expr_span) = reduce_unit_expression(cx, closure_expr) {


just move this if let into the lint closure. That way you don't need the intermediate enum.

phansch · 2018-04-15T13:48:41Z

Should be fine now :)

killercup suggested changes Jan 23, 2017

View reviewed changes

Manishearth reviewed Jan 23, 2017

View reviewed changes

phansch added the E-help-wanted Call for participation: Help is requested to fix this issue. label Apr 7, 2018

phansch changed the title ~~Lint Option.map(f) where f returns nil~~ WIP: Lint Option.map(f) where f returns nil Apr 7, 2018

phansch force-pushed the option_map_nil_fn branch 3 times, most recently from 1ec3aca to d19082a Compare April 9, 2018 06:34

phansch removed the E-help-wanted Call for participation: Help is requested to fix this issue. label Apr 9, 2018

phansch changed the title ~~WIP: Lint Option.map(f) where f returns nil~~ Lint Option.map(f) where f returns nil Apr 9, 2018

oli-obk reviewed Apr 9, 2018

View reviewed changes

phansch force-pushed the option_map_nil_fn branch from d19082a to 85ad541 Compare April 10, 2018 20:14

phansch changed the title ~~Lint Option.map(f) where f returns nil~~ Lint Option.map(f) where f returns unit Apr 14, 2018

phansch changed the title ~~Lint Option.map(f) where f returns unit~~ WIP: Lint Option.map(f) where f returns unit Apr 14, 2018

phansch force-pushed the option_map_nil_fn branch 2 times, most recently from 663d5c8 to 53f1a98 Compare April 15, 2018 09:38

philipturnbull and others added 6 commits April 15, 2018 13:01

Lint Option.map(f) where f returns nil

e5ecbb5

Lint Option.map(f) where f never returns

302f5d0

Lint closures that return nil

30f2480

Return Spans instead of Cow<&str>'s

2f52d1d

Handle non-trivial nil closures

d0bdfe5

`reduce_nil_closure` mixed together a) 'is this a nil closure?' and b) 'can it be reduced to a simple expression?'. Split the logic into two functions so we can still generate a basic warning when the closure can't be simplified.

Make it compile again

991a302

phansch added 9 commits April 15, 2018 13:01

Cleanup misc::check_nan

ca60e8a

This was a bit messed up after a bigger rebase.

Use declare_clippy_lint and 'complexity' category

fbd71f9

Rename lint to option_map_unit_fn

a3ff21f

Rust does not have nil.

Move test to new UI test system

bcc335f

Allow new lint in ui/eta.rs

db60c67

Remove further semicolon reduction

7de707f

Use approximate_suggestion for non-reducible closures

d87385b

Generate let binding variable name for some cases

d54f70f

Given a map call like `x.field.map ...` the suggestion will contain: `if let Some(x_field) ...` Given a map call like `x.map ...` the suggestion will contain: `if let Some(_x) ...` Otherwise it will suggest: `if let Some(_) ...`

Rename option_map_unit_fn to map_unit_fn

8307a89

phansch force-pushed the option_map_nil_fn branch 2 times, most recently from 85befa8 to b61affb Compare April 15, 2018 11:09

Also lint Result.map for unit returns

4f4e20c

phansch force-pushed the option_map_nil_fn branch from b61affb to 4f4e20c Compare April 15, 2018 12:00

phansch changed the title ~~WIP: Lint Option.map(f) where f returns unit~~ Lint Option.map(f) where f returns unit Apr 15, 2018

oli-obk suggested changes Apr 15, 2018

View reviewed changes

Refactor out enum and address nits

26b9911

oli-obk approved these changes Apr 15, 2018

View reviewed changes

oli-obk merged commit ae32137 into rust-lang:master Apr 15, 2018

Lint Option.map(f) where f returns unit #1467

Lint Option.map(f) where f returns unit #1467

Conversation

philipturnbull commented Jan 23, 2017

killercup left a comment • edited

Choose a reason for hiding this comment

killercup Jan 23, 2017 • edited

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

Manishearth 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

Manishearth commented Jan 23, 2017

oli-obk commented Mar 24, 2017

oli-obk commented Jun 9, 2017

philipturnbull commented Jun 9, 2017

oli-obk commented Jun 9, 2017

phansch commented Apr 7, 2018

oli-obk commented Apr 7, 2018

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

phansch commented Apr 10, 2018

oli-obk commented Apr 10, 2018

phansch commented Apr 15, 2018 • edited

oli-obk 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

phansch commented Apr 15, 2018

Lint `Option.map(f)` where f returns unit #1467

Lint `Option.map(f)` where f returns unit #1467

killercup left a comment •

edited

killercup Jan 23, 2017 •

edited

phansch commented Apr 15, 2018 •

edited