Merge pull request #189 from Shopify/order-more-signature-builders

Add `final`, `bind`, & `implementation` to `SignatureBuildOrder`

Gemfile

@@ -7,6 +7,6 @@ gemspec
 
 gem "byebug"
 gem "rake", ">= 12.3.3"
-gem "rspec"
+gem "rspec", "~> 3.7"
 gem "rubocop-shopify", require: false
 gem "yard", "~> 0.9"

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
@@ -62,10 +59,9 @@ PLATFORMS
 DEPENDENCIES
   byebug
   rake (>= 12.3.3)
-  rspec
+  rspec (~> 3.7)
   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:

config/default.yml

@@ -188,12 +188,12 @@ Sorbet/BuggyObsoleteStrictMemoization:
     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'
@@ -223,6 +223,20 @@ Sorbet/SignatureBuildOrder:
                   then params, then return and finally the modifier
                   such as: `abstract.params(...).returns(...).soft`.'
   Enabled: true
+  Order:
+    - final
+    - abstract
+    - implementation
+    - override
+    - overridable
+    - type_parameters
+    - params
+    - bind
+    - returns
+    - void
+    - soft
+    - checked
+    - on_failure
   VersionAdded: 0.3.0
 
 Sorbet/SingleLineRbiClassModuleDefinitions:

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

@@ -1,20 +1,13 @@
 # frozen_string_literal: true
 
-begin
-  require "unparser"
-rescue LoadError
-  nil
-end
-
 module RuboCop
   module Cop
     module Sorbet
-      # Checks for the correct order of sig builder methods:
-      # - abstract, override, or overridable
-      # - type_parameters
-      # - params
-      # - returns, or void
-      # - soft, checked, or on_failure
+      # Checks for the correct order of `sig` builder methods.
+      #
+      # Options:
+      #
+      # * `Order`: The order in which to enforce the builder methods are called.
       #
       # @example
       #   # bad
@@ -28,109 +21,80 @@ 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
 
-        ORDER =
-          [
-            :abstract,
-            :override,
-            :overridable,
-            :type_parameters,
-            :params,
-            :returns,
-            :void,
-            :soft,
-            :checked,
-            :on_failure,
-          ].each_with_index.to_h.freeze
-
         # @!method root_call(node)
         def_node_search(:root_call, <<~PATTERN)
-          (send nil? {#{ORDER.keys.map(&:inspect).join(" ")}} ...)
+          (send nil? #builder? ...)
         PATTERN
 
         def on_signature(node)
-          calls = call_chain(node.children[2]).map(&:method_name)
-          return if calls.empty?
+          body = node.body
 
-          # While the developer is typing, we may have an incomplete call statement, which means `ORDER[call]` will
-          # return `nil`. In that case, invoking `sort_by` will raise
-          return if calls.any? { |call| ORDER[call].nil? }
+          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]
 
-          expected_order = calls.sort_by { |call| ORDER[call] }
-          return if expected_order == calls
-
-          message = "Sig builders must be invoked in the following order: #{expected_order.join(", ")}."
-
-          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| ORDER[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?
-
-          call_node = root_call(sig_child_node).first
-          return [] unless call_node
+        # Split foo.bar.baz into [foo, foo.bar, foo.bar.baz]
+        def call_chain(node)
+          chain = []
 
-          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
+          chain.reverse!
+
+          chain
+        end
 
-          calls
+        def builder_method_indexes
+          @configured_order ||= cop_config.fetch("Order").map(&:to_sym).each_with_index.to_h.freeze
         end
       end
     end

manual/cops_sorbet.md

@@ -733,12 +733,11 @@ Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChan
 --- | --- | --- | --- | ---
 Enabled | Yes | Yes  | 0.3.0 | -
 
-Checks for the correct order of sig builder methods:
-- abstract, override, or overridable
-- type_parameters
-- params
-- returns, or void
-- soft, checked, or on_failure
+Checks for the correct order of `sig` builder methods.
+
+Options:
+
+* `Order`: The order in which to enforce the builder methods are called.
 
 ### Examples
 
@@ -756,6 +755,12 @@ sig { returns(Integer).params(x: Integer) }
 sig { params(x: Integer).returns(Integer) }
 ```
 
+### Configurable attributes
+
+Name | Default value | Configurable values
+--- | --- | ---
+Order | `final`, `abstract`, `implementation`, `override`, `overridable`, `type_parameters`, `params`, `bind`, `returns`, `void`, `soft`, `checked`, `on_failure` | Array
+
 ## Sorbet/SingleLineRbiClassModuleDefinitions
 
 Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

rubocop-sorbet.gemspec

@@ -25,8 +25,5 @@ Gem::Specification.new do |spec|
   spec.executables   = spec.files.grep(%r{^exe/}) { |f| File.basename(f) }
   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