Merge pull request #175 from Shopify/buggy-obsolete-memoization-cop

Extract new `BuggyObsoleteStrictMemoization` cop

config/default.yml

@@ -171,6 +171,23 @@ Sorbet/ObsoleteStrictMemoization:
   Safe: true
   SafeAutoCorrect: true
 
+Sorbet/BuggyObsoleteStrictMemoization:
+  Description: >-
+    Checks for the a mistaken variant of the "obsolete memoization pattern" that used to be required
+    for older Sorbet versions in `#typed: strict` files. The mistaken variant would overwrite the ivar with `nil`
+    on every call, causing the memoized value to be discarded and recomputed on every call.
+    
+    This cop will correct it to read from the ivar instead of `nil`, which will memoize it correctly.
+    
+    The result of this correction will be the "obsolete memoization pattern", which can further be corrected by
+    the `Sorbet/ObsoleteStrictMemoization` cop.
+    
+    See `Sorbet/ObsoleteStrictMemoization` for more details.
+  Enabled: true
+  VersionAdded: '0.7.3'
+  Safe: true
+  SafeAutoCorrect: false
+
 Sorbet/OneAncestorPerLine:
   Description: 'Enforces one ancestor per call to requires_ancestor'
   Enabled: false

lib/rubocop/cop/sorbet/buggy_obsolete_strict_memoization.rb

@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+require "rubocop"
+
+module RuboCop
+  module Cop
+    module Sorbet
+      # Checks for the a mistaken variant of the "obsolete memoization pattern" that used to be required
+      # for older Sorbet versions in `#typed: strict` files. The mistaken variant would overwrite the ivar with `nil`
+      # on every call, causing the memoized value to be discarded and recomputed on every call.
+      #
+      # This cop will correct it to read from the ivar instead of `nil`, which will memoize it correctly.
+      #
+      # The result of this correction will be the "obsolete memoization pattern", which can further be corrected by
+      # the `Sorbet/ObsoleteStrictMemoization` cop.
+      #
+      # See `Sorbet/ObsoleteStrictMemoization` for more details.
+      #
+      # @safety
+      #   If the computation being memoized had side effects, calling it only once (instead of once on every call
+      #   to the affected method) can be observed, and might be a breaking change.
+      #
+      # @example
+      #   # bad
+      #   sig { returns(Foo) }
+      #   def foo
+      #     # This `nil` is likely a mistake, causing the memoized value to be discarded and recomputed on every call.
+      #     @foo = T.let(nil, T.nilable(Foo))
+      #     @foo ||= some_computation
+      #   end
+      #
+      #   # good
+      #   sig { returns(Foo) }
+      #   def foo
+      #     # This will now memoize the value as was likely intended, so `some_computation` is only ever called once.
+      #     # ⚠️If `some_computation` has side effects, this might be a breaking change!
+      #     @foo = T.let(@foo, T.nilable(Foo))
+      #     @foo ||= some_computation
+      #   end
+      #
+      # @see Sorbet/ObsoleteStrictMemoization
+      class BuggyObsoleteStrictMemoization < RuboCop::Cop::Base
+        include RuboCop::Cop::MatchRange
+        include RuboCop::Cop::Alignment
+        include RuboCop::Cop::LineLengthHelp
+        include RuboCop::Cop::RangeHelp
+        extend AutoCorrector
+
+        include TargetSorbetVersion
+
+        MSG = "This might be a mistaken variant of the two-stage workaround that used to be needed for memoization in "\
+          "`#typed: strict` files. See https://sorbet.org/docs/type-assertions#put-type-assertions-behind-memoization."
+
+        # @!method buggy_legacy_memoization_pattern?(node)
+        def_node_matcher :buggy_legacy_memoization_pattern?, <<~PATTERN
+          (begin
+            ...                                                       # Ignore any other lines that come first.
+            (ivasgn $_ivar                                           # First line: @_ivar = ...
+              (send                                                   # T.let(_ivar, T.nilable(_ivar_type))
+                (const {nil? cbase} :T) :let
+                $nil
+                (send (const {nil? cbase} :T) :nilable _ivar_type))) # T.nilable(_ivar_type)
+            (or-asgn (ivasgn _ivar) _initialization_expr))          # Second line: @_ivar ||= _initialization_expr
+        PATTERN
+
+        def on_begin(node)
+          buggy_legacy_memoization_pattern?(node) do |ivar, nil_node|
+            add_offense(nil_node) do |corrector|
+              corrector.replace(
+                range_between(nil_node.source_range.begin_pos, nil_node.source_range.end_pos),
+                ivar,
+              )
+            end
+          end
+        end
+
+        def relevant_file?(file)
+          super && sorbet_enabled?
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/sorbet/mixin/target_sorbet_version.rb

@@ -11,21 +11,25 @@ def included(target)
         end
 
         module ClassMethods
