Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extract new BuggyObsoleteStrictMemoization cop #175

Merged
merged 3 commits into from
Sep 1, 2023
Merged

Extract new BuggyObsoleteStrictMemoization cop #175

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88b5979
Clarify “specs” variable name
amomchilov Aug 31, 2023
dc4828a
Copy `ObsoleteStrictMemoization` as starting point
amomchilov Aug 31, 2023
0ab5e8c
Introduce `BuggyObsoleteStrictMemoization` cop
amomchilov Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
92 changes: 92 additions & 0 deletions lib/rubocop/cop/sorbet/buggy_obsolete_strict_memoization.rb
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# frozen_string_literal: true

require "rubocop"

module RuboCop
module Cop
module Sorbet
# Checks for the obsolete pattern for initializing instance variables that was required for older Sorbet
# versions in `#typed: strict` files.
#
# It's no longer required, as of Sorbet 0.5.10210
# See https://sorbet.org/docs/type-assertions#put-type-assertions-behind-memoization
#
# @example
#
# # bad
# sig { returns(Foo) }
# def foo
# @foo = T.let(@foo, T.nilable(Foo))
# @foo ||= Foo.new
# end
#
# # bad
# 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
#
# # good
# sig { returns(Foo) }
# def foo
# @foo ||= T.let(Foo.new, T.nilable(Foo))
# end
#
class ObsoleteStrictMemoization < RuboCop::Cop::Base
include RuboCop::Cop::MatchRange
include RuboCop::Cop::Alignment
include RuboCop::Cop::LineLengthHelp
include RuboCop::Cop::RangeHelp
extend AutoCorrector

include TargetSorbetVersion
minimum_target_sorbet_static_version "0.5.10210"

MSG = "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."

# @!method legacy_memoization_pattern?(node)
def_node_matcher :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
{(ivar _ivar) nil}
(send (const {nil? cbase} :T) :nilable $_ivar_type))) # T.nilable(_ivar_type)
$(or-asgn (ivasgn _ivar) $_initialization_expr)) # Second line: @_ivar ||= _initialization_expr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Done.

PATTERN

def on_begin(node)
legacy_memoization_pattern?(node) do |first_asgn_node, ivar, t, ivar_type, second_or_asgn_node, init_expr| # rubocop:disable Metrics/ParameterLists
add_offense(first_asgn_node) do |corrector|
indent = offset(node)
correction = "#{ivar} ||= #{t.source}.let(#{init_expr.source}, #{t.source}.nilable(#{ivar_type.source}))"

# We know good places to put line breaks, if required.
if line_length(indent + correction) > max_line_length || correction.include?("\n")
correction = <<~RUBY.chomp
#{ivar} ||= #{t.source}.let(
#{indent} #{init_expr.source.gsub("\n", "\n#{indent}")},
#{indent} #{t.source}.nilable(#{ivar_type.source.gsub("\n", "\n#{indent}")}),
#{indent})
RUBY
end

corrector.replace(
range_between(first_asgn_node.source_range.begin_pos, second_or_asgn_node.source_range.end_pos),
correction,
)
end
end
end

def relevant_file?(file)
super && enabled_for_sorbet_static_version?
end
end
end
end
end
283 changes: 283 additions & 0 deletions spec/rubocop/cop/sorbet/buggy_obsolete_strict_memoization_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,283 @@
# frozen_string_literal: true

require "spec_helper"

RSpec.describe(RuboCop::Cop::Sorbet::ObsoleteStrictMemoization, :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

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 obsolete memoization pattern" do
it "registers an offence and autocorrects" do
expect_offense(<<~RUBY)
def foo
@foo = T.let(@foo, 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

describe "with fully qualified ::T" do
it "registers an offence and autocorrects" do
expect_offense(<<~RUBY)
def foo
@foo = ::T.let(@foo, ::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

describe "with a complex type" do
it "registers an offence and autocorrects" do
expect_offense(<<~RUBY)
def client_info_hash
@client_info_hash = T.let(@client_info_hash, T.nilable(T::Hash[Symbol, T.untyped]))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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.
@client_info_hash ||= client_info.to_hash
end
RUBY

expect_correction(<<~RUBY)
def client_info_hash
@client_info_hash ||= T.let(client_info.to_hash, T.nilable(T::Hash[Symbol, T.untyped]))
end
RUBY
end
end

describe "with a long initialization expression" do
let(:max_line_length) { 90 }

let(:config) do
RuboCop::Config.new(
"Layout/LineLength" => { "Max" => max_line_length },
"Sorbet/ObsoleteStrictMemoization" => cop_config,
)
end

it "registers an offence and autocorrects into a multiline expression" do
expect_offense(<<~RUBY)
def foo
@foo = T.let(@foo, T.nilable(SomeReallyLongTypeName______________________________________))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 ||= some_really_long_initialization_expression______________________________________
end
RUBY

expect_correction(<<~RUBY)
def foo
@foo ||= T.let(
some_really_long_initialization_expression______________________________________,
T.nilable(SomeReallyLongTypeName______________________________________),
)
end
RUBY

autocorrected_source = autocorrect_source(<<~RUBY)
def foo
@foo = T.let(@foo, T.nilable(SomeReallyLongTypeName______________________________________))
@foo ||= some_really_long_initialization_expression______________________________________
end
RUBY

longest_line = autocorrected_source.lines.max_by(&:length)
expect(longest_line.length).to(be <= max_line_length)
end
end

describe "with multiline initialization expression" do
it "registers an offence and autocorrects into a multiline expression" do
# There's special auto-correct logic to handle a multiline initialization expression, so that it
# *doesn't* end up like this:
#
# def foo
# @foo = T.let(multiline_method_call(
# foo,
# bar,
# baz,
# ), T.nilable(Foo))
# end

expect_offense(<<~RUBY)
def foo
@foo = T.let(@foo, 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 ||= multiline_method_call(
foo,
bar,
baz,
)
end
RUBY

expect_correction(<<~RUBY)
def foo
@foo ||= T.let(
multiline_method_call(
foo,
bar,
baz,
),
T.nilable(Foo),
)
end
RUBY
end

describe "with a gap between the two lines" do
it "registers an offence and autocorrects into a multiline expression" do
expect_offense(<<~RUBY)
def foo
@foo = T.let(@foo, 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 ||= multiline_method_call(
foo,
bar,
baz,
)
end
RUBY

expect_correction(<<~RUBY)
def foo
@foo ||= T.let(
multiline_method_call(
foo,
bar,
baz,
),
T.nilable(Foo),
)
end
RUBY
end
end
end

context "when its not the first line in a method" do
it "registers an offence and autocorrects" do
expect_offense(<<~RUBY)
def foo
some
other
code
@foo = T.let(@foo, 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
some
other
code
@foo ||= T.let(Foo.new, T.nilable(Foo))
end
RUBY
end
end

context "using an old version of Sorbet" do
# For old versions, the obsolete memoization pattern isn't actually obsolete.

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_without_sorbet,
Gem::Specification.new("sorbet-static", "0.5.10209"),
]),
)

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

context "not using Sorbet" do
# If the project is not using Sorbet, the obsolete memoization pattern might be intentional.

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_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
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