Rails/ReflectionClassName doesn't match relations that have an optional scope proc or lambda

[bubaflub]

At work we're upgrading from Rails 5.0 to 5.1 and I just noticed some deprecation warnings:

DEPRECATION WARNING: Passing a class to the `class_name` is deprecated and will raise an ArgumentError in Rails 5.2. It eagerloads more classes than necessary and potentially creates circular dependencies. Please pass the class name as a string: `has_many :redacted, class_name: 'redacted'` (called from <class:Redacted> at redacted.rb:36)

This was surprising since I thought we had fixed all of these with the Rails/ReflectionClassName rubocop. On closer inspection the ones that weren't flagged by the Rails/ReflectionClassName cop all had an optional second parameter that was a proc or lambda that acts as a scope on the relation. See https://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-belongs_to-label-Scopes for more details on what that scope does.

Deleting the proc or lambda caused Rails/ReflectionClassName to correctly flag these relations.

---

Expected behavior

Expected Rails/ReflectionClassName to warn me about relations in my models that had both:

• a proc or lambda scope 2nd parameter
• a String for the class_name option

Actual behavior

Rails/ReflectionClassName didn't warn about these lines.

Steps to reproduce the problem

Files:

Gemfile

gem 'rubocop-rails'

Gemfile.lock

GEM
  specs:
    activesupport (5.1.7)
      concurrent-ruby (~> 1.0, >= 1.0.2)
      i18n (>= 0.7, < 2)
      minitest (~> 5.1)
      tzinfo (~> 1.1)
    ast (2.4.1)
    concurrent-ruby (1.1.6)
    i18n (1.8.2)
      concurrent-ruby (~> 1.0)
    minitest (5.14.1)
    parallel (1.19.2)
    parser (2.7.1.4)
      ast (~> 2.4.1)
    rack (2.2.3)
    rainbow (3.0.0)
    regexp_parser (1.7.1)
    rexml (3.2.4)
    rubocop (0.89.1)
      parallel (~> 1.10)
      parser (>= 2.7.1.1)
      rainbow (>= 2.2.2, < 4.0)
      regexp_parser (>= 1.7)
      rexml
      rubocop-ast (>= 0.3.0, < 1.0)
      ruby-progressbar (~> 1.7)
      unicode-display_width (>= 1.4.0, < 2.0)
    rubocop-ast (0.3.0)
      parser (>= 2.7.1.4)
    rubocop-rails (2.5.2)
      activesupport
      rack (>= 1.1)
      rubocop (>= 0.72.0)
    ruby-progressbar (1.10.1)
    thread_safe (0.3.6)
    tzinfo (1.2.7)
      thread_safe (~> 0.1)
    unicode-display_width (1.7.0)
PLATFORMS

ruby
DEPENDENCIES

rubocop-rails
BUNDLED WITH

1.16.6

.rubocop.yml

require:
  - rubocop-rails
Rails:

Enabled: true

app/models/bar.rb

class Bar
end

app/models/foo.rb

class Foo < ActiveRecord::Base
  belongs_to :bar_1, class_name: 'Bar'
  belongs_to :bar_2, class_name: Bar
  belongs_to :bar_3, -> { }, class_name: 'Bar'
  belongs_to :bar_4, -> { }, class_name: Bar
end

rubocop output:

bundle exec rubocop --only Rails/ReflectionClassName app/models/foo.rb
The following cops were added to RuboCop, but are not configured. ...
...
Inspecting 1 file
C

Offenses:

app/models/foo.rb:3:22: C: Rails/ReflectionClassName: Use a string value for class_name.
  belongs_to :bar_2, class_name: Bar
                     ^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

I expected the cop to flag both line 3 and line 5 but it doesn't because it won't match against a relation that has a 2nd pararmeter.

RuboCop version

bob@MacBook-Pro truelink % bundle exec rubocop -V
0.89.1 (using Parser 2.7.1.4, rubocop-ast 0.3.0, running on ruby 2.5.8 x86_64-darwin19)

[bubaflub]

I believe this will cause the Rails/ReflectionClassName cop to also match when there is a second parameter:

diff --git a/lib/rubocop/cop/rails/reflection_class_name.rb b/lib/rubocop/cop/rails/reflection_class_name.rb
index 28dbfc1dd..336b95fc2 100644
--- a/lib/rubocop/cop/rails/reflection_class_name.rb
+++ b/lib/rubocop/cop/rails/reflection_class_name.rb
@@ -17,7 +17,7 @@ module RuboCop
         MSG = 'Use a string value for `class_name`.'

         def_node_matcher :association_with_reflection, <<~PATTERN
-          (send nil? {:has_many :has_one :belongs_to} _
+          (send nil? {:has_many :has_one :belongs_to} _ _ ?
             (hash <$#reflection_class_name ...>)
           )
         PATTERN

It seems to work on my simple test case in the ticket:

bundle exec rubocop --only Rails/ReflectionClassName app/models/foo.rb
...
Offenses:

app/models/foo.rb:3:22: C: Rails/ReflectionClassName: Use a string value for class_name.
  belongs_to :bar_2, class_name: Bar
                     ^^^^^^^^^^^^^^^
app/models/foo.rb:5:30: C: Rails/ReflectionClassName: Use a string value for class_name.
  belongs_to :bar_4, -> { }, class_name: Bar
                             ^^^^^^^^^^^^^^^

1 file inspected, 2 offenses detected

And it also worked on the remaining instances in the larger code base at work. I'd be happy to open up a PR with this change and some additional test cases if you'd like.

[koic]

Good catch! I'm looking forward to your pull request!

[bubaflub]

@koic Great! I've opened up #334.