Refactor Hash#transform_values and Hash#transform_values!

[sferik]

This patch makes two small changes to the new Active Support core extensions Hash#transform_values and Hash#transform_values!:

1. These methods now return an Enumerator if no block is given. This makes them more consistent with other Enumerable methods and allows for method chaining, for example:

   {a: 1, b: 2, c: 3}.transform_values.with_index{|x, i| [x * x, i]}
   #=> {:a=>[1, 0], :b=>[4, 1], :c=>[9, 2]}

   Before this patch, such chaining would result in a LocalJumpError: no block given.

2. Since the &block parameter is never referenced, I’ve removed it from Hash#transform_values. This prevents a garbage Proc object from being constructed automatically on method invocation, resulting in measurably faster performance. Here is the benchmark I ran:

   require 'benchmark/ips'
   
   class Hash
     def slow(&block)
       return enum_for(:slow) unless block_given?
       result = self.class.new
       each do |key, value|
         result[key] = yield(value)
       end
       result
     end
   
     def fast
       return enum_for(:fast) unless block_given?
       result = self.class.new
       each do |key, value|
         result[key] = yield(value)
       end
       result
     end
   end
   
   HASH = {a: 1, b: 2, c: 3}
   
   Benchmark.ips do |x|
     x.report("slow") { HASH.slow { |x| x * x } }
     x.report("fast") { HASH.fast { |x| x * x } }
   end

   slow 388660.2 (±10.6%) i/s - 1931275 in 5.030443s
   fast 621865.9 (±11.5%) i/s - 3085425 in 5.029155s
   

References #15819. cc/ @guilleiguaran @rafaelfranca @sgrif

[matthewd]

Should we have a test for the enum_for behaviour? What's the precedent in our other Enumerable extensions?

[sferik]

I should also mention that I considered refactoring Hash#transform_values to:

def transform_values(&block)
  dup.transform_values!(&block)
end

This would be more DRY but it also incurs a performance penalty (from duping) that I decided was not a worthwhile tradeoff.

require 'benchmark/ips'

class Hash
  def slow(&block)
    dup.transform_values!(&block)
  end

  def fast(&block)
    return enum_for(:fast) unless block_given?
    result = self.class.new
    each do |key, value|
      result[key] = yield(value)
    end
    result
  end

  def transform_values!
    return enum_for(:transform_values!) unless block_given?
    each do |key, value|
      self[key] = yield(value)
    end
  end
end

HASH = {a: 1, b: 2, c: 3}

Benchmark.ips do |x|
  x.report("slow") { HASH.slow { |x| x * x } }
  x.report("fast") { HASH.fast { |x| x * x } }
end

slow 297345.7 (±10.5%) i/s - 1473507 in 5.009120s
fast 396055.9 (±11.1%) i/s - 1967210 in 5.030581s

[sferik]

@matthewd Regardless of precedent, I’d argue that returning an Enumerable is the better behavior. I’ll gladly update Hash#transform_keys and similar methods to make them all consistent. I’ll also happy add tests for this behavior.

[matthewd]

@sferik oh, yes, I was just wondering about precedent for whether we bother writing a test for it; the behaviour itself is desirable.

I believe the dup-based spelling, which was abandoned for performance, was the source of the unused '&block' wart, incidentally.

[schneems]

Great work 👍

[sferik]

@matthewd I’ve added a couple tests and update the other extensions to Hash for consistency.

[matthewd]

This sounds like it would have implications in the vicinity of HWIA...

[sferik]

Could you please state those implications in the form of a failing test?

[matthewd]

I was more just expressing subtle surprise that we didn't already have a test that expected HWIA#transform_keys to return a plain Hash. I have no actual objection to the more consistent behaviour.

[sgrif]

LGTM