[Fix #6888] Fix an error for Rails/ActiveRecordOverride

[diachini]

Fixes #6888.

This PR fixes the following error for Rails/ActiveRecordOverride
when there is no parent_class to match against.

% cat app/models/user.rb
class User
  def initialize; end

  def create; end
end

% rubocop app/models/user.rb --only Rails/ActiveRecordOverride -d
For /Users/dannyiachini/github/jpw/rubocop_ex: configuration from /Users/dannyiachini/github/jpw/rubocop_ex/.rubocop.yml
Default configuration from /Users/dannyiachini/.gem/ruby/2.5.3/gems/rubocop-0.67.2/config/default.yml
Inspecting 1 file
Scanning /Users/dannyiachini/github/jpw/rubocop_ex/app/models/user.rb
An error occurred while Rails/ActiveRecordOverride cop was inspecting /Users/dannyiachini/github/jpw/rubocop_ex/app/models/user.rb:6:2.
undefined method `const_name' for nil:NilClass
/Users/dannyiachini/.gem/ruby/2.5.3/gems/rubocop-0.67.2/lib/rubocop/cop/rails/active_record_override.rb:42:in `on_def'

(snip)

1 file inspected, no offenses detected

1 error occurred:
An error occurred while Rails/ActiveRecordOverride cop was inspecting /Users/dannyiachini/github/jpw/rubocop_ex/app/models/user.rb:6:2.
Errors are usually caused by RuboCop bugs.
Please, report your problems to RuboCop's issue tracker.
https://github.com/rubocop-hq/rubocop/issues

Mention the following information in the issue report:
0.67.2 (using Parser 2.6.2.0, running on ruby 2.5.3 x86_64-darwin17)

Replace this text with a summary of the changes in your PR.
The more detailed you are, the better.

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• 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 bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[diachini]

• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

Had a configuration problem locally, but am running now and addressing the problems! Sorry for the early-PR submission!

[diachini]

Originally, I had just done:

-                        .include?(parent_class.const_name)
+                        .include?(parent_class&.const_name)

And that led to:

lib/rubocop/cop/rails/active_record_override.rb:42:49: E: Lint/Syntax: unexpected token error
(Using Ruby 2.2 parser; configure using TargetRubyVersion parameter, under AllCops)
                          .include?(parent_class&.const_name)

---

So then changed it to:

-          return unless %w[ApplicationRecord ActiveModel::Base]
+          return unless parent_class && %w[ApplicationRecord ActiveModel::Base]
                           .include?(parent_class.const_name)

And that resulted in:

lib/rubocop/cop/rails/active_record_override.rb:33:9: C: Metrics/AbcSize: Assignment Branch Condition size for on_def is too high. [17.03/17]
        def on_def(node) ...

So that's what got us to this extraction of active_model?

[mikegee]

I wonder if we should add ActiveRecord::Base to this list to handle Rails versions lower than 5.

[bbatsov]

And I'd probably move this list to a constant.

[diachini]

I wonder if we should add ActiveRecord::Base to this list to handle Rails versions lower than 5.

Makes sense to me!

And I'd probably move this list to a constant.

Totally!

https://github.com/rubocop-hq/rubocop/compare/4cc3eac423a8e0f5a1ac10c62f46282221acf790..8d676717302ccba341bf12afa98318b09ec72157

[diachini]

I rebased to fix the merge conflict in CHANGELOG.md, and now CircleCI is getting an error (seemingly) unrelated to my changes:

Fetching byebug 11.0.1
Installing byebug 11.0.1 with native extensions
Gem::RuntimeRequirementNotMetError: byebug requires Ruby version >= 2.3.0. The
current ruby version is 2.2.0.
An error occurred while installing byebug (11.0.1), and Bundler
cannot continue.
Make sure that `gem install byebug -v '11.0.1'` succeeds before bundling.

Any pointers on how to address this?

[bquorning]

CircleCI is getting an error (seemingly) unrelated to my changes

I have seen that error a couple of times, and I’m not sure how to fix it. Perhaps it’s actually a bug in Bundler…

I’ve restarted the build which hopefully will result in a green build.

[bquorning]

I don’t understand the description – this class does not override #save…

[diachini]

Nice catch! I updated to actually be save (for consistency with the other tests in there).

[elebow]

What do you think about finding the class name by recursing upward? That also addresses the module_function case identified by @pdobb at #6888 (comment), and seems more reliable in general than always assuming that the def's parent is the class node.

diff --git a/lib/rubocop/cop/rails/active_record_override.rb b/lib/rubocop/cop/rails/active_record_override.rb
index 1e74a41ef..248511785 100644
--- a/lib/rubocop/cop/rails/active_record_override.rb
+++ b/lib/rubocop/cop/rails/active_record_override.rb
@@ -36,11 +36,8 @@ module RuboCop
           method_name = node.method_name
           return unless BAD_METHODS.include?(node.method_name)
 
-          parent_parts = node.parent.node_parts
-          parent_class = parent_parts.take_while do |part|
-            !part.nil? && part.const_type?
-          end.last
-          return unless active_model?(parent_class)
+          parent_class_name = find_parent_class_name(node)
+          return unless active_model?(parent_class_name)
 
           return unless node.descendants.any?(&:zsuper_type?)
 
@@ -49,9 +46,8 @@ module RuboCop
 
         private
 
-        def active_model?(parent_class)
-          parent_class && ACTIVE_RECORD_CLASSES
-            .include?(parent_class.const_name)
+        def active_model?(parent_class_name)
+          ACTIVE_RECORD_CLASSES.include?(parent_class_name)
         end
 
         def callback_names(method_name)
@@ -67,6 +63,20 @@ module RuboCop
         def message(method_name)
           format(MSG, prefer: callback_names(method_name), bad: method_name)
         end
+
+        def find_parent_class_name(node)
+          return nil unless node
+
+          if node.class_type?
+            parent_class_name = node.node_parts[1]
+
+            return nil if parent_class_name.nil?
+
+            return parent_class_name.source
+          end
+
+          find_parent_class_name(node.parent)
+        end
       end
     end
   end
diff --git a/spec/rubocop/cop/rails/active_record_override_spec.rb b/spec/rubocop/cop/rails/active_record_override_spec.rb
index 8fce8de86..8469b2646 100644
--- a/spec/rubocop/cop/rails/active_record_override_spec.rb
+++ b/spec/rubocop/cop/rails/active_record_override_spec.rb
@@ -41,6 +41,8 @@ RSpec.describe RuboCop::Cop::Rails::ActiveRecordOverride do
   it 'registers an offense when overriding update' do
     expect_offense(<<-RUBY.strip_indent)
       class X < ActiveModel::Base
+        module_function
+
         def update
         ^^^^^^^^^^ Use `before_update`, `around_update`, or `after_update` callbacks instead of overriding the Active Record method `update`.
           super

[fwininger]

@diachini thanks for this PR.

[diachini]

What do you think about finding the class name by recursing upward?

That works for me! Thanks for doing the heavy lifting and sharing the diff @elebow! I incorporated it into the solo commit on here (diff for posterity).