Add cop to disallow case structures where all when clauses are proc literals

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    rubocop-shopify (3.0.1)
+    rubocop-shopify (3.0.2)
       lint_roller
       rubocop (~> 1.72, >= 1.72.1)
 

config/default.yml

@@ -2,3 +2,8 @@ Lint/NoReturnInMemoization:
   Enabled: true
   VersionAdded: "3.0.0"
   Description: "Checks for the use of a `return` with a value in `begin..end` blocks in the context of instance variable assignment such as memoization."
+
+Lint/ProcCaseWhen:
+  Enabled: true
+  VersionAdded: "3.0.2"
+  Description: "Checks for `case`/`when` where every `when` is a proc or value literal: each inline proc literal allocates a new `Proc` every time the `case` runs and adds `Proc#call` overhead, for what is just a harder-to-read `if`/`elsif` tree."

lib/rubocop-shopify.rb

@@ -4,3 +4,4 @@
 require "rubocop/shopify/plugin"
 
 require "rubocop/cop/lint/no_return_in_memoization"
+require "rubocop/cop/lint/proc_case_when"

lib/rubocop/cop/lint/proc_case_when.rb

@@ -0,0 +1,140 @@
+# frozen_string_literal: true
+
+require "rubocop"
+
+module RuboCop
+  module Cop
+    module Lint
+      # Checks for `case` statements whose `when` conditions are only proc/lambda
+      # literals and value literals (with at least one proc).
+      #
+      # Such a `case` is just a harder-to-read `if`/`elsif` tree with a needless
+      # performance cost: a `when` clause matches using `pattern === subject`,
+      # and for a proc `Proc#===` is an alias for `Proc#call`. Every inline proc
+      # literal therefore allocates a brand new `Proc` object each time the
+      # `case` is evaluated (on a hot path, once per branch per call) and adds
+      # `Proc#call` indirection, where an `if`/`elsif` allocates nothing. For a
+      # value literal (number, string, symbol, `nil`, `true`, `false`) `===` is
+      # `==`, so the whole statement is exactly equivalent to `if`/`elsif` using
+      # `Proc#call` and `==`. Use `if`/`elsif` instead.
+      #
+      # Constants assigned a proc literal or a value literal *in the same file*
+      # are resolved and treated as such. Constants defined in other files cannot
+      # be resolved: a bare `when SOME_CONST` is indistinguishable from matching
+      # against a class, so those are left alone to avoid flagging idiomatic
+      # `case obj when SomeClass`.
+      #
+      # Cases that use class, range, or regexp patterns are also left alone:
+      # those rely on `===` in ways that read far worse as `if` conditions
+      # (`is_a?`, `cover?`, `match?`), which is exactly what `case` is for.
+      #
+      # @example
+      #
+      #   # bad - every `when` is a proc (a new proc is allocated on each call)
+      #   case value
+      #   when ->(x) { x > 10 } then :big
+      #   when ->(x) { x < 0 }  then :negative
+      #   else :other
+      #   end
+      #
+      #   # bad - only procs and value literals (including value-literal constants)
+      #   NAME = "widget"
+      #   case value
+      #   when NAME             then :named
+      #   when ->(x) { x > 10 } then :big
+      #   end
+      #
+      #   # good - no proc allocation, and easier to read
+      #   if value > 10
+      #     :big
+      #   elsif value < 0
+      #     :negative
+      #   else
+      #     :other
+      #   end
+      #
+      #   # good - a class/range/regexp pattern makes `case` the clearer choice,
+      #   # even alongside a proc.
+      #   case value
+      #   when Integer          then :int
+      #   when ->(x) { x > 10 } then :big
+      #   end
+      class ProcCaseWhen < ::RuboCop::Cop::Base
+        MSG = "Avoid a `case`/`when` where every `when` is a proc or value " \
+          "literal: each proc literal allocates a new `Proc` every time the " \
+          "`case` is evaluated and adds `Proc#call` overhead, and the whole " \
+          "thing is just a harder-to-read `if`/`elsif` tree. Use `if`/`elsif` " \
+          "instead."
+
+        # Matches `->(x) {}`, `lambda {}`, `proc {}` and `Proc.new {}`, including
+        # their numbered-parameter (`_1`) block variants.
+        # @!method proc_literal?(node)
+        def_node_matcher :proc_literal?, <<~PATTERN
+          {
+            ({block numblock} (send nil? {:lambda :proc}) ...)
+            ({block numblock} (send (const {nil? cbase} :Proc) :new) ...)
+          }
+        PATTERN
+
+        def on_new_investigation
+          super
+          @constant_kinds = collect_constant_kinds
+        end
+
+        def on_case(node)
+          # `case` without a subject evaluates each `when` for truthiness rather
+          # than with `===`, so procs there are not used as matchers.
+          return unless node.condition
+
+          kinds = node.when_branches.flat_map(&:conditions).map { |condition| kind_of(condition) }
+          # Require at least one proc; a case of only value literals is a fine
+          # dispatch and out of scope here.
+          return unless kinds.include?(:proc)
+          # Bail out if any condition is something other than a proc or a value
+          # literal (e.g. a class, range, regexp, or an unresolvable constant),
+          # where `case` reads better than the equivalent `if`.
+          return unless kinds.all? { |kind| kind == :proc || kind == :value }
+
+          add_offense(node)
+        end
+
+        private
+
+        # Classifies a `when` condition (or a constant's assigned value) as a
+        # proc, a value literal, or `:other` (anything we should not rewrite).
+        def kind_of(node)
+          # Unwrap a trailing `.freeze` so `FOO = "bar".freeze` counts as a value.
+          node = node.receiver if node.send_type? && node.method?(:freeze) && node.receiver
+
+          if proc_literal?(node)
+            :proc
+          elsif node.basic_literal?
+            :value
+          elsif node.const_type?
+            # `@constant_kinds` is still nil while being built; an unresolved
+            # constant is treated as `:other` (conservative).
+            (@constant_kinds || {}).fetch(node.short_name, :other)
+          else
+            :other
+          end
+        end
+
+        # Maps the short (demodulized) name of every constant assigned in this
+        # file to its kind, so bare `when CONST` references can be resolved.
+        def collect_constant_kinds
+          kinds = {}
+          ast = processed_source.ast
+          return kinds unless ast
+
+          ast.each_node(:casgn) do |casgn|
+            value = casgn.children[2]
+            next unless value
+
+            kinds[casgn.name] = kind_of(value)
+          end
+          kinds
+        end
+      end
+    end
+  end
+end

