diff --git a/Gemfile.lock b/Gemfile.lock index df08f1cf..58134b9e 100644 --- a/Gemfile.lock +++ b/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) diff --git a/config/default.yml b/config/default.yml index c9b8d30d..711e7f81 100644 --- a/config/default.yml +++ b/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." diff --git a/lib/rubocop-shopify.rb b/lib/rubocop-shopify.rb index 95baaa34..3cfd6c68 100644 --- a/lib/rubocop-shopify.rb +++ b/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" diff --git a/lib/rubocop/cop/lint/proc_case_when.rb b/lib/rubocop/cop/lint/proc_case_when.rb new file mode 100644 index 00000000..54a994d3 --- /dev/null +++ b/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 diff --git a/lib/rubocop/shopify/version.rb b/lib/rubocop/shopify/version.rb index 36e03392..25af74e2 100644 --- a/lib/rubocop/shopify/version.rb +++ b/lib/rubocop/shopify/version.rb @@ -2,6 +2,6 @@ module RuboCop module Shopify - VERSION = "3.0.1" + VERSION = "3.0.2" end end diff --git a/test/fixtures/full_config.yml b/test/fixtures/full_config.yml index 4d4190a3..22c4ad6c 100644 --- a/test/fixtures/full_config.yml +++ b/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 diff --git a/test/rubocop/cop/lint/proc_case_when_test.rb b/test/rubocop/cop/lint/proc_case_when_test.rb new file mode 100644 index 00000000..e7bc6077 --- /dev/null +++ b/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