Revert "[Fix #5426] Make Rails/InverseOf accept inverse_of: nil to opt-out (#5430)

[bdewater]

This reverts commit b12978c. /cc @wata727 @johnnyshields

ActiveRecord specifically checks for false, not a falsey value, to stop evaluating if the inverse can be found automatically:

https://github.com/rails/rails/blob/d35875b7a3f559155a9378cbe9203b0b8ea580f9/activerecord/lib/active_record/reflection.rb#L647-L652

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

[wata727]

Hi @bdewater, Thank you for this useful cop.

Considering the implementation of can_find_inverse_of_automatically?, this change seems to be correct. However, even using nil, we can get the same behavior as a result.

Example (Rails v5.1.4):

# app/models/author.rb
class Author < ApplicationRecord
  has_many :books
end

# app/models/book.rb
class Book < ApplicationRecord
  belongs_to :author
end

irb(main):003:0> author = Author.first
irb(main):004:0> book = author.books.build
irb(main):006:0> author.equal? book.author
=> true

# app/models/author.rb
class Author < ApplicationRecord
  has_many :books, inverse_of: nil
end

# app/models/book.rb
class Book < ApplicationRecord
  belongs_to :author
end

irb(main):001:0> author = Author.first
irb(main):002:0> book = author.books.build
irb(main):004:0> author.equal? book.author
=> false

Looking at this behavior, I guess that it is not unnatural to be able to use nil for opt-out. 🤔

WDYT?

[bdewater]

Hi @wata727! Thank you for your useful additions to the cop 😄

Yes the end-result is the same in your example, but how we get there is different since can_find_inverse_of_automatically? doesn't return early. I don't have an example ready to demonstrate, but when creating the cop I noticed for polymorphic associations nil could raise an exception here:

https://github.com/rails/rails/blob/d35875b7a3f559155a9378cbe9203b0b8ea580f9/activerecord/lib/active_record/reflection.rb#L237-L243

Which is why I chose to follow exactly what the code told me. I also noticed that the documentation and the tests all use inverse_of: false and not inverse_of: nil.

[wata727]

Thank you for your reply! I agree with your opinion. This change should be reverted.

[wata727]

I would like to leave this example :)

[bdewater]

👍 good call!

[johnnyshields]

@bdewater I use Mongoid, which allows nil.

[bdewater]

Right, but Mongoid is not ActiveRecord or part of Rails. I've never used it so I can't even say if the assumptions the cop makes in options_requiring_inverse_of? are correct for it.

[bbatsov]

The changes look good, but your branch has to be rebased on top of the current master branch due to merge conflicts.

On a related note - it might be good to add some other check for inverse_of: nil, as it seems to me most of the time people are probably using it when they wanted to use inverse_of: false instead.

The documentation of this cop can be extended a bit as well.

[bdewater]

@bbatsov I rebased and addressed your comments 👍

[bbatsov]

Thanks!