Shopify / rubocop-sorbet

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

Extract new `BuggyObsoleteStrictMemoization` cop #175

Closed amomchilov closed 1 year ago

amomchilov commented 1 year ago

The Sorbet/ObsoleteStrictMemoization cop introduced in #162 was initially meant to detect this kind of pattern:

sig { returns(Foo) }
def foo
  @foo = T.let(@foo, T.nilable(Foo))
  @foo ||= Foo.new
end

@Morriar had the fantastic idea to also detect a mistaken variant:

sig { returns(Foo) }
def foo
  # This would have been a mistake, causing the memoized value to be discarded and recomputed on every call.
  @foo = T.let(nil, T.nilable(Foo))
  #            !!!
  @foo ||= Foo.new
end

This was a great idea and found several such bugs in our program, but wasn't implemented quite right.

Auto-correcting this is dangerous. If the computation being memoized (Foo.new, in this case) had side effects, calling it only once (instead of once on every call to foo) can be observed, and might be a breaking change. This is problematic because Sorbet/ObsoleteStrictMemoization is marked Safe: true and SafeAutoCorrect: true.

This PR splits off this behaviour into a new BuggyObsoleteStrictMemoization cop. The output of this cop is the correct (but still obsolete) memoization pattern. From there, ObsoleteStrictMemoization can be applied to bring it to the modern standard. This cop is marked Safe: true, SafeAutoCorrect: false.

andyw8 commented 1 year ago

I wonder if it should be named BuggyStrictMemoization, since this would still detect problems even if the Obsolete cop wasn't in use?

amomchilov commented 1 year ago

@andyw8 I'm torn on it. It's a bit wordier, but I think it helped with clarity and connecting the two cops. That was the name I originally had, but I liked being able to reference the phrase "obsolete memoization pattern" in docs to help explain it.

"strict memoization pattern" wouldn't be as clear, since there's the old 2-line one, and the new one-liner.