diff --git a/lib/rubocop/cop/sorbet/callback_conditionals_binding.rb b/lib/rubocop/cop/sorbet/callback_conditionals_binding.rb index f0a42a66..4f8a6a35 100644 --- a/lib/rubocop/cop/sorbet/callback_conditionals_binding.rb +++ b/lib/rubocop/cop/sorbet/callback_conditionals_binding.rb @@ -32,8 +32,13 @@ module Sorbet # true # end # end - class CallbackConditionalsBinding < RuboCop::Cop::Cop # rubocop:todo InternalAffairs/InheritDeprecatedCopClass - CALLBACKS = [ + class CallbackConditionalsBinding < RuboCop::Cop::Base + extend AutoCorrector + include Alignment + + MSG = "Callback conditionals should be bound to the right type. Use T.bind(self, %{type})" + + RESTRICT_ON_SEND = [ :validate, :validates, :validates_with, @@ -72,97 +77,56 @@ class CallbackConditionalsBinding < RuboCop::Cop::Cop # rubocop:todo InternalAff :append_after_action, ].freeze - def autocorrect(node) - lambda do |corrector| - options = node.each_child_node.find(&:hash_type?) - - conditional = nil - options.each_pair do |keyword, block| - if keyword.value == :if || keyword.value == :unless - conditional = block - break - end - end - - _, _, block = conditional.child_nodes - - # Find the class node and check if it includes a namespace on the - # same line e.g.: Namespace::Class, which will require the fully - # qualified name - - klass = node.ancestors.find(&:class_type?) - - expected_class = if klass.children.first.children.first.nil? - node.parent_module_name.split("::").last - else - klass.identifier.source - end - - do_end_lambda = conditional.source.include?("do") && conditional.source.include?("end") + # @!method argumentless_unbound_callable_callback_conditional?(node) + def_node_matcher :argumentless_unbound_callable_callback_conditional?, <<~PATTERN + (pair (sym {:if :unless}) # callback conditional + $(block + (send nil? {:lambda :proc}) # callable + (args) # argumentless + !`(send(const {cbase nil?} :T) :bind self $_ ) # unbound + ) + ) + PATTERN - unless do_end_lambda - # We are converting a one line lambda into a multiline - # Remove the space after the `{` - if /{\s/.match?(conditional.source) - corrector.remove_preceding(block, 1) + def on_send(node) + type = immediately_enclosing_module_name(node) + return unless type + + node.arguments.each do |arg| + next unless arg.hash_type? # Skip non-keyword arguments + + arg.each_child_node do |pair_node| + argumentless_unbound_callable_callback_conditional?(pair_node) do |block| + add_offense(pair_node, message: format(MSG, type: type)) do |corrector| + block_opening_indentation = block.source_range.source_line[/\A */] + block_body_indentation = block_opening_indentation + SPACE * configured_indentation_width + + if block.single_line? # then convert to multi-line block first + # 1. Replace whitespace (if any) between the opening delimiter and the block body, + # with newline and the correct indentation for the block body. + preceeding_whitespace_range = block.loc.begin.end.join(block.body.source_range.begin) + corrector.replace(preceeding_whitespace_range, "\n#{block_body_indentation}") + + # 2. Replace whitespace (if any) between the block body and the closing delimiter, + # with newline and the same indentation as the block opening. + trailing_whitespace_range = block.body.source_range.end.join(block.loc.end.begin) + corrector.replace(trailing_whitespace_range, "\n#{block_opening_indentation}") + end + + # Prepend the binding to the block body + corrector.insert_before(block.body, "T.bind(self, #{type})\n#{block_body_indentation}") + end end - - # Remove the last space and `}` and re-add it with a line break - # and the correct indentation - base_indentation = " " * node.loc.column - chars_to_remove = /\s}/.match?(conditional.source) ? 2 : 1 - corrector.remove_trailing(conditional, chars_to_remove) - corrector.insert_after(block, "\n#{base_indentation}}") end - - # Add the T.bind - indentation = " " * (node.loc.column + 2) - line_start = do_end_lambda ? "" : "\n#{indentation}" - bind = "#{line_start}T.bind(self, #{expected_class})\n#{indentation}" - - corrector.insert_before(block, bind) end end - def on_send(node) - return unless CALLBACKS.include?(node.method_name) - - options = node.each_child_node.find(&:hash_type?) - return if options.nil? - - conditional = nil - options.each_pair do |keyword, block| - next unless keyword.sym_type? - - if keyword.value == :if || keyword.value == :unless - conditional = block - break - end - end - - return if conditional.nil? || conditional.array_type? || conditional.child_nodes.empty? - - return unless conditional.arguments.empty? + private - type, _, block = conditional.child_nodes - return unless type.lambda_or_proc? || type.block_literal? - - klass = node.ancestors.find(&:class_type?) - - expected_class = if klass&.children&.first&.children&.first.nil? - node.parent_module_name&.split("::")&.last - else - klass.identifier.source - end - - return if expected_class.nil? - - unless block.source.include?("T.bind(self") - add_offense( - node, - message: "Callback conditionals should be bound to the right type. Use T.bind(self, #{expected_class})", - ) - end + # Find the immediately enclosing class or module name. + # Returns `nil`` if the immediate parent (skipping begin if present) is not a class or module. + def immediately_enclosing_module_name(node) + (node.parent&.begin_type? ? node.parent.parent : node.parent)&.defined_module_name end end end diff --git a/spec/rubocop/cop/callback_conditionals_binding_spec.rb b/spec/rubocop/cop/callback_conditionals_binding_spec.rb index 373d22b4..99d5b594 100644 --- a/spec/rubocop/cop/callback_conditionals_binding_spec.rb +++ b/spec/rubocop/cop/callback_conditionals_binding_spec.rb @@ -69,6 +69,16 @@ class Post < ApplicationRecord end RUBY end + + it("allows blocks that already have a T.bind") do + expect_no_offenses <<~RUBY + module Namespace + class Post + validates :it, presence: true, if: -> { T.bind(self, Namespace::Post).should? } + end + end + RUBY + end end describe("offenses") do @@ -76,7 +86,16 @@ class Post < ApplicationRecord expect_offense(<<~RUBY) class Post < ApplicationRecord before_create :do_it, if: -> { should? && ready? } - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + end + RUBY + + expect_correction(<<~RUBY) + class Post < ApplicationRecord + before_create :do_it, if: -> { + T.bind(self, Post) + should? && ready? + } end RUBY end @@ -85,7 +104,16 @@ class Post < ApplicationRecord expect_offense(<<~RUBY) class Post < ApplicationRecord before_create :do_it, unless: -> { shouldnt? } - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + ^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + end + RUBY + + expect_correction(<<~RUBY) + class Post < ApplicationRecord + before_create :do_it, unless: -> { + T.bind(self, Post) + shouldnt? + } end RUBY end @@ -94,96 +122,92 @@ class Post < ApplicationRecord expect_offense(<<~RUBY) class Post < ApplicationRecord before_create :do_it, unless: lambda { - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) - shouldnt? + ^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + shouldnt? } end RUBY - end - it("disallows having callback conditionals without bindings in multi line blocks using do end") do - expect_offense(<<~RUBY) + expect_correction(<<~RUBY) class Post < ApplicationRecord - before_create :do_it, unless: -> do - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) - shouldnt? - end + before_create :do_it, unless: lambda { + T.bind(self, Post) + shouldnt? + } end RUBY end - end - describe("autocorrect") do - it("autocorrects by adding the missing binding") do - source = <<~RUBY + it("disallows having callback conditionals without bindings in multi line blocks using do end") do + expect_offense(<<~RUBY) class Post < ApplicationRecord - before_create :do_it, if: -> { should? && ready? } + before_create :do_it, unless: -> do + ^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + shouldnt? + end end RUBY - corrected_source = <<~CORRECTED + expect_correction(<<~RUBY) class Post < ApplicationRecord - before_create :do_it, if: -> { + before_create :do_it, unless: -> do T.bind(self, Post) - should? && ready? - } + shouldnt? + end end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("autocorrects with chaining if the lambda includes a single statement") do - source = <<~RUBY + expect_offense(<<~RUBY) class Post < ApplicationRecord before_create :do_it, if: -> { should? } + ^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) end RUBY - corrected_source = <<~CORRECTED + expect_correction(<<~RUBY) class Post < ApplicationRecord before_create :do_it, if: -> { T.bind(self, Post) should? } end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("autocorrects multi line blocks with a single statement") do - source = <<~RUBY + expect_offense(<<~RUBY) class Post < ApplicationRecord before_create :do_it, if: -> do + ^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) should? end end RUBY - corrected_source = <<~CORRECTED + expect_correction(<<~RUBY) class Post < ApplicationRecord before_create :do_it, if: -> do T.bind(self, Post) should? end end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end - it("autocorrects multi line blocks with multie statements") do - source = <<~RUBY + it("autocorrects multi line blocks with multiple statements") do + expect_offense <<~RUBY class Post < ApplicationRecord before_create :do_it, if: -> do + ^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) a = should? a && ready? end end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY class Post < ApplicationRecord before_create :do_it, if: -> do T.bind(self, Post) @@ -191,21 +215,20 @@ class Post < ApplicationRecord a && ready? end end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("autocorrects with the correct type when there are multiple parent levels") do - source = <<~RUBY + expect_offense <<~RUBY class Post extend Something validates :it, presence: true, if: -> {should? && ready?} + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY class Post extend Something @@ -214,21 +237,20 @@ class Post should? && ready? } end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("autocorrects to multiline if the receiver of the send node is not self") do - source = <<~RUBY + expect_offense <<~RUBY class Post extend Something validates :it, presence: true, if: -> { %w(first second).include?(name) } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY class Post extend Something @@ -237,23 +259,22 @@ class Post %w(first second).include?(name) } end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("doesn't try to add more lines if already a do end block") do - source = <<~RUBY + expect_offense <<~RUBY class Post extend Something validates :it, presence: true, if: -> do + ^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) should? && ready? end end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY class Post extend Something @@ -262,21 +283,20 @@ class Post should? && ready? end end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("corrects chained methods to a single statement") do - source = <<~RUBY + expect_offense <<~RUBY class Post extend Something validates :it, presence: true, if: -> { something.present? } + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY class Post extend Something @@ -285,42 +305,40 @@ class Post something.present? } end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("does not try to chain if the condition is an instance variable") do - source = <<~RUBY + expect_offense <<~RUBY class Post validates :it, presence: true, if: lambda { @ready } + ^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY class Post validates :it, presence: true, if: lambda { T.bind(self, Post) @ready } end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("does not use fully qualified names for corrections") do - source = <<~RUBY + expect_offense <<~RUBY module First module Second class Post validates :it, presence: true, if: -> { should? } + ^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) end end end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY module First module Second class Post @@ -331,44 +349,43 @@ class Post end end end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("uses fully qualified name if defined on the same line") do - source = <<~RUBY + expect_offense <<~RUBY class First::Second::Post validates :it, presence: true, if: -> { should? } + ^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, First::Second::Post) end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY class First::Second::Post validates :it, presence: true, if: -> { T.bind(self, First::Second::Post) should? } end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end it("finds the right class when there are multiple inside a namespace") do - source = <<~RUBY + expect_offense <<~RUBY module First class Article validates :that, if: -> { must? } + ^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Article) end class Second::Post validates :it, presence: true, if: -> { should? } + ^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Second::Post) end end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY module First class Article validates :that, if: -> { @@ -384,48 +401,99 @@ class Second::Post } end end - CORRECTED - - expect(autocorrect_source(source)).to(eq(corrected_source)) + RUBY end - it("accepts proc as block") do - source = <<~RUBY + it("detects offenses in procs") do + expect_offense <<~RUBY class Post validates :it, presence: true, if: proc { should? } + ^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) end RUBY - corrected_source = <<~CORRECTED + expect_correction <<~RUBY class Post validates :it, presence: true, if: proc { T.bind(self, Post) should? } end - CORRECTED + RUBY + end - expect(autocorrect_source(source)).to(eq(corrected_source)) + it "handles the presence of both if: and unless: conditionals" do + expect_offense(<<~RUBY) + class Post < ApplicationRecord + before_create :do_it, if: -> { should? }, unless: -> { shouldnt? } + ^^^^^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + ^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + end + RUBY + + expect_correction(<<~RUBY) + class Post < ApplicationRecord + before_create :do_it, if: -> { + T.bind(self, Post) + should? + }, unless: -> { + T.bind(self, Post) + shouldnt? + } + end + RUBY end - it("does not attempt to correct blocks that already have a T.bind") do - source = <<~RUBY - module Namespace - class Post - validates :it, presence: true, if: -> { T.bind(self, Namespace::Post).should? } - end + it "detects offenses inside single line do-end blocks" do + expect_offense(<<~RUBY) + class Post < ApplicationRecord + before_create :do_it, if: -> do should end + ^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + after_create :do_it, if: -> do should end + ^^^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) end RUBY - corrected_source = <<~CORRECTED - module Namespace - class Post - validates :it, presence: true, if: -> { T.bind(self, Namespace::Post).should? } + expect_correction(<<~RUBY) + class Post < ApplicationRecord + before_create :do_it, if: -> do + T.bind(self, Post) + should + end + after_create :do_it, if: -> do + T.bind(self, Post) + should end end - CORRECTED + RUBY + end + + describe "custom indentation widths" do + let(:config) do + RuboCop::Config.new( + "Layout/IndentationWidth" => { + "Width" => 4, + }, + ) + end + + it "indents the autocorrected code with the same width as the original code" do + expect_offense(<<~RUBY) + class Post < ApplicationRecord + before_create :do_it, if: -> { should? } + ^^^^^^^^^^^^^^^^^^ Callback conditionals should be bound to the right type. Use T.bind(self, Post) + end + RUBY - expect(autocorrect_source(source)).to(eq(corrected_source)) + expect_correction(<<~RUBY) + class Post < ApplicationRecord + before_create :do_it, if: -> { + T.bind(self, Post) + should? + } + end + RUBY + end end end end