Merge pull request #225 from Shopify/emily/t-enum-comparable-cop

Introduce `ForbidComparableTEnum` cop

config/default.yml

@@ -312,3 +312,8 @@ Sorbet/MultipleTEnumValues:
   Description: 'Ensures that all `T::Enum`s have multiple values.'
   Enabled: true
   VersionAdded: 0.8.2
+
+Sorbet/ForbidComparableTEnum:
+  Description: 'Disallows including the `Comparable` module in a `T::Enum`.'
+  Enabled: true
+  VersionAdded: 0.8.2

lib/rubocop/cop/sorbet/mixin/t_enum.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Sorbet
+      # Mixing for writing cops that deal with `T::Enum`s
+      module TEnum
+        extend RuboCop::NodePattern::Macros
+        def initialize(*)
+          @scopes = []
+          super
+        end
+
+        # @!method t_enum?(node)
+        def_node_matcher :t_enum?, <<~PATTERN
+          (class (const...) (const (const nil? :T) :Enum) ...)
+        PATTERN
+
+        def on_class(node)
+          @scopes.push(node)
+        end
+
+        def after_class(node)
+          @scopes.pop
+        end
+
+        private
+
+        def in_t_enum_class?
+          t_enum?(@scopes&.last)
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/sorbet/t_enum/forbid_comparable_t_enum.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Sorbet
+      # Disallow including the `Comparable` module in `T::Enum`.
+      #
+      # @example
+      #
+      #  # bad
+      #  class Priority < T::Enum
+      #    include Comparable
+      #
+      #    enums do
+      #      High = new(3)
+      #      Medium = new(2)
+      #      Low = new(1)
+      #    end
+      #
+      #    def <=>(other)
+      #      serialize <=> other.serialize
+      #    end
+      #  end
+      class ForbidComparableTEnum < Base
+        include TEnum
+
+        MSG = "Do not use `T::Enum` as a comparable object because of significant performance overhead."
+
+        RESTRICT_ON_SEND = [:include, :prepend].freeze
+
+        # @!method mix_in_comparable?(node)
+        def_node_matcher :mix_in_comparable?, <<~PATTERN
+          (send nil? {:include | :prepend} (const nil? :Comparable))
+        PATTERN
+
+        def on_send(node)
+          return unless in_t_enum_class? && mix_in_comparable?(node)
+
+          add_offense(node)
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/sorbet/multiple_t_enum_values.rb → lib/rubocop/cop/sorbet/t_enum/multiple_t_enum_values.rb

@@ -22,35 +22,23 @@ module Sorbet
       #    end
       #  end
       class MultipleTEnumValues < Base
-        def initialize(*)
-          @scopes = []
-          super
-        end
+        include TEnum
 
         MSG = "`T::Enum` should have at least two values."
 
-        # @!method t_enum?(node)
-        def_node_matcher :t_enum?, <<~PATTERN
-          (class (const...) (const (const nil? :T) :Enum) ...)
-        PATTERN
-
         # @!method enums_block?(node)
         def_node_matcher :enums_block?, <<~PATTERN
           (block (send nil? :enums) ...)
         PATTERN
 
         def on_class(node)
-          @scopes.push(node)
+          super
 
           add_offense(node) if t_enum?(node) && node.body.nil?
         end
 
-        def after_class(node)
-          @scopes.pop
-        end
-
         def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler
-          return if @scopes.empty? || !t_enum?(@scopes.last)
+          return unless in_t_enum_class?
           return unless enums_block?(node)
 
           scope = @scopes.last

lib/rubocop/cop/sorbet_cops.rb

@@ -1,7 +1,8 @@
 # frozen_string_literal: true
 
-require_relative "sorbet/mixin/target_sorbet_version.rb"
-require_relative "sorbet/mixin/signature_help.rb"
+require_relative "sorbet/mixin/target_sorbet_version"
+require_relative "sorbet/mixin/t_enum"
+require_relative "sorbet/mixin/signature_help"
 
 require_relative "sorbet/binding_constant_without_type_alias"
 require_relative "sorbet/constants_from_strings"
@@ -18,7 +19,6 @@
 require_relative "sorbet/type_alias_name"
 require_relative "sorbet/obsolete_strict_memoization"
 require_relative "sorbet/buggy_obsolete_strict_memoization"
