Unlimited terms after ellipsis (#6843)

* Create constants

* Factorize methods

* Refactor to avoid repeated code

* Return an array instead of element or nil.

This fits better with the functionality

* Factorize repetitive code

* Move code where it belongs.

* Take care of the index where it should. Improve readability.

* Move sequence head handling where it belongs.

This maintains current behaviors, but would make it trivial to raise instead
or also capture the current_node's type. See [#6842]

* Remove redundant return value

* Add missing tests for invalid syntax with ellipsis use

* Allow unlimited terms after ellipsis [#6840]

CHANGELOG.md

@@ -10,6 +10,7 @@
 * Add new `Style/ConstantVisibility` cop for enforcing visibility declarations of class- and module constants. ([@drenmi][])
 * [#6378](https://github.com/rubocop-hq/rubocop/issues/6378): Add `Lint/ToJSON` cop to enforce an argument when overriding #to_json. ([@allcentury][])
 * [#6346](https://github.com/rubocop-hq/rubocop/issues/6346): Add auto-correction to `Rails/TimeZone`. ([@dcluna][])
+* [#6840](https://github.com/rubocop-hq/rubocop/issues/6840): Node patterns now allow unlimited elements after `...`. ([@marcandre][])
 
 ### Bug fixes
 

lib/rubocop/node_pattern.rb

@@ -37,8 +37,7 @@ module RuboCop
   #     '(send !const ...)' # ! negates the next part of the pattern
   #     '$(send const ...)' # arbitrary matching can be performed on a capture
   #     '(send _recv _msg)' # wildcards can be named (for readability)
-  #     '(send ... :new)'   # you can specifically match against the last child
-  #                         # (this only works for the very last)
+  #     '(send ... :new)'   # you can match against the last children
   #     '(send $...)'       # capture all the children as an array
   #     '(send $... int)'   # capture all children but the last as an array
   #     '(send _x :+ _x)'   # unification is performed on named wildcards
@@ -118,8 +117,13 @@ class Compiler
       PARAM     = /\A#{PARAM_NUMBER}\Z/.freeze
       CLOSING   = /\A(?:\)|\}|\])\Z/.freeze
 
+      REST      = '...'.freeze
+      CAPTURED_REST = '$...'.freeze
+
       attr_reader :match_code
 
+      SEQ_HEAD_INDEX = -1
+
       def initialize(str, node_var = 'node0')
         @string   = str
         @root     = node_var
@@ -190,77 +194,65 @@ def compile_guard_clause(cur_node)
       end
 
       def compile_seq_terms(tokens, cur_node)
-        ret, size =
+        ret =
           compile_seq_terms_with_size(tokens, cur_node) do |token, terms, index|
-            case token
-            when '...'.freeze
-              return compile_ellipsis(tokens, cur_node, terms, index)
-            when '$...'.freeze
-              return compile_capt_ellip(tokens, cur_node, terms, index)
+            capture = next_capture if token == CAPTURED_REST
+            if capture || token == REST
+              index = 0 if index == SEQ_HEAD_INDEX # Consider ($...) as (_ $...)
+              return compile_ellipsis(tokens, cur_node, terms, index, capture)
             end
           end
-
-        ret << "(#{cur_node}.children.size == #{size})"
+        ret << "(#{cur_node}.children.size == #{ret.size - 1})"
       end
 
       def compile_seq_terms_with_size(tokens, cur_node)
-        index = nil
+        index = SEQ_HEAD_INDEX
         terms = []
         until tokens.first == ')'
-          yield tokens.first, terms, index || 0
-          term, index = compile_expr_with_index(tokens, cur_node, index)
+          yield tokens.first, terms, index
+          term = compile_expr_with_index(tokens, cur_node, index)
+          index += 1
           terms << term
         end
 
         tokens.shift # drop concluding )
-        [terms, index]
+        terms
       end
 
       def compile_expr_with_index(tokens, cur_node, index)
-        if index.nil?
+        if index == SEQ_HEAD_INDEX
           # in 'sequence head' position; some expressions are compiled
           # differently at 'sequence head' (notably 'node type' expressions)
           # grep for seq_head to see where it makes a difference
-          [compile_expr(tokens, cur_node, true), 0]
+          compile_expr(tokens, cur_node, true)
         else
           child_node = "#{cur_node}.children[#{index}]"
-          [compile_expr(tokens, child_node, false), index + 1]
+          compile_expr(tokens, child_node, false)
         end
       end
 
-      def compile_ellipsis(tokens, cur_node, terms, index)
-        if (term = compile_seq_tail(tokens, "#{cur_node}.children.last"))
-          terms << "(#{cur_node}.children.size > #{index})"
-          terms << term
-        elsif index > 0
-          terms << "(#{cur_node}.children.size >= #{index})"
-        end
-        terms
-      end
-
-      def compile_capt_ellip(tokens, cur_node, terms, index)
-        capture = next_capture
-        if (term = compile_seq_tail(tokens, "#{cur_node}.children.last"))
-          terms << "(#{cur_node}.children.size > #{index})"
-          terms << term
-          terms << "(#{capture} = #{cur_node}.children[#{index}..-2])"
-        else
-          terms << "(#{cur_node}.children.size >= #{index})" if index > 0
-          terms << "(#{capture} = #{cur_node}.children[#{index}..-1])"
+      def compile_ellipsis(tokens, cur_node, terms, index, capture = nil)
+        tokens.shift # drop ellipsis
+        tail = compile_seq_tail(tokens, cur_node)
+        terms << "(#{cur_node}.children.size >= #{index + tail.size})"
+        terms.concat tail
+        if capture
+          range = index..-tail.size - 1
+          terms << "(#{capture} = #{cur_node}.children[#{range}])"
         end
         terms
       end
 
       def compile_seq_tail(tokens, cur_node)
-        tokens.shift
-        if tokens.first == ')'
-          tokens.shift
-          nil
-        else
-          expr = compile_expr(tokens, cur_node, false)
-          fail_due_to('missing )') unless tokens.shift == ')'
-          expr
+        child_node = "#{cur_node}.children[%<revindex>i]"
+        terms = []
+        until tokens.first == ')'
+          terms << compile_expr(tokens, child_node, false)
         end
+        tokens.shift # drop ')'
+        # E.g. for terms.size == 3, we want to replace the three [%<revindex>i]
+        # with [-3], [-2] and [-1]
+        terms.map.with_index { |term, i| format term, revindex: i - terms.size }
       end
 
       def compile_union(tokens, cur_node, seq_head)

spec/rubocop/node_pattern_spec.rb

@@ -585,6 +585,14 @@
       it_behaves_like 'single capture'
     end
 
+    context 'with remaining patterns at the end' do
+      let(:pattern) { '(send $... int int)' }
+      let(:ruby) { '[].push(1, 2, 3)' }
+      let(:captured_val) { [s(:array), :push, s(:int, 1)] }
+
+      it_behaves_like 'single capture'
+    end
+
     context 'with a remaining sequence at the end' do
       let(:pattern) { '(send $... (int 4))' }
       let(:ruby) { '5 + 4' }
@@ -802,6 +810,14 @@
       it_behaves_like 'single capture'
     end
 
+    context 'preceding multiple captures' do
+      let(:pattern) { '(send array :push ... $_ $_)' }
+      let(:ruby) { '[1].push(2, 3, 4, 5)' }
+      let(:captured_vals) { [s(:int, 4), s(:int, 5)] }
+
+      it_behaves_like 'multiple capture'
+    end
+
     context 'with a wildcard at the end, but no remaining child to match it' do
       let(:pattern) { '(send array :zip array ... _)' }
       let(:ruby) { '[1,2].zip([3,4])' }
@@ -1173,6 +1189,12 @@ def withargs(foo, bar, qux)
       it_behaves_like 'invalid'
     end
 
+    context 'with unmatched opening paren and `...`' do
+      let(:pattern) { '(send ...' }
+
+      it_behaves_like 'invalid'
+    end
+
     context 'with unmatched closing paren' do
       let(:pattern) { '(send (const)))' }