Refactor SignatureBuildOrder

This refactor removes the dependency on `unparser` for autocorrection,
and teaches the cop to handle unknown builder methods by ignoring their
ordering amongst known methods, rather than skipping the entire `sig`.

Incidentally, it also migrates from the deprecated `RuboCop::Cop::Cop`
class to `RuboCop::Cop::Base`.

Gemfile.lock

@@ -51,9 +51,6 @@ GEM
       rubocop (~> 1.51)
     ruby-progressbar (1.13.0)
     unicode-display_width (2.4.2)
-    unparser (0.6.0)
-      diff-lcs (~> 1.3)
-      parser (>= 3.0.0)
     yard (0.9.36)
 
 PLATFORMS
@@ -65,7 +62,6 @@ DEPENDENCIES
   rspec
   rubocop-shopify
   rubocop-sorbet!
-  unparser (~> 0.6)
   yard (~> 0.9)
 
 BUNDLED WITH

README.md

@@ -15,13 +15,6 @@ or, if you use `Bundler`, add this line your application's `Gemfile`:
 gem 'rubocop-sorbet', require: false
 ```
 
-Note: in order to use the [Sorbet/SignatureBuildOrder](https://github.com/Shopify/rubocop-sorbet/blob/main/manual/cops_sorbet.md#sorbetsignaturebuildorder) cop autocorrect feature, it is necessary
-to install `unparser` in addition to `rubocop-sorbet`.
-
-```ruby
-gem "unparser", require: false
-```
-
 ## Usage
 
 You need to tell RuboCop to load the Sorbet extension. There are three ways to do this:

lib/rubocop/cop/sorbet/signatures/signature_build_order.rb

@@ -1,11 +1,5 @@
 # frozen_string_literal: true
 
-begin
-  require "unparser"
-rescue LoadError
-  nil
-end
-
 module RuboCop
   module Cop
     module Sorbet
@@ -27,7 +21,8 @@ module Sorbet
       #
       #   # good
       #   sig { params(x: Integer).returns(Integer) }
-      class SignatureBuildOrder < ::RuboCop::Cop::Cop # rubocop:todo InternalAffairs/InheritDeprecatedCopClass
+      class SignatureBuildOrder < ::RuboCop::Cop::Base
+        extend AutoCorrector
         include SignatureHelp
 
         # @!method root_call(node)
@@ -36,92 +31,66 @@ class SignatureBuildOrder < ::RuboCop::Cop::Cop # rubocop:todo InternalAffairs/I
         PATTERN
 
         def on_signature(node)
-          calls = call_chain(node.children[2]).map(&:method_name)
-          return if calls.empty?
-
-          expected_order = calls.sort_by do |call|
-            builder_method_indexes.fetch(call) do
-              # Abort if we don't have a configured order for this call,
-              # likely because the method name is still being typed.
-              return nil
-            end
-          end
-          return if expected_order == calls
+          body = node.body
 
-          message = "Sig builders must be invoked in the following order: #{expected_order.join(", ")}."
+          actual_calls_and_indexes = call_chain(body).map.with_index do |send_node, actual_index|
+            # The index this method call appears at in the configured Order.
+            expected_index = builder_method_indexes[send_node.method_name]
 
-          unless can_autocorrect?
-            message += " For autocorrection, add the `unparser` gem to your project."
+            [send_node, actual_index, expected_index]
           end
 
-          add_offense(
-            node.children[2],
-            message: message,
-          )
-          node
-        end
+          # Temporarily extract unknown method calls
+          expected_calls_and_indexes, unknown_calls_and_indexes = actual_calls_and_indexes
+            .partition { |_, _, expected_index| expected_index }
 
-        def autocorrect(node)
-          return unless can_autocorrect?
+          # Sort known method calls by expected index
+          expected_calls_and_indexes.sort_by! { |_, _, expected_index| expected_index }
 
-          lambda do |corrector|
-            tree = call_chain(node_reparsed_with_modern_features(node))
-              .sort_by { |call| builder_method_indexes[call.method_name] }
-              .reduce(nil) do |receiver, caller|
-                caller.updated(nil, [receiver] + caller.children.drop(1))
-              end
+          # Re-insert unknown method calls in their positions
+          unknown_calls_and_indexes.each do |entry|
+            _, original_index, _ = entry
 
-            corrector.replace(
-              node,
-              Unparser.unparse(tree),
-            )
+            expected_calls_and_indexes.insert(original_index, entry)
           end
-        end
 
-        # Create a subclass of AST Builder that has modern features turned on
-        class ModernBuilder < RuboCop::AST::Builder
-          modernize
+          # Compare expected and actual ordering
+          expected_method_names = expected_calls_and_indexes.map { |send_node, _, _| send_node.method_name }
+          actual_method_names = actual_calls_and_indexes.map { |send_node, _, _| send_node.method_name }
+          return if expected_method_names == actual_method_names
+
+          add_offense(
+            body,
+            message: "Sig builders must be invoked in the following order: #{expected_method_names.join(", ")}.",
+          ) { |corrector| corrector.replace(body, expected_source(expected_calls_and_indexes)) }
         end
-        private_constant :ModernBuilder
 
         private
 
-        # This method exists to reparse the current node with modern features enabled.
-        # Modern features include "index send" emitting, which is necessary to unparse
-        # "index sends" (i.e. `[]` calls) back to index accessors (i.e. as `foo[bar]``).
-        # Otherwise, we would get the unparsed node as `foo.[](bar)`.
-        def node_reparsed_with_modern_features(node)
-          # Create a new parser with a modern builder class instance
-          parser = Parser::CurrentRuby.new(ModernBuilder.new)
-          # Create a new source buffer with the node source
-          buffer = Parser::Source::Buffer.new(processed_source.path, source: node.source)
-          # Re-parse the buffer
-          parser.parse(buffer)
-        end
+        def expected_source(expected_calls_and_indexes)
+          expected_calls_and_indexes.reduce(nil) do |receiver_source, (send_node, _, _)|
+            send_source = if send_node.arguments?
+              "#{send_node.method_name}(#{send_node.arguments.map(&:source).join(", ")})"
+            else
+              send_node.method_name.to_s
+            end
 
