[Fix #178] Add new Performance/SelectMap cop

[koic]

Fixes #178 and #193.

This PR adds new Performance/SelectMap cop.

In Ruby 2.7, Enumerable#filter_map has been added.

This cop identifies places where select.map can be replaced by filter_map. And it allows map { ... }.compact that is not compatible with filter_map.

[true, false, nil].compact              #=> [true, false]
[true, false, nil].filter_map(&:itself) #=> [true]

The following is good and bad cases.

# bad
ary.select(&:foo).map(&:bar)
ary.filter(&:foo).map(&:bar)

# good
ary.filter_map { |o| o.bar if o.foo }

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[marcandre]

I'm not sure we should enable this cop by default; in many cases (including the examples with foo and bar above) the resulting code is less clear. I didn't check, but I doubt the performance gain is particularly big (especially in the example for &foo & &bar)

[koic]

Yeah, I have no strong objection to disabling this cop by default. On the other hand, Performance department's cop may prioritize performance over code readability (e.g. A.ancestors.include?(B) is easier to read than A <= B, but A <= B is a good case for Performance/AncestorsInclude cop). In any case, I think this cop can start with disabled by default.

[bbatsov]

I agree that generally it's fine to have the performance cop optimize for speed over clarity. After all that's what they are meant to do, and that's also part of the reason they are not part of the main RuboCop anymore.

[koic]

I think about splitting this cop into two.
The two cases for filter_map are different in compatibility and readability for bad case and good case.

1. Performance/SelectMap cop

# bad
ary.select(&:foo).map(&:bar)
ary.filter(&:foo).map(&:bar)

# good
ary.filter_map { |o| o.bar if o.foo }

• Bad case and good case are compatible behaviour.
• Readability will change, but performance will be prioritized.
• This cop will be renamed and updated this PR. And start the default from pending status, first.

2. Performance/MapCompact cop

# bad
ary.map(&:foo).compact
ary.select(&:foo).compact

# good
ary.filter_map(&:foo)

• Bad case and good case are not compatible behaviour. In other words, it means that safe auto-correction is not possible.
• It will reduce compact method call and improve readability.
• I will open and implement a new PR.

Cc @rubocop/rubocop-core

[koic]

The cop have been split into Performance/SelectMap cop (this PR) and Performance/MapCompact cop (#229), renamed to Performance/SelectMap cop, and changed to pending status.

[Drenmi]

I agree performance cops should be disabled by default, also because many of them depend on VM implementation details, so they may sometimes be slower on a different Ruby version from which it was first tested. As far as I know, no-one keeps track of these micro-optimizations across Ruby versions.

[koic]

many of them depend on VM implementation details, so they may sometimes be slower on a different Ruby version from which it was first tested. As far as I know, no-one keeps track of these micro-optimizations across Ruby versions.

Sure. Since the performance result of Performance cops depends on Ruby implementations, I think it would be better to add to documentation that it is at least based on CRuby.

I agree performance cops should be disabled by default

Which the Performance cops does this refer to? If all the Performance cops were pointed to, rubocop-performance gem itself would be meaningless 😅 I think it will be considered individually whether it can be disabled by default.

To return to this PR topic :-)
I think the Performance/SelectMap cop can be disabled by default again as performance improvements are unclear for user impact. Also, the cop does not have auto-correction, conversion is a little difficult.
On the other hand, Performance/MapCompact cop has unsafe auto-correction and will not compromise readability, so it can be pending by default.