-          # The version of the Sorbet static type checker required by this cop
+          # Sets the version of the Sorbet static type checker required by this cop
           def minimum_target_sorbet_static_version(version)
             @minimum_target_sorbet_static_version = Gem::Version.new(version)
           end
 
-          def support_target_sorbet_static_version?(version)
+          def supports_target_sorbet_static_version?(version)
             @minimum_target_sorbet_static_version <= Gem::Version.new(version)
           end
         end
 
+        def sorbet_enabled?
+          !target_sorbet_static_version_from_bundler_lock_file.nil?
+        end
+
         def enabled_for_sorbet_static_version?
           sorbet_static_version = target_sorbet_static_version_from_bundler_lock_file
           return false unless sorbet_static_version
 
-          self.class.support_target_sorbet_static_version?(sorbet_static_version)
+          self.class.supports_target_sorbet_static_version?(sorbet_static_version)
         end
 
         def target_sorbet_static_version_from_bundler_lock_file

lib/rubocop/cop/sorbet/obsolete_strict_memoization.rb

@@ -54,7 +54,7 @@ class ObsoleteStrictMemoization < RuboCop::Cop::Base
             $(ivasgn $_ivar                                           # First line: @_ivar = ...
               (send                                                   # T.let(_ivar, T.nilable(_ivar_type))
                 $(const {nil? cbase} :T) :let
-                {(ivar _ivar) nil}
+                (ivar _ivar)
                 (send (const {nil? cbase} :T) :nilable $_ivar_type))) # T.nilable(_ivar_type)
             $(or-asgn (ivasgn _ivar) $_initialization_expr))          # Second line: @_ivar ||= _initialization_expr
         PATTERN

lib/rubocop/cop/sorbet_cops.rb

@@ -15,6 +15,7 @@
 require_relative "sorbet/redundant_extend_t_sig"
 require_relative "sorbet/type_alias_name"
 require_relative "sorbet/obsolete_strict_memoization"
+require_relative "sorbet/buggy_obsolete_strict_memoization"
 
 require_relative "sorbet/rbi/forbid_extend_t_sig_helpers_in_shims"
 require_relative "sorbet/rbi/forbid_rbi_outside_of_allowed_paths"

manual/cops.md

