Introduce ForbidComparableTEnum cop

[egiurleo]

Related to #219. Best read commit-by-commit.

Thanks @sambostock for working on this with me!

Comparing T::Enum values is very slow. This cop adds an offense on any T::Enum class that includes the Comparable module and warns about performance impact.

Performance evaluation

To assess the performance of T::Enum#<=>, I performed a benchmark comparing a few different implementations of comparable T::Enums:

1. A regular T::Enum with no methods redefined
2. A T::Enum with a rank method that returns an integer and a reimplemented <=> method that compares ranks
3. A T::Enum with the <=> method reimplemented to compare serialized values
4. Comparisons between Ruby constants

Here are the benchmark results:

Warming up --------------------------------------
Comparison check on T::Enum
                         3.923k i/100ms
Comparison check on T::Enum with ranks
                         3.474k i/100ms
Comparison check on T::Enum with redefined spaceship method
                         8.327k i/100ms
Comparison check on constants
                        30.295k i/100ms
Calculating -------------------------------------
Comparison check on T::Enum
                         39.374k (± 1.1%) i/s -    200.073k in   5.081946s
Comparison check on T::Enum with ranks
                         34.766k (± 1.1%) i/s -    177.174k in   5.096786s
Comparison check on T::Enum with redefined spaceship method
                         84.247k (± 2.4%) i/s -    424.677k in   5.043881s
Comparison check on constants
                        309.638k (± 0.7%) i/s -      1.575M in   5.087974s

Comparison:
Comparison check on constants:   309637.8 i/s
Comparison check on T::Enum with redefined spaceship method:    84247.0 i/s - 3.68x  slower
Comparison check on T::Enum:    39374.0 i/s - 7.86x  slower
Comparison check on T::Enum with ranks:    34766.3 i/s - 8.91x  slower

Even the most performant implementation of comparable T::Enum (with the redefined <=> method) is 3-4x slower than just comparing constants. If a developer didn't know this, it would be really easy for them to add significant performance overhead to their application.

Click here for the full benchmark code comparing various implementations of comparable T::Enum

# frozen_string_literal: true

require "benchmark/ips"
require "sorbet-runtime"

class Enum < T::Enum
  include Comparable

  enums do
    A = new(0)
    B = new(1)
    C = new(2)
  end
end

class RankedEnum < T::Enum
  include Comparable

  enums do
    A = new
    B = new
    C = new
  end

  def rank
    case self
    when A then 1
    when B then 2
    when C then 3
    end
  end

  def <=>(other)
    return unless other.is_a?(self.class)

    rank <=> other.rank
  end
end

class RedefinedSpaceshipEnum < T::Enum
  include Comparable

  enums do
    A = new(0)
    B = new(1)
    C = new(2)
  end

  def <=>(other)
    return unless other.is_a?(self.class)

    serialize <=> other.serialize
  end
end

CONST_A = 0
CONST_B = 1
CONST_C = 2

arr = Array.new(100) { Random.rand(2) }
enum_values = Enum.values
ranked_enum_values = RankedEnum.values
redefined_spaceship_enum_values = RedefinedSpaceshipEnum.values
constant_values = [CONST_A, CONST_B, CONST_C]

Benchmark.ips do |x|
  x.report("Comparison check on T::Enum") do
    arr.each do |i|
      enum_values[i] <=> Enum::A
    end
  end

  x.report("Comparison check on T::Enum with ranks") do
    arr.each do |i|
      ranked_enum_values[i] <=> RankedEnum::A
    end
  end

  x.report("Comparison check on T::Enum with redefined spaceship method") do
    arr.each do |i|
      redefined_spaceship_enum_values[i] <=> RedefinedSpaceshipEnum::A
    end
  end

  x.report("Comparison check on constants") do
    arr.each do |i|
      constant_values[i] <=> CONST_A
    end
  end

  x.compare!
end

Why is the default implementation so slow?

To figure out why T::Enum#<=> is so slow, I used ruby-prof to profile two of the implementations above -- the default Sorbet implementation and the implementation that redefines #<=>. (I called T::Utils.run_all_sig_blocks before generating the profiles to cut down on sig-related noise in the profile.)

Default implementation of T::Enum#<=>

Implementation with redefined <=> method

As you can see in the call tree generated by ruby-prof, the default Sorbet implementation spends a significant amount of time in UnboundMethod#bind_call, which is Sorbet wrapping the typed spaceship operator method. This adds significant performance overhead.

Even without Sorbet's wrapping, the need to call serialize on the T::Enum values when comparing them adds overhead compared to, say, comparing Ruby constants.

Click here to see the code that generates the profiles above

require "ruby-prof"
require "sorbet-runtime"

class Enum < T::Enum
  include Comparable

  enums do
    A = new(0)
    B = new(1)
    C = new(2)
  end
end

class RedefinedSpaceshipEnum < T::Enum
  include Comparable

  enums do
    A = new(0)
    B = new(1)
    C = new(2)
  end

  def <=>(other)
    return unless other.is_a?(self.class)

    serialize <=> other.serialize
  end
end

arr = Array.new(100) { Random.rand(2) }
enum_values = Enum.values
redefined_spaceship_enum_values = RedefinedSpaceshipEnum.values

T::Utils.run_all_sig_blocks

RubyProf.start

arr.each do |i|
  enum_values[i] <=> Enum::A
end

result = RubyProf.stop

printer = RubyProf::CallStackPrinter.new(result)
f = File.open("enum_with_comparable.html", "w")
printer.print(f)

RubyProf.start

arr.each do |i|
  redefined_spaceship_enum_values[i] <=> RedefinedSpaceshipEnum::A
end

result = RubyProf.stop

printer = RubyProf::CallStackPrinter.new(result)
f = File.open("enum_with_redefined_spaceship.html", "w")
printer.print(f)

RubyProf.start

[egiurleo]

@Morriar Here's the prepend check