New Lint/RandOne cop

[DNNX]

It checks for rand(1.0) and similar calls that return 0.

Such calls most likely show an error in the code.

I'm not sure if it is really useful. I came across one real life occurrence though: Andrew8xx8/kirpich#13. I wish GitHub search could be better (more like git grep) so that I could estimate if it is really a common issue.

Benchmarks:

Benchmark.ips do |x|
  x.report('rand(1)') { rand(1) }
  x.report('0') { 0 }
  x.compare!
end
__END__
Calculating -------------------------------------
             rand(1)    48.243k i/100ms
                   0    60.051k i/100ms
-------------------------------------------------
             rand(1)      2.066M (± 1.1%) i/s -     10.372M
                   0      4.608M (± 0.9%) i/s -     23.060M

Comparison:
                   0:  4608288.0 i/s
             rand(1):  2065579.0 i/s - 2.23x slower

[rrosenblum]

This can be rewritten as

rand_call(node) do |rand_argument|
  return unless ONES.include?(rand_argument)
  add_offense(node, :expression, format(MSG, node.source))
end

[DNNX]

@rrosenblum thank you. I'll update the code.

[rrosenblum]

A couple small tweaks and it looks good to me. I am surprised that rand(1) is only 2.23 times slower than hard coding 0.

[alexdowad]

Alternatively, you could include the literals here in the pattern. I'm not sure if we support float literals in node patterns... but I could add it if necessary.

[DNNX]

@alexdowad There are no negative literals (or I didn't manage to find the right syntax). (int -1) doesn't work.

[alexdowad]

OK. Do you want negative literals? It's easy to add them.

[DNNX]

Yes, please :)

On Fri, Jan 8, 2016 at 8:47 AM, Alex Dowad notifications@github.com wrote:

In lib/rubocop/cop/lint/rand_one.rb
#2598 (comment):

•  #
  

•  #   @bad
  

•  #   rand 1
  

•  #   Kernel.rand(-1)
  

•  #   rand 1.0
  

•  #   rand(-1.0)
  

•  #
  

•  #   @good
  

•  #   0
  

•  class RandOne < Cop
  

•    MSG = '`%s` always returns `0`. Perhaps you meant `rand(2)` or `rand`?'
  

•    ONES = [1, -1, 1.0, -1.0]
  

•    def_node_matcher :rand_call, <<-PATTERN
  

•      (send {(const nil :Kernel) nil} :rand {(int $_) (float $_)})
  

•    PATTERN
  

OK. Do you want negative literals? It's easy to add them.

—
Reply to this email directly or view it on GitHub
https://github.com/bbatsov/rubocop/pull/2598/files#r49160027.

[alexdowad]

See PR #2605.

[DNNX]

I am surprised that rand(1) is only 2.23 times slower than hard coding 0.

Me too! I was sure that rand is slow.

[jawshooah]

Should probably be in the Performance namespace, not Lint.

[DNNX]

@jawshooah the reason I put it in Lint is that most likely if people write rand(1), they mean either rand(2) or rand. It's unlikely that someone who wants zero value uses Kernel.rand(1.0). So this cop essentially fixes a points to a program rather than blindly (but technically correctly) replaces rand(1) with 0.

Performance-wise, rand(1), rand(2) and rand are approximately the same, rand being a tiny bit faster.

[DNNX]

Rebased on top of #2605

[bbatsov]

Looks good.

[rrosenblum]

I am surprised that rand(1) is only 2.23 times slower than hard coding 0.

Me too! I was sure that rand is slow.

I wasn't thinking that rand was necessarily slow. I was more thinking the amount of time it takes to run any method I would expect to be an order of magnitude higher than interpreting a literal.

[alexdowad]

I was more thinking the amount of time it takes to run any method I would expect to be an order of magnitude higher than interpreting a literal.

Absolutely. There must be other overhead in the benchmark which is throwing the results out.