Use Option.map() instead of match { Some(x) => ..., None => None }

[pythonesque]

We can do this whenever there is no unusual flow in the first branch, which I think shouldn't be too difficult to detect.

[llogiq]

There are a few patterns that could be detected:

match opt { Some(x) => x, None => y } === opt.or(y)
match opt { Some(x) => foo(x), None => y } === opt.map_or(y, foo)
if let Some(x) = opt { foo(x) } === opt.map_or((), foo)
if let Some(x) = opt { x } else { foo() } === opt.unwrap_or_else(foo)

I could find a lot more examples...

[Manishearth]

If let is an okay alternative to map & co; one that everyone uses these days.

-----Original Message-----
From: "llogiq" notifications@github.com
Sent: ‎5/‎23/‎2015 3:17 AM
To: "Manishearth/rust-clippy" rust-clippy@noreply.github.com
Subject: Re: [rust-clippy] Use Option.map() instead of match { Some(x) => ...,None => None } (#6)

There are a few patterns that could be detected:
match opt { Some(x) => x, None => y } === opt.or(y)
match opt { Some(x) => foo(x), None => y } === opt.map_or(y, foo)
if let Some(x) = opt { foo(x) } === opt.map_or((), foo)
if let Some(x) = opt { x } else { foo() } === opt.unwrap_or_else(foo)
I could find a lot more examples...
—
Reply to this email directly or view it on GitHub.

[llogiq]

I certainly agree that it's a matter of style, not correctness. Even matching, while sometimes cumbersome is certainly not a "problem" per se. The interesting question is: Do we help people if we steer them towards map & co? If so, the lint should be as general as possible.

A lint that detects the single case where someone re-implements map probably doesn't pull its weight. So where do we draw the line? Is if let more readable than unwrap_or? (I'm actually on the edge regarding this) Is a match more readable than a map_or?

[Manishearth]

Methods taking closures can be hard to read, but full blown match is also verbose.

Style wise, if let ~> map >> match

-----Original Message-----
From: "llogiq" notifications@github.com
Sent: ‎5/‎23/‎2015 3:28 AM
To: "Manishearth/rust-clippy" rust-clippy@noreply.github.com
Cc: "Manish Goregaokar" manishsmail@gmail.com
Subject: Re: [rust-clippy] Use Option.map() instead of match { Some(x) => ...,None => None } (#6)

I certainly agree that it's a matter of style, not correctness. Even matching, while sometimes cumbersome is certainly not a "problem" per se. The interesting question is: Do we help people if we steer them towards map & co? If so, the lint should be as general as possible.
A lint that detects the single case where someone re-implements map probably doesn't pull its weight. So where do we draw the line? Is if let more readable than unwrap_or? (I'm actually on the edge regarding this) Is a match more readable than a map_or?
—
Reply to this email directly or view it on GitHub.

[llogiq]

Methods taking closures can be hard to read

People coming from functional languages would probably disagree here. Me, I'm fine with both if let and map. Also, if we have a suitable fn, no closure is even needed. So, we could at least match and suggest:

if (opt.is_some()) { opt.unwrap() } else { y } === opt.unwrap_or(y)

match opt { Some(x) => x, None => y } === opt.unwrap_or(y)
match opt { Some(x) => x, None => foo() } === opt.unwrap_or_else(foo)
match opt { Some(x) => Some(foo(x)), None => None } === opt.map(foo)

I'm not completely sure if we should suggest the following, as having two parameters, one of which may be a closure, can impede readability:

match opt { Some(x) => foo(x), None => y } === opt.map_or(y, foo)
match opt { Some(x) => foo(x), None => bar() } === opt.map_or_else(bar, foo)

[Manishearth]

People coming from functional languages would probably disagree here.

Yeah, but this isn't a universal style rule. if let over .map() is a personal choice, but if a match can be converted to one of those, it should.

 match opt { Some(x) => foo(x), None => y } === opt.map_or(y, foo)
 match opt { Some(x) => foo(x), None => bar() } === opt.map_or_else(bar, foo)

Perhaps test that y is a simple expression here?

The rest sound good to me. unwrap_or might also benefit from being checked as a simple expression.

A rough definition of simple is either an ident (well, path) a function or method call, with all paths as args.

[Manishearth]

(Checking for simple exprs because match is way more readable when you have a block of code)

[llogiq]

Agreed. This is why I wrote x/y and foo(_), bar(): The former denote local vars (i.e. Paths), while the latter are Method/Function calls with a fitting number of args — this way we avoid suggesting closures, which may be off-putting to newbies, but get the cases in which there are benefits.

I think we may want to figure out the types of the args, too, and insert as_ref` where appropriate.

[kitsonk]

Don't know if it is best to comment here or create a new issue, but #6573 is overmatching some patterns that can't be converted to a .map easily... for example it is suggesting that this:

let base_url = match args.base_url {
  Some(base_url) => Some(Url::parse(&base_url)?), 
  _ => None,
};

be refactored to:

let base_url = args.base_url.map(|base_url| Url::parse(&base_url)?);

Which doesn't work because of the result unwrap, and there is no easy way to refactor this as a map and preserve the result unwrapping in the containing function.