-require_relative "sorbet/multiple_t_enum_values"
 
 require_relative "sorbet/rbi/forbid_extend_t_sig_helpers_in_shims"
 require_relative "sorbet/rbi/forbid_rbi_outside_of_allowed_paths"
@@ -42,4 +42,7 @@
 require_relative "sorbet/sigils/enforce_sigil_order"
 require_relative "sorbet/sigils/enforce_single_sigil"
 
+require_relative "sorbet/t_enum/forbid_comparable_t_enum"
+require_relative "sorbet/t_enum/multiple_t_enum_values"
+
 require_relative "sorbet/mutable_constant_sorbet_aware_behaviour"

manual/cops.md

@@ -16,6 +16,7 @@ In the following section you find all available cops:
 * [Sorbet/EnforceSignatures](cops_sorbet.md#sorbetenforcesignatures)
 * [Sorbet/EnforceSingleSigil](cops_sorbet.md#sorbetenforcesinglesigil)
 * [Sorbet/FalseSigil](cops_sorbet.md#sorbetfalsesigil)
+* [Sorbet/ForbidComparableTEnum](cops_sorbet.md#sorbetforbidcomparabletenum)
 * [Sorbet/ForbidExtendTSigHelpersInShims](cops_sorbet.md#sorbetforbidextendtsighelpersinshims)
 * [Sorbet/ForbidIncludeConstLiteral](cops_sorbet.md#sorbetforbidincludeconstliteral)
 * [Sorbet/ForbidRBIOutsideOfAllowedPaths](cops_sorbet.md#sorbetforbidrbioutsideofallowedpaths)

manual/cops_sorbet.md

@@ -295,6 +295,33 @@ SuggestedStrictness | `false` | String
 Include | `**/*.{rb,rbi,rake,ru}` | Array
 Exclude | `bin/**/*`, `db/**/*.rb`, `script/**/*` | Array
 
+## Sorbet/ForbidComparableTEnum
+
+Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+--- | --- | --- | --- | ---
+Enabled | Yes | No | 0.8.2 | -
+
+Disallow including the `Comparable` module in `T::Enum`.
+
+### Examples
+
+```ruby
+# bad
+class Priority < T::Enum
+  include Comparable
+
+  enums do
+    High = new(3)
+    Medium = new(2)
+    Low = new(1)
+  end
+
+  def <=>(other)
+    serialize <=> other.serialize
+  end
+end
+```
+
 ## Sorbet/ForbidExtendTSigHelpersInShims
 
 Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

spec/rubocop/cop/sorbet/t_enum/forbid_comparable_t_enum_spec.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+RSpec.describe(RuboCop::Cop::Sorbet::ForbidComparableTEnum, :config) do
+  it "registers an offense when T::Enum includes Comparable" do
+    expect_offense(<<~RUBY)
+      class MyEnum < T::Enum
+        include Comparable
+        ^^^^^^^^^^^^^^^^^^ Do not use `T::Enum` as a comparable object because of significant performance overhead.
+      end
+    RUBY
+  end
+
+  it "registers an offense when T::Enum prepends Comparable" do
+    expect_offense(<<~RUBY)
+      class MyEnum < T::Enum
+        prepend Comparable
+        ^^^^^^^^^^^^^^^^^^ Do not use `T::Enum` as a comparable object because of significant performance overhead.
+      end
+    RUBY
+  end
+
+  it "does not register an offense when T::Enum includes other modules" do
+    expect_no_offenses(<<~RUBY)
+      class MyEnum < T::Enum
+        include T::Sig
+      end
+    RUBY
+  end
+
+  it "does not register an offense when T::Enum includes no other modules" do
+    expect_no_offenses(<<~RUBY)
+      class MyEnum < T::Enum; end
+    RUBY
+  end
+
+  it "does not register an offense when Comparable is included in a nested, non T::Enum class" do
+    expect_no_offenses(<<~RUBY)
+      class MyEnum < T::Enum
+        class Foo
+          include Comparable
+        end
+      end
+    RUBY
+  end
+
+  it "does not register an offense when Comparable is prepended in a nested, non T::Enum class" do
+    expect_no_offenses(<<~RUBY)
+      class MyEnum < T::Enum
+        class Foo
+          prepend Comparable
+        end
+      end
+    RUBY
+  end
+end