Performance::RedundantMerge dislikes implicit self

[supremebeing7]

When running merge! on an implicit self, rubocop raises undefined methodsource' for nil:NilClass`.

Expected behavior

No error

Actual behavior

Error in RuboCop::Cop::Performance::RedundantMerge#to_assignments.

Stacktrace:

undefined method `source' for nil:NilClass
/ruby-2.2.4/gems/rubocop-0.41.2/lib/rubocop/cop/performance/redundant_merge.rb:94:in `block in to_assignments'
/ruby-2.2.4/gems/rubocop-0.41.2/lib/rubocop/cop/performance/redundant_merge.rb:86:in `map'
/ruby-2.2.4/gems/rubocop-0.41.2/lib/rubocop/cop/performance/redundant_merge.rb:86:in `to_assignments'
/ruby-2.2.4/gems/rubocop-0.41.2/lib/rubocop/cop/performance/redundant_merge.rb:28:in `block in on_send'
/ruby-2.2.4/gems/rubocop-0.41.2/lib/rubocop/cop/performance/redundant_merge.rb:64:in `block in each_redundant_merge'

Steps to reproduce the problem

The error occurred because merge! was being called implicitly on self in the class:

merge! AFFILIATE_KEY => affiliate_id if affiliate_id.present?

So the receiver on rubocop/cop/performance/redundant_merge.rb:94 was nil. Changing my code to

self.merge! AFFILIATE_KEY => affiliate_id if affiliate_id.present?

fixes the error, but should not be necessary.

RuboCop version

$ rubocop -V
0.41.2 (using Parser 2.3.1.2, running on ruby 2.2.4 x86_64-darwin14)

Ruby version

$ ruby -v
ruby 2.2.4p230 (2015-12-16 revision 53155) [x86_64-darwin14]

[supremebeing7]

After looking at this a bit, I'm not sure what the best fix for this is.

We could make it so that when rubocop encounters a nil value for receiver, it automatically sets it to :self, but that seems like it could result in some negative unintended consequences. And if that is the best fix, are there other cops that it should be applied to as well? I am not overly familiar with rubocop so I don't know the answer to that, and unfortunately don't have extra time to investigate.

Or, perhaps it's just bad practice to call merge! on an implicit self? I'm open to the possibility that the real problem is actually in my code style...

[supremebeing7]

Maybe I'm misunderstanding, but in the fix in eb54e93, it looks like instead of making sure receiver gets the value of the implicit self, we're just skipping it entirely as part of the block. This doesn't seem like it fixes the problem so much as just avoids it throwing an error? Is that accurate?

I don't mean to disparage, because I don't have an alternative solution, but this does not seem ideal if I'm understanding it correctly.

[Drenmi]

@supremebeing7: I was on the fence for quite a while about this. I haven't seen the construct in production code, so I had to make a judgement call on whether it is more likely that someone is inheriting from Hash, or implementing their own #merge! method. 😅

Since you probably have more experience with this kind of code than I do, would you say that we should treat it the same as a #merge! with an explicit receiver? 😀

[supremebeing7]

@Drenmi I definitely understand. This is a bit of a weird edge case.

This is the first time I've seen code like this in production, and I didn't write it, so I'm unfortunately not terribly familiar with it either 😟 Looking more closely at my codebase, this is the root of the issue:

    delegate :merge!, to: :metadata

    def metadata
      @_metadata ||= {}.with_indifferent_access
    end

So, it's a little odd maybe, but I don't know that rubocop should make a judgment on its oddness (not until there's a cop for that, anyway 😄 ).

I think the goal of the RedundantMerge cop is to check all Hash#merge! calls. With that in mind, it seems to me that this should be treated the same as a #merge! with an explicit receiver, but it should probably also check the implicit receiver to ensure that it, in fact, is_a? Hash.

What do you think? Does that make sense, or is it getting too complicated?