Rails/ReflectionClassName gives false positive

[jcoyne]

Expected behavior

It should not mark an error when a string is given

Actual behavior

Describe here what actually happened.

Steps to reproduce the problem

app/models/spotlight/page.rb:13:33: C: Rails/ReflectionClassName: Use a string has value for a class_name.
    belongs_to :last_edited_by, class_name: Spotlight::Engine.config.user_class, optional: true

However Spotlight::Engine.config.user_class is returning a string.

RuboCop version

$ bundle exec rubocop -v
0.64.0

[marcotc]

The problem reported can be fixed (we can allow anything that is not a constant as the class_name) but this raises a different concern around ReflectionClassName:

STR_CONST = 'user'
belongs_to :last_edited_by, class_name: STR_CONST

belongs_to :last_edited_by, class_name: User

These two different declarations of belongs_to cannot be differentiated by Rubocop, they are both constants being passed as class_name.

I'm not sure that this cop can reliably enforce its original intention without side-effects.

[petewalker]

This is also raising an error on:

belongs_to :last_edited_by, class_name: User.to_s

Is this intended?

[Drenmi]

This is also raising an error on:

belongs_to :last_edited_by, class_name: User.to_s

Is this intended?

The rationale of the cop is to avoid triggering auto-loading, so this seems intended, as any reference to the class constant User will do so.

[petewalker]

Ah, that makes sense. If this cop remains, is it worth adding this detail to the rule overview? It currently reads in 0.65:

This cop checks if the value of the option class_name, in the definition of a reflection is a string.

Perhaps we could add:

Referencing a class name directly can trigger an autoload as a side-effect, so Rubocop recommends using a string literal.

[jcoyne]

There is no reason that a literal is required here. Any method or variable that has a string is just as good. The only reason we recommend a string literal is because that's the only case that Rubocop can detect. This to me seems like Rubocop overreaching. You can't solve this category of problem using static analysis. If one wanted to prevent a Class being passed, why not make a PR against activerecord and deprecate anything beside a String.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[floehopper]

There is no reason that a literal is required here. Any method or variable that has a string is just as good. The only reason we recommend a string literal is because that's the only case that Rubocop can detect. This to me seems like Rubocop overreaching. You can't solve this category of problem using static analysis. If one wanted to prevent a Class being passed, why not make a PR against activerecord and deprecate anything beside a String.

I agree with @jcoyne's analysis. I opened this duplicate (sorry!) issue to report the following use case which seems legitimate to me.

class Comment < ApplicationRecord
  belongs_to :parent, class_name: model_name.to_s
end

We're avoiding duplication by using model_name.to_s (which is still a String) vs using the 'Comment' String literal.

[Bhacaz]

If one wanted to prevent a Class being passed, why not make a PR against activerecord and deprecate anything beside a String.

@jcoyne, In Rails 5.2, it's already the case, you cannot pass a Class (take a look at the original PR rails/rails#27551). The cop try to prevent the same think, circular dependencies, where ActiveRecord::Reflection cannot. If you use class_name: User.name or class_name: User.to_s Rails will be fine but you still eagerloading the Class User.

---

class Comment < ApplicationRecord
 belongs_to :parent, class_name: model_name.to_s
end

@floehopper you got a good example where the cop see an offense and there is no danger of circular dependencies. I don't know if it's an edge case or the cop needs to be change so it will not only search for a literal string.

[jcoyne]

@Bhacaz using a class was @petewalker's example. My example was using a string from configuration

[Bhacaz]

@jcoyne Sorry I miss that detail 😕. It's right to flag the issue.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[nachokb]

in my case I'm using a Module Builder, something like

module HasOwner
  module With
    def with(klazz_name)
      Module.new do
        extend ActiveSupport::Concern

        included do
          has_many :xyz,
            as: :owner,
            class_name: klazz_name
        end
      end
    end
  end
  extend With
end

So that it gets used like

class Thing < AR::Base
  include HasOwner.with('Person')
end

NOTE: the real case is more complex and this is a good solution.

Now, 2 things:

1. klazz_name is not a constant;
2. I know it's a String;

[Drenmi]

The only reason we recommend a string literal is because that's the only case that Rubocop can detect. This to me seems like Rubocop overreaching.

The principle being followed is to prefer false positives, since there are an abundance of ways to address those built in. Conversely, there is no way to get at false negatives.