rubocop / rubocop-performance

An extension of RuboCop focused on code performance checks.
https://docs.rubocop.org/rubocop-performance
MIT License
686 stars 81 forks source link

Cop idea: 2.6+ detect `Hash#merge` chain and suggest a single call to merge #447

Closed tagliala closed 8 months ago

tagliala commented 8 months ago

Apologies if this is a duplicate, I've searched for merge chain and I didn't found results

Starting from Ruby 2.6, it is possible to pass multiple hashes to merge

TEST = { a: 'b' }
X = { c: 'd', a: 'd' }
Y = { e: 'f', a: 'e' }

raise if TEST.merge(X).merge(Y) != TEST.merge(X, Y)

%i[ips memory].each do |benchmark|
  Benchmark.send(benchmark) do |x|
    x.report('merge(X).merge(Y)') { TEST.merge(X).merge(Y) }
    x.report('merge(X, Y)') { TEST.merge(X, Y) }
    x.compare!
  end
end
Ruby version: 2.6.10

Comparison (IPS):
         merge(X, Y):  3836168.4 i/s
   merge(X).merge(Y):  2568155.6 i/s - 1.49x  (± 0.00) slower

Comparison (Memory):
         merge(X, Y):        232 allocated
   merge(X).merge(Y):        464 allocated - 2.00x more

Ref:

Describe the solution you'd like

A cop to detect merge chain and suggest to use a single call

Describe alternatives you've considered

Regexp merge.+merge

koic commented 8 months ago

There is concern regarding false positives in scenarios where the Active Record merge method is chained: https://api.rubyonrails.org/classes/ActiveRecord/SpawnMethods.html#method-i-merge

To ensure accurate detection, one approach could be to only consider cases where the receiver is a hash literal. However, this restriction may prevent practical applicability.

While the idea is interesting, these issues might make it difficult to provide as an effective cop.

tagliala commented 8 months ago

Got it, thanks for answering, I see the issues.

I'm going to close here, it is good to have a reference for googlers and searches