Shopify / rubocop-sorbet

A collection of RuboCop rules for Sorbet
MIT License
178 stars 26 forks source link

Discourage misuses of `T::Enum` #219

Closed egiurleo closed 4 months ago

egiurleo commented 5 months ago

While T::Enum provides a low-effort, type-safe way to construct an exhaustive set of values, it is also less performant than implementing the same functionality using data structures from Ruby (e.g. list of constants).

rubocop-sorbet should discourage using T::Enum in ways that exacerbates performance issues or doesn't use T::Enum for its intended purpose.

Potential cops include:


EDIT (2024-04-22): I decided not to create a cop forbidding case statements above a certain size.

After running some benchmarks, it seems that, at runtime, case statements have exponentially decreasing performance whether they're being used with T::Enum values or Ruby constants, and when statically type checking, time spent increases linearly. There isn't an obvious point at which performance drops off in either of these cases, making any cut off we assign totally arbitrary. We can revisit this issue if it comes up in the future.

Screenshot 2024-04-22 at 4 25 43 PM Screenshot 2024-04-22 at 4 28 00 PM

EDIT (2024-04-23): I've just verified that these results are the same with or without YJIT on (thanks @amomchilov for prompting me to do that).

Screenshot 2024-04-23 at 9 08 45 AM
amomchilov commented 5 months ago

it is also less performant than implementing the same functionality using data structures from Ruby (e.g. list of constants)

The most common operation done with enum values is to compare them (either directly with ==, or indirectly via case/when/===)

T::Enum#== and T::Enum#=== can be made just as fast as comparing string/symbol constants (in either case, it just uses the identity-based comparison inherited from Object#==).

I explore this in https://github.com/Shopify/shopify-types/pull/454/files , but had trouble getting CI to validate my changes. Perhaps that's worth exploring first?

I think that change (having #== and #=== only be overridden when T::Configuration.legacy_t_enum_migration_mode? is set) can even be upstreamed into sorbet/sorbet.

egiurleo commented 5 months ago

@amomchilov That's really interesting! I will take a look at this as it ties in well with our Sorbet performance project. I wonder if we could apply the same logic to the <=> method? I did some benchmarks where </> comparisons were 8x slower with T::Enum values than constants, which was pretty wild to me.

amomchilov commented 5 months ago

Hmm, when is <=> even used? T::Enum doesn't even include Comparable