lib/rubocop/shopify/version.rb

@@ -2,6 +2,6 @@
 
 module RuboCop
   module Shopify
-    VERSION = "3.0.1"
+    VERSION = "3.0.2"
   end
 end

test/fixtures/full_config.yml

@@ -4788,6 +4788,12 @@ Lint/NoReturnInMemoization:
   VersionAdded: 3.0.0
   Description: Checks for the use of a `return` with a value in `begin..end` blocks
     in the context of instance variable assignment such as memoization.
+Lint/ProcCaseWhen:
+  Enabled: true
+  VersionAdded: 3.0.2
+  Description: 'Checks for `case`/`when` where every `when` is a proc or value literal:
+    each inline proc literal allocates a new `Proc` every time the `case` runs and
+    adds `Proc#call` overhead, for what is just a harder-to-read `if`/`elsif` tree.'
 inherit_mode:
   merge:
   - Exclude

test/rubocop/cop/lint/proc_case_when_test.rb

@@ -0,0 +1,213 @@
+# frozen_string_literal: true
+
+require "test_helper"
+require "rubocop/minitest/assert_offense"
+
+module RuboCop
+  module Cop
+    module Lint
+      class ProcCaseWhenTest < ::Minitest::Test
+        include ::RuboCop::Minitest::AssertOffense
+
+        MESSAGE = "Avoid a `case`/`when` where every `when` is a proc or value " \
+          "literal: each proc literal allocates a new `Proc` every time the " \
+          "`case` is evaluated and adds `Proc#call` overhead, and the whole " \
+          "thing is just a harder-to-read `if`/`elsif` tree. Use `if`/`elsif` " \
+          "instead."
+
+        def setup
+          @cop = ProcCaseWhen.new
+        end
+
+        def test_every_when_is_a_lambda_literal_is_an_offense
+          assert_offense(<<~RUBY)
+            case value
+            ^^^^^^^^^^ Lint/ProcCaseWhen: #{MESSAGE}
+            when ->(x) { x > 10 } then :big
+            when ->(x) { x < 0 } then :negative
+            else
+              :other
+            end
+          RUBY
+        end
+
+        def test_lambda_proc_and_proc_new_are_offenses
+          assert_offense(<<~RUBY)
+            case value
+            ^^^^^^^^^^ Lint/ProcCaseWhen: #{MESSAGE}
+            when lambda { |x| x > 10 } then :big
+            when proc { |x| x < 0 } then :negative
+            when Proc.new { |x| x.nil? } then :nil
+            end
+          RUBY
+        end
+
+        def test_numbered_parameter_procs_are_offenses
+          assert_offense(<<~RUBY)
+            case value
+            ^^^^^^^^^^ Lint/ProcCaseWhen: #{MESSAGE}
+            when proc { _1 > 10 } then :big
+            end
+          RUBY
+        end
+
+        def test_single_when_listing_multiple_procs_is_an_offense
+          assert_offense(<<~RUBY)
+            case value
+            ^^^^^^^^^^ Lint/ProcCaseWhen: #{MESSAGE}
+            when ->(x) { x > 10 }, ->(x) { x < 0 } then :extreme
+            end
+          RUBY
+        end
+
+        def test_procs_mixed_with_value_literals_are_offenses
+          assert_offense(<<~RUBY)
+            case value
+            ^^^^^^^^^^ Lint/ProcCaseWhen: #{MESSAGE}
+            when 0 then :zero
+            when "big", :huge then :large
+            when ->(x) { x > 10 } then :big
+            end
+          RUBY
+        end
+
+        def test_single_when_mixing_proc_and_value_literal_is_an_offense
+          assert_offense(<<~RUBY)
+            case value
+            ^^^^^^^^^^ Lint/ProcCaseWhen: #{MESSAGE}
+            when nil, ->(x) { x > 10 } then :thing
+            end
+          RUBY
+        end
+
+        def test_proc_mixed_with_in_file_value_literal_constant_is_an_offense
+          assert_offense(<<~RUBY)
+            PERSONAL_AGENT = "personal_agent"
+            INTERNAL_SOURCES = ["a"].to_set.freeze
+
+            def bucket_for(source)
+              case source
+              ^^^^^^^^^^^ Lint/ProcCaseWhen: #{MESSAGE}
+              when PERSONAL_AGENT then :personal
+              when ->(s) { INTERNAL_SOURCES.include?(s) } then :internal
+              else :other
+              end
+            end
+          RUBY
+        end
+
+        def test_proc_stored_in_in_file_constant_is_an_offense
+          assert_offense(<<~RUBY)
+            MATCHER = ->(s) { s.positive? }
+            case value
+            ^^^^^^^^^^ Lint/ProcCaseWhen: #{MESSAGE}
+            when MATCHER then :m
+            when 0 then :zero
+            end
+          RUBY
+        end
+
+        def test_proc_mixed_with_unresolvable_cross_file_constant_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            case value
+            when EXTERNAL_CONST then :x
+            when ->(x) { x > 10 } then :big
+            end
+          RUBY
+        end
+
+        def test_proc_mixed_with_in_file_range_constant_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            RANGE = 1..10
+            case value
+            when RANGE then :small
+            when ->(x) { x > 100 } then :big
+            end
+          RUBY
+        end
+
+        def test_proc_mixed_with_class_pattern_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            case value
+            when Integer then :int
+            when ->(x) { x > 10 } then :big
+            end
+          RUBY
+        end
+
+        def test_proc_mixed_with_range_pattern_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            case value
+            when 1..10 then :small
+            when ->(x) { x > 100 } then :big
+            end
+          RUBY
+        end
+
+        def test_proc_mixed_with_regexp_pattern_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            case value
+            when /\\Aadmin/ then :admin
+            when ->(x) { x.empty? } then :blank
+            end
+          RUBY
+        end
+
+        def test_single_when_mixing_proc_and_class_pattern_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            case value
+            when Integer, ->(x) { x > 10 } then :thing
+            end
+          RUBY
+        end
+
+        def test_case_without_any_procs_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            case value
+            when Integer then :int
+            when 1..10 then :small
+            when 42 then :answer
+            end
+          RUBY
+        end
+
+        def test_case_of_only_value_literals_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            case value
+            when 0 then :zero
+            when 1 then :one
+            end
+          RUBY
+        end
+
+        def test_case_without_a_subject_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            case
+            when ->(x) { x > 10 } then :big
+            end
+          RUBY
+        end
+
+        def test_case_in_pattern_matching_is_not_an_offense
+          # `case`/`in` builds a `case_match` node, not a `case` node, so this
+          # cop (which only hooks `on_case`) must not fire even though a proc
+          # pattern is a valid value pattern that matches via `===`.
+          assert_no_offenses(<<~RUBY)
+            case value
+            in ->(x) { x > 10 } then :big
+            in ->(x) { x < 0 } then :negative
+            end
+          RUBY
+        end
+
+        def test_non_proc_literal_block_is_not_an_offense
+          assert_no_offenses(<<~RUBY)
+            case value
+            when build_matcher { |x| x > 10 } then :big
+            end
+          RUBY
+        end
+      end
+    end
+  end
+end