-        def can_autocorrect?
-          defined?(::Unparser)
+            receiver_source ? "#{receiver_source}.#{send_source}" : send_source
+          end
         end
 
-        def call_chain(sig_child_node)
-          return [] if sig_child_node.nil?
+        # Split foo.bar.baz into [foo, foo.bar, foo.bar.baz]
+        def call_chain(node)
+          chain = []
 
-          call_node = root_call(sig_child_node).first
-          return [] unless call_node
-
-          calls = []
-          while call_node != sig_child_node
-            calls << call_node
-            call_node = call_node.parent
+          while node&.send_type?
+            chain << node
+            node = node.receiver
           end
 
-          calls << sig_child_node
-
-          calls
-        end
+          chain.reverse!
 
-        def builder?(method_name)
-          builder_method_indexes.key?(method_name)
+          chain
         end
 
         def builder_method_indexes

rubocop-sorbet.gemspec

@@ -26,7 +26,6 @@ Gem::Specification.new do |spec|
   spec.require_paths = ["lib"]
 
   spec.add_development_dependency("rspec", "~> 3.7")
-  spec.add_development_dependency("unparser", "~> 0.6")
 
   spec.add_runtime_dependency("rubocop", ">= 0.90.0")
 end

spec/rubocop/cop/sorbet/signatures/signature_build_order_spec.rb

@@ -49,50 +49,45 @@
     end
 
     it("enforces orders of builder calls") do
-      message = "Sig builders must be invoked in the following order: type_parameters, params, void."
       expect_offense(<<~RUBY)
         sig { void.type_parameters(:U).params(x: T.type_parameter(:U)) }
-              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sig builders must be invoked in the following order: type_parameters, params, void.
       RUBY
     end
   end
 
   describe("autocorrect") do
     it("autocorrects sigs in the correct order") do
-      source = <<~RUBY
+      expect_offense <<~RUBY
         sig { void.type_parameters(:U).params(x: T.type_parameter(:U)) }
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sig builders must be invoked in the following order: type_parameters, params, void.
+      RUBY
+
+      expect_correction <<~RUBY
+        sig { type_parameters(:U).params(x: T.type_parameter(:U)).void }
       RUBY
-      expect(autocorrect_source(source))
-        .to(eq(<<~RUBY))
-          sig { type_parameters(:U).params(x: T.type_parameter(:U)).void }
-        RUBY
     end
 
     it("autocorrects sigs with generic types properly") do
-      source = <<~RUBY
+      expect_offense <<~RUBY
         sig { void.type_parameters(:U).params(x: T.type_parameter(:U), y: T::Hash[String, Integer]) }
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sig builders must be invoked in the following order: type_parameters, params, void.
+      RUBY
+
+      expect_correction <<~RUBY
+        sig { type_parameters(:U).params(x: T.type_parameter(:U), y: T::Hash[String, Integer]).void }
       RUBY
-      expect(autocorrect_source(source))
-        .to(eq(<<~RUBY))
-          sig { type_parameters(:U).params(x: T.type_parameter(:U), y: T::Hash[String, Integer]).void }
-        RUBY
     end
-  end
 
-  describe("without the unparser gem") do
-    it("catches the errors and suggests using Unparser for the correction") do
-      original_unparser = Unparser
-      Object.send(:remove_const, :Unparser) # What does "constant" even mean?
-      message =
-        "Sig builders must be invoked in the following order: type_parameters, params, void. " \
-          "For autocorrection, add the `unparser` gem to your project."
+    it("autocorrects sigs even with many unknown methods") do
+      expect_offense <<~RUBY
+        sig { void.foo.type_parameters(:U).bar.params(x: T.type_parameter(:U), y: T::Hash[String, Integer]).baz }
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sig builders must be invoked in the following order: type_parameters, foo, params, bar, void, baz.
+      RUBY
 
-      expect_offense(<<~RUBY)
-        sig { void.type_parameters(:U).params(x: T.type_parameter(:U)) }
-              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+      expect_correction <<~RUBY
+        sig { type_parameters(:U).foo.params(x: T.type_parameter(:U), y: T::Hash[String, Integer]).bar.void.baz }
       RUBY
-    ensure
-      Object.const_set(:Unparser, original_unparser)
     end
   end
 
@@ -106,9 +101,33 @@
       }
     end
 
-    it("ignores chains including unknown methods") do
-      expect_no_offenses(<<~RUBY)
-        sig { override.params(x: Integer).returns(Integer) } # params not in Order
+    it("ignores unknown methods, while sorting the remainder of the chain") do
+      expect_offense(<<~RUBY)
+        sig { override.params(x: Integer).returns(Integer) }
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sig builders must be invoked in the following order: returns, params, override.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        sig { returns(Integer).params(x: Integer).override }
+      RUBY
+
+      # Doesn't actually care about where params appears; only cares about relative ordering of returns and override.
+      expect_offense(<<~RUBY)
+        sig { override.returns(Integer).params(x: Integer) }
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sig builders must be invoked in the following order: returns, override, params.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        sig { returns(Integer).override.params(x: Integer) }
+      RUBY
+
+      expect_offense(<<~RUBY)
+        sig { params(x: Integer).override.returns(Integer) }
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sig builders must be invoked in the following order: params, returns, override.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        sig { params(x: Integer).returns(Integer).override }
       RUBY
     end