Extract new BuggyObsoleteStrictMemoization cop

[amomchilov]

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.

[amomchilov]

Kinda nice, the ^^^ is now only under the nil part of this, rather than the whole line.

[amomchilov]

For an easier reviewing experience, take a look at the commit list.

I made this by first copying over the ObsoleteStrictMemoization as-is, then making changes to it in a second commit. YoSo you can just review the diff to go from ObsoleteStrictMemoization to BuggyObsoleteStrictMemoization

[andyw8]

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

[amomchilov]

@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.

[Morriar]

Should we add a test showing that the cop doesn't trigger if the lines are separated:

@foo = T.let(@foo, T.nilable(Foo))
bar
@foo ||= Foo.new

[amomchilov]

Good call. Done.

[Morriar]

👍