RedundantMerge breaks ActiveRecord objects

[jcoyne]

Performance/RedundantMerge: Use record[:title] = "Test" instead of record.update(title: "Test").

In this case the record object is an instance of an ActiveRecord object. It is not a hash. ActiveRecord::Base#update changes the object in memory and then persists the object, the ActiveRecord::Base#[]= only changes the object in memory

[jaredbeck]

I'm seeing this too. Justin's description is good, but here's an example from my codebase, in case it helps.

Source:

User.find(REDACTED2).update(REDACTED3: true)

Output:

== db/migrate/20100917172144_REDACTED.rb ==
C:  9:  9: Use User.find(REDACTED2)[:REDACTED3] = true instead of 
  User.find(REDACTED2).update(REDACTED3: true).

[lucascaton]

Same here:

# Rubocop ➙ Performance/RedundantMerge:
# Use origin_account[:amount_cents] = origin_account.amount_cents - amount_cents
# instead of origin_account.update amount_cents: origin_account.amount_cents - amount_cents

# My code:
origin_account.update(amount_cents: origin_account.amount_cents - amount_cents)

#2674

[Drenmi]

Yes. I brought this up in the issue when I made the PR. It's a thorny path, and I've been thinking a lot about it since then. It is a Ruby static code analyzer, after all, but the question of how many exceptions should be made on account of popular frameworks might not be that straight forward.

Since we already have Rails specific cops, maybe we can have some way of marking a cop as "Rails unsafe", or, though more complicated, use an alternate cop when the Rails flag i used?

[bbatsov]

The Rails-specific cops will be removed from RuboCop at some point as they don't really belong there. This was a silly mistake I made when I had way more time to work on the project and my ambitions for it were bigger. Cops in general should probably have some "safety" level or something - we're quite aware which cops can produce false positives and cause problems and which can't. This has been proposed several times so far, but we've yet to make any progress in this direction.

[fwininger]

👍 to remove this bad change for Rails Project !

[alexdowad]

maybe we can have some way of marking a cop as "Rails unsafe"

The trouble is that seeing a call to update in no way means that Hash#update is being used, unless we trace backwards to see what possible values for the receiver might be. This would include matching up class definitions to object instantiations (to predict what type an object might have), then matching up method calls to the (likely) corresponding method definitions, as well as predicting what the (likely) return types of the methods are.

This whole process would likely slow RC down quite a bit, and would be unable to conclude anything about code which uses metaprogramming techniques.

This would be a whole new direction for RC, quite distinct from the mostly "coding style"/syntactic checks which it currently performs.

[jawshooah]

This whole process would likely slow RC down quite a bit, and would be unable to conclude anything about code which uses metaprogramming techniques.

It would also be entirely impossible to trace the type of objects returned from calls into an API external to the codebase being linted.

[jaredbeck]

Once 108acca is released, should we give this cop (Performance/RedundantMerge) another chance? My team disabled it entirely.

[bbatsov]

It's released.

[jaredbeck]

I can confirm that rubocop 0.37.1, with Performance/RedundantMerge enabled, no longer has this issue. Thanks Bozhidar!

[fwininger]

Thanks @bbatsov, the version 0.37.1 works fine for our Rails project 👍