@@ -7,6 +7,7 @@ In the following section you find all available cops:
 
 * [Sorbet/AllowIncompatibleOverride](cops_sorbet.md#sorbetallowincompatibleoverride)
 * [Sorbet/BindingConstantWithoutTypeAlias](cops_sorbet.md#sorbetbindingconstantwithouttypealias)
+* [Sorbet/BuggyObsoleteStrictMemoization](cops_sorbet.md#sorbetbuggyobsoletestrictmemoization)
 * [Sorbet/CallbackConditionalsBinding](cops_sorbet.md#sorbetcallbackconditionalsbinding)
 * [Sorbet/CheckedTrueInSignature](cops_sorbet.md#sorbetcheckedtrueinsignature)
 * [Sorbet/ConstantsFromStrings](cops_sorbet.md#sorbetconstantsfromstrings)

manual/cops_sorbet.md

@@ -41,6 +41,44 @@ FooOrBar = T.any(Foo, Bar)
 FooOrBar = T.type_alias { T.any(Foo, Bar) }
 ```
 
+## Sorbet/BuggyObsoleteStrictMemoization
+
+Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+--- | --- | --- | --- | ---
+Enabled | Yes | Yes (Unsafe) | 0.7.3 | -
+
+Checks for the a mistaken variant of the "obsolete memoization pattern" that used to be required
+for older Sorbet versions in `#typed: strict` files. The mistaken variant would overwrite the ivar with `nil`
+on every call, causing the memoized value to be discarded and recomputed on every call.
+
+This cop will correct it to read from the ivar instead of `nil`, which will memoize it correctly.
+
+The result of this correction will be the "obsolete memoization pattern", which can further be corrected by
+the `Sorbet/ObsoleteStrictMemoization` cop.
+
+See `Sorbet/ObsoleteStrictMemoization` for more details.
+
+### Examples
+
+```ruby
+# bad
+sig { returns(Foo) }
+def foo
+  # This `nil` is likely a mistake, causing the memoized value to be discarded and recomputed on every call.
+  @foo = T.let(nil, T.nilable(Foo))
+  @foo ||= some_computation
+end
+
+# good
+sig { returns(Foo) }
+def foo
+  # This will now memoize the value as was likely intended, so `some_computation` is only ever called once.
+  # ⚠️If `some_computation` has side effects, this might be a breaking change!
+  @foo = T.let(@foo, T.nilable(Foo))
+  @foo ||= some_computation
+end
+```
+
 ## Sorbet/CallbackConditionalsBinding
 
 Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

spec/rubocop/cop/sorbet/buggy_obsolete_strict_memoization_spec.rb

@@ -0,0 +1,165 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe(RuboCop::Cop::Sorbet::BuggyObsoleteStrictMemoization, :config) do
+  let(:specs_without_sorbet) do
+    [
+      Gem::Specification.new("foo", "0.0.1"),
+      Gem::Specification.new("bar", "0.0.2"),
+    ]
+  end
+
+  before(:each) do
+    allow(Bundler).to(receive(:locked_gems)).and_return(
+      Struct.new(:specs).new([
+        *specs_without_sorbet,
+        Gem::Specification.new("sorbet-static", "0.5.10210"),
+      ]),
+    )
+    allow(cop).to(receive(:configured_indentation_width).and_return(2))
+  end
+
+  describe "non-offending cases" do
+    it "the new memoization pattern doesn't register any offense" do
+      expect_no_offenses(<<~RUBY)
+        def foo
+          @foo ||= T.let(Foo.new, T.nilable(Foo))
+        end
+      RUBY
+    end
+
+    describe "the correct obsolete memoization pattern" do
+      it " doesn't register any offense" do
+        expect_no_offenses(<<~RUBY)
+          def foo
+            @foo = T.let(@foo, T.nilable(Foo))
+            @foo ||= Foo.new
+          end
+        RUBY
+      end
+
+      describe "with fully qualified ::T" do
+        it " doesn't register any offense" do
+          expect_no_offenses(<<~RUBY)
+            def foo
+              @foo = ::T.let(@foo, ::T.nilable(Foo))
+              @foo ||= Foo.new
+            end
+          RUBY
+        end
+      end
+
+      describe "with a complex type" do
+        it "doesn't register any offense" do
+          expect_no_offenses(<<~RUBY)
+            def client_info_hash
+              @client_info_hash = T.let(@client_info_hash, T.nilable(T::Hash[Symbol, T.untyped]))
+              @client_info_hash ||= client_info.to_hash
+            end
+          RUBY
+        end
+      end
+
+      describe "with multiline initialization expression" do
+        it "doesn't register any offense" do
+          expect_no_offenses(<<~RUBY)
+            def foo
+              @foo = T.let(@foo, T.nilable(Foo))
+              @foo ||= multiline_method_call(
+                foo,
+                bar,
+                baz,
+              )
+            end
+          RUBY
+        end
+
+        describe "with a gap between the two lines" do
+          it "doesn't register any offense" do
+            expect_no_offenses(<<~RUBY)
+              def foo
+                @foo = T.let(@foo, T.nilable(Foo))
+
+                @foo ||= multiline_method_call(
+                  foo,
+                  bar,
+                  baz,
+                )
+              end
+            RUBY
+          end
+        end
+      end
+
+      describe "with non-empty lines between the two lines" do
+        it "doesn't register any offense" do
+          expect_no_offenses(<<~RUBY)
+            def foo
+              @foo = T.let(@foo, T.nilable(Foo))
+             some_other_computation
+              @foo ||= multiline_method_call(
+                foo,
+                bar,
+                baz,
+              )
+            end
+          RUBY
+        end
+      end
+
+      context "when its not the first line in a method" do
+        it "doesn't register any offense" do
+          expect_no_offenses(<<~RUBY)
+            def foo
+              some
+              other
+              code
+              @foo = T.let(@foo, T.nilable(Foo))
+              @foo ||= Foo.new
+            end
+          RUBY
+        end
+      end
+    end
+  end
+
+  describe "a mistaken variant of the obsolete memoization pattern" do
+    context "not using Sorbet" do
+      # If the project is not using Sorbet, the obsolete memoization pattern might be intentional.
+      it "does not register an offence" do
+        allow(Bundler).to(receive(:locked_gems)).and_return(
+          Struct.new(:specs).new(specs_without_sorbet),
+        )
+
+        expect_no_offenses(<<~RUBY)
+          sig { returns(Foo) }
+          def foo
+            @foo = T.let(@foo, T.nilable(Foo))
+            @foo ||= Foo.new
+          end
+        RUBY
+      end
+    end
+
+    it "registers an offence and autocorrects" do
+      # This variant would have been a mistake, which would have caused the memoized value to be discarded
+      # and recomputed on every call. We can fix it up into the working version.
+
+      expect_offense(<<~RUBY)
+        def foo
+          @foo = T.let(nil, T.nilable(Foo))
+                       ^^^ This might be a mistaken variant of the two-stage workaround that used to be needed for memoization in `#typed: strict` files. See https://sorbet.org/docs/type-assertions#put-type-assertions-behind-memoization.
+          @foo ||= Foo.new
+        end
+      RUBY
+
+      expect_correction(<<~RUBY)
+        def foo
+          @foo = T.let(@foo, T.nilable(Foo))
+          @foo ||= Foo.new
+        end
+      RUBY
+    end
+  end
+end