Skip to content

Commit ca46576

Browse files
committed
Add new Performance/ConditionalDefinition cop
There's a well-known pattern of defining methods conditionally, especially in gems: ```ruby def foo if RUBY_VERSION < '4.0.' ... else ... end end ``` If `foo` is a hotspot, its performance can be improved (see benchmarks below) by moving the conditional logic outside the method body definition: ```ruby if RUBY_VERSION < '4.0.' def foo ... end else def foo ... end end ``` I'd like to propose a new `Performance/ConditionalDefinition` cop that detects conditional method definitions, as described above. We can start by matching constant expressions such as `{RUBY_VERSION,RUBY_RELEASE_DATE,...} <op> ...`, and generalize the detection logic later. ```ruby require 'benchmark/ips' class ConditionInBody def foo if RUBY_VERSION >= '3.2' 1 else 2 end end end class ConditionalDefinition if RUBY_VERSION >= '3.2' def foo 1 end else def foo 2 end end end obj1 = ConditionInBody.new obj2 = ConditionalDefinition.new Benchmark.ips do |x| x.report('condition in body') do obj1.foo end x.report('conditional method definition') do obj2.foo end x.compare! end ``` ```bash $ ruby bm.rb ruby 3.4.3 (2025-04-14 revision d0b7e5b6a0) +PRISM [x86_64-linux] Warming up -------------------------------------- condition in body 1.170M i/100ms conditional method definition 2.228M i/100ms Calculating ------------------------------------- condition in body 11.683M (± 2.6%) i/s (85.60 ns/i) - 58.485M in 5.009850s conditional method definition 22.148M (± 1.0%) i/s (45.15 ns/i) - 111.418M in 5.031059s Comparison: conditional method definition: 22148111.6 i/s condition in body: 11682500.0 i/s - 1.90x slower ``` I'm not an expert in YJIT (or any other MRI JIT compiler), but AFAIK even when YJIT determines that `RUBY_VERSION <op> ...` is constant, it still needs to check a guard clause for every method call. So there's still a performance improvement: ```bash $ ruby --yjit bm.rb ruby 3.4.3 (2025-04-14 revision d0b7e5b6a0) +YJIT +PRISM [x86_64-linux] Warming up -------------------------------------- condition in body 1.853M i/100ms conditional method definition 3.356M i/100ms Calculating ------------------------------------- condition in body 22.752M (± 0.4%) i/s (43.95 ns/i) - 114.907M in 5.050444s conditional method definition 51.223M (± 0.4%) i/s (19.52 ns/i) - 258.439M in 5.045467s Comparison: conditional method definition: 51223091.5 i/s condition in body: 22752233.2 i/s - 2.25x slower ``` **NOTE**: This is a WIP, and the cop is not yet ready -- at this point I just want to get feedback on the idea so I can continue working on it.
1 parent d080ec3 commit ca46576

File tree

4 files changed

+129
-0
lines changed

4 files changed

+129
-0
lines changed

config/default.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,13 @@ Performance/ConcurrentMonotonicTime:
8989
Enabled: pending
9090
VersionAdded: '1.12'
9191

92+
Performance/ConditionalDefinition:
93+
Description: >-
94+
Move conditional logic outside of the method body to
95+
improve performance by avoiding repeated evaluation of conditions.
96+
Enabled: pending
97+
VersionAdded: '<<next>>'
98+
9299
Performance/ConstantRegexp:
93100
Description: 'Finds regular expressions with dynamic components that are all constants.'
94101
Enabled: pending
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Performance
6+
# Move conditional logic outside of the method body definition to
7+
# improve performance by avoiding repeated evaluation of constant condition.
8+
#
9+
# @example
10+
# # bad
11+
# def answer
12+
# if RUBY_VERSION < '4.0.'
13+
# 42
14+
# else
15+
# 239
16+
# end
17+
# end
18+
#
19+
# # good
20+
# if RUBY_VERSION < '4.0.'
21+
# def answer
22+
# 42
23+
# end
24+
# else
25+
# def answer
26+
# 239
27+
# end
28+
# end
29+
class ConditionalDefinition < Base
30+
MSG = 'Move conditional logic outside of the method body definition to ' \
31+
'improve performance by avoiding repeated evaluation of constant condition.'
32+
33+
CONSTANTS = Set[
34+
:RUBY_VERSION,
35+
:RUBY_RELEASE_DATE,
36+
:RUBY_PLATFORM,
37+
:RUBY_PATCHLEVEL,
38+
:RUBY_REVISION,
39+
:RUBY_COPYRIGHT,
40+
:RUBY_ENGINE,
41+
:RUBY_ENGINE_VERSION,
42+
:RUBY_DESCRIPTION
43+
].freeze
44+
OPERATORS = Set[:<, :<=, :==, :>=, :>, :=~, :===].freeze
45+
private_constant :CONSTANTS, :OPERATORS
46+
47+
def on_def(node)
48+
return unless dynamic_definition_in_body?(node)
49+
50+
dynamic_definition_in_body?(node) do
51+
add_offense(node)
52+
end
53+
end
54+
55+
# @!method bad_method?(node)
56+
def_node_matcher :dynamic_definition_in_body?, <<~PATTERN
57+
(def _ (...)
58+
(if
59+
{
60+
(send (const nil? CONSTANTS) OPERATORS _)
61+
(send _ OPERATORS (const nil? CONSTANTS))
62+
}
63+
_ _))
64+
PATTERN
65+
end
66+
end
67+
end
68+
end

lib/rubocop/cop/performance_cops.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
require_relative 'performance/collection_literal_in_loop'
1515
require_relative 'performance/compare_with_block'
1616
require_relative 'performance/concurrent_monotonic_time'
17+
require_relative 'performance/conditional_definition'
1718
require_relative 'performance/constant_regexp'
1819
require_relative 'performance/count'
1920
require_relative 'performance/delete_prefix'
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe RuboCop::Cop::Performance::ConditionalDefinition, :config do
4+
%i[
5+
RUBY_VERSION
6+
RUBY_PLATFORM
7+
RUBY_PATCHLEVEL
8+
RUBY_ENGINE
9+
RUBY_DESCRIPTION
10+
].each do |constant|
11+
%i[< <= === >= > =~ ===].each do |operator|
12+
it "registers an offense when using `#{operator}` comparison with `#{constant}` as LHS" do
13+
expect_offense(<<~RUBY)
14+
def foo
15+
^^^^^^^ Move conditional logic outside of the method body definition to improve performance by avoiding repeated evaluation of constant condition.
16+
if #{constant} #{operator} '3.0'
17+
1
18+
else
19+
2
20+
end
21+
end
22+
RUBY
23+
end
24+
25+
it "registers an offense when using `#{operator}` comparison with `#{constant}` as RHS" do
26+
expect_offense(<<~RUBY)
27+
def foo
28+
^^^^^^^ Move conditional logic outside of the method body definition to improve performance by avoiding repeated evaluation of constant condition.
29+
if '3.0' #{operator} #{constant}
30+
1
31+
else
32+
2
33+
end
34+
end
35+
RUBY
36+
end
37+
end
38+
end
39+
40+
it 'does not register an offense when conditional logic is moved outside of method definition' do
41+
expect_no_offenses(<<~RUBY)
42+
if RUBY_VERSION < '4.0.'
43+
def answer
44+
42
45+
end
46+
else
47+
def answer
48+
239
49+
end
50+
end
51+
RUBY
52+
end
53+
end

0 commit comments

Comments
 (0)