diff --git a/config/default.yml b/config/default.yml index b69275ea..f943701f 100644 --- a/config/default.yml +++ b/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 diff --git a/lib/rubocop/cop/sorbet/buggy_obsolete_strict_memoization.rb b/lib/rubocop/cop/sorbet/buggy_obsolete_strict_memoization.rb new file mode 100644 index 00000000..3e369eda --- /dev/null +++ b/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 diff --git a/lib/rubocop/cop/sorbet/mixin/target_sorbet_version.rb b/lib/rubocop/cop/sorbet/mixin/target_sorbet_version.rb index 9da570d3..c3c10f37 100644 --- a/lib/rubocop/cop/sorbet/mixin/target_sorbet_version.rb +++ b/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 diff --git a/lib/rubocop/cop/sorbet/obsolete_strict_memoization.rb b/lib/rubocop/cop/sorbet/obsolete_strict_memoization.rb index 018a6817..ee5af2c1 100644 --- a/lib/rubocop/cop/sorbet/obsolete_strict_memoization.rb +++ b/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 diff --git a/lib/rubocop/cop/sorbet_cops.rb b/lib/rubocop/cop/sorbet_cops.rb index c9556dcd..83912772 100644 --- a/lib/rubocop/cop/sorbet_cops.rb +++ b/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" diff --git a/manual/cops.md b/manual/cops.md index 45374c58..6946c44e 100644 --- a/manual/cops.md +++ b/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) diff --git a/manual/cops_sorbet.md b/manual/cops_sorbet.md index 23ec09ed..b2addd10 100644 --- a/manual/cops_sorbet.md +++ b/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 diff --git a/spec/rubocop/cop/sorbet/buggy_obsolete_strict_memoization_spec.rb b/spec/rubocop/cop/sorbet/buggy_obsolete_strict_memoization_spec.rb new file mode 100644 index 00000000..2e7d6307 --- /dev/null +++ b/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 diff --git a/spec/rubocop/cop/sorbet/obsolete_strict_memoization_spec.rb b/spec/rubocop/cop/sorbet/obsolete_strict_memoization_spec.rb index c1fe9b5d..fee1eb41 100644 --- a/spec/rubocop/cop/sorbet/obsolete_strict_memoization_spec.rb +++ b/spec/rubocop/cop/sorbet/obsolete_strict_memoization_spec.rb @@ -3,7 +3,7 @@ require "spec_helper" RSpec.describe(RuboCop::Cop::Sorbet::ObsoleteStrictMemoization, :config) do - let(:specs) do + let(:specs_without_sorbet) do [ Gem::Specification.new("foo", "0.0.1"), Gem::Specification.new("bar", "0.0.2"), @@ -13,7 +13,7 @@ before(:each) do allow(Bundler).to(receive(:locked_gems)).and_return( Struct.new(:specs).new([ - *specs, + *specs_without_sorbet, Gem::Specification.new("sorbet-static", "0.5.10210"), ]), ) @@ -223,7 +223,7 @@ def foo it "does not register an offence" do allow(Bundler).to(receive(:locked_gems)).and_return( Struct.new(:specs).new([ - *specs, + *specs_without_sorbet, Gem::Specification.new("sorbet-static", "0.5.10209"), ]), ) @@ -245,9 +245,7 @@ def foo describe "the obsolete memoization pattern" do it "does not register an offence" do allow(Bundler).to(receive(:locked_gems)).and_return( - Struct.new(:specs).new([ - *specs, - ]), + Struct.new(:specs).new(specs_without_sorbet), ) expect_no_offenses(<<~RUBY) @@ -261,25 +259,4 @@ def foo end end end - - describe "a mistaken variant of the obsolete memoization pattern" do - 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 two-stage workaround for memoization in `#typed: strict` files is no longer necessary. 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.new, T.nilable(Foo)) - end - RUBY - end - end end