New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from Apr 15, 2018

Conversation

Projects
None yet
6 participants
@philipturnbull
Contributor

philipturnbull commented Jan 23, 2017

This fixes #1352.

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

@killercup

killercup suggested changes Jan 23, 2017 edited

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.)

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

This comment has been minimized.

@killercup

killercup Jan 23, 2017

Contributor

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

This comment has been minimized.

@Manishearth

Manishearth Jan 23, 2017

Collaborator

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.

This comment has been minimized.

@philipturnbull

philipturnbull Jan 24, 2017

Contributor

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

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

This comment has been minimized.

@killercup

killercup Jan 23, 2017

Contributor

I'd put blank lines around code blocks

// 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

This comment has been minimized.

@killercup

killercup Jan 23, 2017

Contributor

could probably be a doc comment with three slashes

@Manishearth

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

@@ -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

This comment has been minimized.

@Manishearth

Manishearth Jan 23, 2017

Collaborator

It's called unit.

README.md Outdated
@@ -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

This comment has been minimized.

@Manishearth

Manishearth Jan 23, 2017

Collaborator

"Function or closure returning unit"

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

This comment has been minimized.

@Manishearth

Manishearth Jan 23, 2017

Collaborator

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

This comment has been minimized.

@llogiq

llogiq Jan 23, 2017

Collaborator

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.

This comment has been minimized.

@philipturnbull

philipturnbull Jan 24, 2017

Contributor

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

This comment has been minimized.

@llogiq

llogiq Jan 24, 2017

Collaborator

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

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

This comment has been minimized.

@Manishearth

Manishearth Jan 23, 2017

Collaborator

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

// 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) {

This comment has been minimized.

@Manishearth

Manishearth Jan 23, 2017

Collaborator

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

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

This comment has been minimized.

@Manishearth

Manishearth Jan 23, 2017

Collaborator

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

This comment has been minimized.

Collaborator

Manishearth commented Jan 23, 2017

@killercup

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

Feel free to review clippy PRs 😄

@oli-obk

This comment has been minimized.

Collaborator

oli-obk commented Mar 24, 2017

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

@oli-obk

This comment has been minimized.

Collaborator

oli-obk commented Jun 9, 2017

Friendly ping @philipturnbull

@philipturnbull

This comment has been minimized.

Contributor

philipturnbull commented Jun 9, 2017

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

This comment has been minimized.

Collaborator

oli-obk commented Jun 9, 2017

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 phansch added the help-wanted label Apr 7, 2018

@phansch

This comment has been minimized.

Collaborator

phansch commented Apr 7, 2018

@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

This comment has been minimized.

Collaborator

oli-obk commented Apr 7, 2018

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

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

@phansch phansch removed the help-wanted label Apr 9, 2018

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

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

This comment has been minimized.

@oli-obk

oli-obk Apr 9, 2018

Collaborator

Is it possible to make this suggestion contain the expression?

This comment has been minimized.

@phansch

phansch Apr 10, 2018

Collaborator

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.

This comment has been minimized.

@phansch

phansch Apr 12, 2018

Collaborator

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.

This comment has been minimized.

@oli-obk

oli-obk Apr 13, 2018

Collaborator

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

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

This comment has been minimized.

@oli-obk

oli-obk Apr 9, 2018

Collaborator

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

This comment has been minimized.

@phansch

phansch Apr 10, 2018

Collaborator

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?

This comment has been minimized.

@oli-obk

oli-obk Apr 11, 2018

Collaborator

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

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

This comment has been minimized.

@oli-obk

oli-obk Apr 9, 2018

Collaborator

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

This comment has been minimized.

@phansch

phansch Apr 10, 2018

Collaborator

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

This comment has been minimized.

Collaborator

phansch commented Apr 10, 2018

@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

This comment has been minimized.

Collaborator

oli-obk commented Apr 10, 2018

Yes that makes sense

@phansch phansch changed the title from Lint `Option.map(f)` where f returns nil to Lint `Option.map(f)` where f returns unit Apr 14, 2018

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

philipturnbull and others added some commits Jan 16, 2017

Handle non-trivial nil closures
`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.
Cleanup misc::check_nan
This was a bit messed up after a bigger rebase.
Rename lint to option_map_unit_fn
Rust does not have nil.

phansch added some commits Apr 15, 2018

Generate let binding variable name for some cases
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

This comment has been minimized.

Collaborator

phansch commented Apr 15, 2018

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.

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

@oli-obk

Just some nits.

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

This comment has been minimized.

@oli-obk

oli-obk Apr 15, 2018

Collaborator

Write Option in lowercase

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

This comment has been minimized.

@oli-obk

oli-obk Apr 15, 2018

Collaborator

an if let statement

"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

This comment has been minimized.

@oli-obk

oli-obk Apr 15, 2018

Collaborator

lowercase Result

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

This comment has been minimized.

@oli-obk

oli-obk Apr 15, 2018

Collaborator

also if let

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

This comment has been minimized.

@oli-obk

oli-obk Apr 15, 2018

Collaborator

lowercase Result

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

This comment has been minimized.

@oli-obk

oli-obk Apr 15, 2018

Collaborator

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

@phansch

This comment has been minimized.

Collaborator

phansch commented Apr 15, 2018

Should be fine now :)

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment