diff --git a/CHANGELOG.md b/CHANGELOG.md index f9ed0a51..04bfbb1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # rubocop-github +## Unreleased + +- Added `GitHub/UnreliableSubclasses` cop. Flags `Class#descendants` and `Class#subclasses` when the receiver is a constant. Both happily skip classes that haven't been autoloaded yet. Both also depend on GC timing for dynamically-defined classes, which is great fun in tests. + ## v0.26.0 - Read the automatic release notes on [the /releases page for this gem](https://github.com/github/rubocop-github/releases). diff --git a/STYLEGUIDE.md b/STYLEGUIDE.md index f14d1000..42aed560 100644 --- a/STYLEGUIDE.md +++ b/STYLEGUIDE.md @@ -31,6 +31,8 @@ This is GitHub's Ruby Style Guide, inspired by [RuboCop's guide][rubocop-guide]. 18. [Rails](#rails) 1. [content_for](#content_for) 2. [Instance Variables in Views](#instance-variables-in-views) +19. [Subclasses](#subclasses) + 1. [Avoid Class#descendants and Class#subclasses](#avoid-classdescendants-and-classsubclasses) ## Layout @@ -1096,4 +1098,43 @@ If you need to call a subview that expects an instance variable be set. If possi Unfortunately the only way to get data into a layout template is with instance variables. You can't explicitly pass locals to them. +## Subclasses + +### Avoid `Class#descendants` and `Class#subclasses` + +Skip `Class#descendants` (ActiveSupport) and `Class#subclasses` (Ruby). They might lie to you in two ways: + +* If a class hasn't been autoloaded yet, they don't see it. So your answer depends on what the app happened to touch first. +* GC can drop dynamically defined classes whenever it feels like it. Tests love this one. + +If you really, really need it, add a `rubocop:disable` on the line with a short note so that future-you isn't confused. + +* RuboCop rule: GitHub/UnreliableSubclasses + +``` ruby +class Person < ApplicationRecord +end + +class Employee < Person +end + +# bad +Person.descendants # => maybe [Employee], maybe [], who knows? Not me! I never lost control. +Person.subclasses # => same problem + +# good. Keep an explicit registry +class Person < ApplicationRecord + TYPES = [] + + def self.inherited(subclass) + super + TYPES << subclass + end +end + +Person::TYPES # => [Employee, ...] +``` + +Other alternatives: eager load the dependency tree ([`Rails.application.eager_load!`](https://api.rubyonrails.org/classes/Rails/Application.html#method-i-eager_load-21) in tests, `config.eager_load = true` in prod) or use [`ActiveSupport::DescendantsTracker`](https://api.rubyonrails.org/classes/ActiveSupport/DescendantsTracker.html) directly if you really need the reflection. + [rubocop-guide]: https://github.com/rubocop-hq/ruby-style-guide diff --git a/config/default.yml b/config/default.yml index 613f999c..3e891b0a 100644 --- a/config/default.yml +++ b/config/default.yml @@ -53,6 +53,10 @@ GitHub/AvoidObjectSendWithDynamicMethod: GitHub/InsecureHashAlgorithm: Enabled: true +GitHub/UnreliableSubclasses: + Enabled: true + StyleGuide: https://github.com/github/rubocop-github/blob/main/STYLEGUIDE.md#avoid-classdescendants-and-classsubclasses + Layout/AccessModifierIndentation: Enabled: false StyleGuide: https://github.com/github/rubocop-github/blob/main/STYLEGUIDE.md#user-content-access-modifier-indentation diff --git a/config/default_cops.yml b/config/default_cops.yml index 00b5ea04..28f1adcb 100644 --- a/config/default_cops.yml +++ b/config/default_cops.yml @@ -1,3 +1,7 @@ GitHub/InsecureHashAlgorithm: Description: 'Encourage usage of secure hash algorithms' Enabled: pending + +GitHub/UnreliableSubclasses: + Description: "Avoid `Class#subclasses` and `Class#descendants`; they miss not-yet-autoloaded classes and depend on GC timing." + Enabled: pending diff --git a/lib/rubocop-github.rb b/lib/rubocop-github.rb index 65bc8dc8..9e0a054c 100644 --- a/lib/rubocop-github.rb +++ b/lib/rubocop-github.rb @@ -8,3 +8,4 @@ require "rubocop/cop/github/avoid_object_send_with_dynamic_method" require "rubocop/cop/github/insecure_hash_algorithm" +require "rubocop/cop/github/unreliable_subclasses" diff --git a/lib/rubocop/cop/github/unreliable_subclasses.rb b/lib/rubocop/cop/github/unreliable_subclasses.rb new file mode 100644 index 00000000..28abae7c --- /dev/null +++ b/lib/rubocop/cop/github/unreliable_subclasses.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require "rubocop" + +module RuboCop + module Cop + module GitHub + # Inform people when they reach for Class#subclasses (Ruby) or + # Class#descendants (ActiveSupport). + # + # two reasons these can be unreliable: + # 1. autoload hasn't run yet, so half the tree is invisible + # 2. GC may have eaten dynamically-defined classes (i.e. in test suites) + # + # If you really need it, add a rubocop:disable on the line with a note + # explaining why future-you won't be sad. + class UnreliableSubclasses < Base + MSG = "Avoid `%s` here. It may miss not-yet-autoloaded classes and depends on GC timing. " \ + "Prefer an explicit registry or eager loading." + + RESTRICT_ON_SEND = %i[descendants subclasses].freeze + + # matches Foo.descendants, Foo::Bar.subclasses, self.descendants + # receiver has to be a constant or `self` + def_node_matcher :unreliable_call?, <<~PATTERN + (send {const self} {:descendants :subclasses}) + PATTERN + + # the same thing but with safe navigation operator + def_node_matcher :unreliable_csend?, <<~PATTERN + (csend {const self} {:descendants :subclasses}) + PATTERN + + def on_send(node) + return unless unreliable_call?(node) + + add_offense(node, message: format(MSG, method: node.method_name)) + end + + def on_csend(node) + return unless unreliable_csend?(node) + + add_offense(node, message: format(MSG, method: node.method_name)) + end + end + end + end +end diff --git a/test/test_unreliable_subclasses.rb b/test/test_unreliable_subclasses.rb new file mode 100644 index 00000000..b85e540e --- /dev/null +++ b/test/test_unreliable_subclasses.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require_relative "cop_test" +require "minitest/autorun" +require "rubocop/cop/github/unreliable_subclasses" + +class TestUnreliableSubclasses < CopTest + def cop_class + RuboCop::Cop::GitHub::UnreliableSubclasses + end + + def test_offended_by_descendants_call + offenses = investigate cop, <<~RUBY + ApplicationRecord.descendants + RUBY + assert_equal 1, offenses.size + assert_match(/Avoid `descendants`/, offenses.first.message) + end + + def test_offended_by_subclasses_call + offenses = investigate cop, <<~RUBY + ApplicationRecord.subclasses + RUBY + assert_equal 1, offenses.size + assert_match(/Avoid `subclasses`/, offenses.first.message) + end + + def test_offended_on_namespaced_constant + offenses = investigate cop, <<~RUBY + Foo::Bar::Baz.descendants + RUBY + assert_equal 1, offenses.size + end + + def test_offended_on_self_receiver + # self.subclasses in a class method body is still Class#subclasses + offenses = investigate cop, <<~RUBY + class ApplicationRecord + def self.known_subclasses + self.subclasses + end + end + RUBY + assert_equal 1, offenses.size + end + + def test_offended_when_chained + offenses = investigate cop, <<~RUBY + Tea.descendants.map(&:name) + Tea.subclasses.each { |k| k.foo } + Tea.descendants.size + Tea.subclasses.count + RUBY + assert_equal 4, offenses.size + end + + def test_offended_by_safe_navigation_on_constant + offenses = investigate cop, <<~RUBY + Tea&.descendants + Tea&.subclasses + RUBY + assert_equal 2, offenses.size + end + + def test_unoffended_by_non_constant_receiver + offenses = investigate cop, <<~RUBY + comment.descendants + category.subclasses + tree_node&.descendants + registry[:foo].subclasses + RUBY + assert_equal 0, offenses.size + end + + def test_unoffended_when_called_with_arguments + # arity mismatch + offenses = investigate cop, <<~RUBY + Registry.subclasses(:include_abstract) + Tree.descendants(depth: 2) + RUBY + assert_equal 0, offenses.size + end + + def test_unoffended_by_unrelated_methods + offenses = investigate cop, <<~RUBY + Tea.all + Tea.where(roasted: true) + ApplicationRecord.connection + RUBY + assert_equal 0, offenses.size + end +end