Performance cops

[bbatsov]

Here's a few ideas worth considering https://github.com/JuanitoFatas/fast-ruby

[basex]

and some more https://github.com/DamirSvrtan/fasterer :)

[bbatsov]

Why use many tools to check your code when you can do this with just one? :-)

All performance checks are pretty easy to implement, so we'll definitely add them to RuboCop at some point. Or at least the sensible ones - e.g. it is silly to compare find with bsearch as one requires sorted data.

[basex]

I agree, rubocop could easily have performance checks. I think it's worth when it's pretty clear that it's a performance gain in any implementation of ruby.

[palkan]

Maybe we should extract reasonable idioms from these tools and create issue for each of them?

[bbatsov]

@palkan Yeah, it makes sense to have separate issues. I created this meta-issue just as a quick reminder.

[jdickey]

Why use many tools? Different tools have different purposes; the differences between fast-ruby and [fasterer](https://github.com/DamirSvrtan/fasterer) provide a good example. fast-rubydemonstrates the speed difference between specific pairs of Ruby idioms, providing hard-if-artificial benchmark numbers.fastereranalyses _your_ code and searches for idioms known to be slower than alternatives. (To see a benchmark-driven estimate of _how much_ slower, runfast-ruby`. 😉)

SRP applies to tools, not just methods or classes.

Cluttering up tools with laundry lists of tenuously-related features just introduces bloat and makes the tool easier to break and slower to run. Unix won the war that systems with one-executable-does-everything started precisely because of its use-the-smallest-right-tool-for-the-job mentality; this filtered through into numerous later systems.

Having RuboCop be a tool that evaluates code against the Ruby Style Guide, with configurable overrides, is insanely useful. Adding auto-correct seemed to make sense, but how hard was/is it to get right, and how many users still run without it, preferring to manually update code?

Adding more cruft will do just that; add more cruft to what started out as remarkably cruft-free code. If somebody really wants a single, fire-and-forget tool that calls out your style violations, chops vegetables and washes the dog, then consider a new "meta-tool" that runs what is now RuboCop, the new performance tools, and whatever other chrome tail-fins you care to throw into the box… but make each fully usable as a standalone tool for those who don't share the same preferences (or available runtime resources). And then explain how your new meta-tool has advantages over Rake and Thor and a long list of other tools in the Ruby omniverse that allow you to hook things up, too. (Maybe your meta-tool is a Thor script.)

Don't fix what isn't broken.

[bbatsov]

I understand your concerns, but on the other hand RuboCop currently represents pretty much the ultimate framework for implementing static code analysis in Ruby. Adding more checks adds little performance overhead and next to zero complexity to the codebase overall. I'm not saying we should start checking for uses of each and suggest the use of while; I'm saying we should check for cases when the speedier solution is the more elegant solution.

There will always be room for alternative tools, but when some task is a good fit for RuboCop I don't see why we shouldn't implement it.

P.S. Some of the fast-ruby tips have part of the style guide for ages...

P.P.S. RuboCop stopped being the tool which simply enforced the style guide the moment we added the first configuration option. No amount of extra cops would ever offset the complexity which configuration brought to the table. But on the other hand the rich config options turned RuboCop into the de facto standard Ruby lint tool.

[jdickey]

Fine; I see where you're going with "the ultimate framework [for] implementing static code analysis in Ruby". I think I'd still (moderately) argue for a "meta", or driver, tool driving a set of "feature" executables (existing RuboCop, performance, etc). The counter to that, of course, is KISS. Not sure where you get the PPS from. Configuration merely affects how it does its job(s); not (at a conceptual level) what tasks it performs. But, as I said, fine; I'll just sit back down and enjoy the view. It has been a fantastic ride. 😄

[bbatsov]

I think I'd still (moderately) argue for a "meta", or driver, tool driving a set of "feature" executables (existing RuboCop, performance, etc).

This has actually been in my backburner for at least a year - there's rubocop-rspec and I've planned to extract rubocop-rails. Unfortunately, lately I've been extremely busy with other stuff and I'm not sure when/if this modularization will happen.

The counter to that, of course, is KISS.

I'm probably more fond of KISS (and the SRP) than you are. But my personal preferences often do not agree with other people's preferences (systemd would be case and point). As for the PPS - without config options RuboCop was just a tool which enforced the Ruby style guide and afterwards it because a tool that can potentially enforce any style. A few examples of the added complexity:

• you don't have to deal with the crazy bugs in Ruby yaml parsers
• you don't have to deal with hierarchical config
• things like style collisions are never an issue if there are no different style
• you end up maintaining a lot of extra code which you'll never personally use

[rrosenblum]

I think that RuboCop checking for performance improvements is a great idea. If we really wanted to get nit picky about SRP, then one could argue that RuboCop, as a single gem, currently does too much by checking metrics, lint, sytle, and rails.

I have interest in this, and I would like to start working on a cop or two.

[bbatsov]

@rrosenblum Go for it!

[guilhermesimoes]

Some more ideas: https://github.com/tcopeland/pippi

And 👍 for modularizing RuboCop!

[sferik]

Very happy to see these features being added to RuboCop. Thanks to @JuanitoFatas for taking the time to add these benchmarks to GitHub.

[alexdowad]

Some which might still be added:

• .each_key instead of .keys.each (danger of false positives here??)
• .casecmp instead of .downcase ==
• .start_with instead of =~ (when doing a literal match anchored at the beginning of the string)
• =~ instead of .match
• using .each instead of for loops
• .sort_by { |x| x }
• .merge!, passed a literal hash with only a single key (better to use []=)
• Range#cover? rather than Range#include?
• methods which use &block but don't do anything with the block besides calling it directly
• .select { ... }.any?
• .select { ... }.empty?
• .select { ... }.none?
• .strip.empty?

[bbatsov]

Totally - there's plenty of work to be done here.

[bbatsov]

=~ instead of .match

Btw, there's something else to consider here - some people dislike operators and favour match?, so maybe this should be a style check that's configurable.

using .each instead of for loops

I believe there's already a cop for this.

P.S. Another classic is .rstrip.lstrip. :-)

[rrosenblum]

I have a cop written for keys.each to each_key. I remember running into issues with it because of some class that treats keys and each_key slightly differently, but I can't remember what the issue was. I will do some digging into my branch to see what the issue was.

What is the replacement or more performant form of .strip.empty??

Another one to consider would be .map.select and .map.each instead of select or each by itself.

I have intentions of expanding the functionality of Performance/Size at some point. .count without arguments isn't always safe to replace with size. Range#size can return nil. I don't know how well it will work, but my hope is to somehow use investigate to build a map of variables that we were able to determine their type.

In

a = b.to_a
a.count

we would know that a is an array and .count could safely be replaced with .size

[alexdowad]

.strip.empty? was from pippi -- they suggest using blank? instead. But that doesn't work without Rails... so maybe forget that one.

[palkan]

I think select {...}.wtf? checks can be implemented as a part of Performance/Detect cop (which in its turn can be renamed to Performance/Select).

[alexdowad]

I may have discovered the problem with the .keys.each cop which @rrosenblum mentioned... replacing .keys.each with .each_key is not valid if you modify the hash during iteration.

[rrosenblum]

replacing .keys.each with .each_key is not valid if you modify the hash during iteration.

That sounds very familiar. I think that is the issue that I ran into. I assume that the same issue exists for #each_value.

[alexdowad]

That sounds very familiar. I think that is the issue that I ran into.

Yeah, I've actually given up on .keys.each for that reason.

[bbatsov]

Guess we can close this.