Fix splat target in multiple assignment losing its element type

lib/solargraph/parser/parser_gem/node_processors/masgn_node.rb

@@ -33,11 +33,19 @@ def process
             process_children
 
             lhs_arr.each_with_index do |lhs, i|
-              location = get_node_location(lhs)
-              pin = if lhs.type == :lvasgn
+              # A splat (`*args`) wraps its target - `s(:splat, s(:lvasgn, :args))` -
+              # at a location that won't match the inner pin's, so unwrap it.
+              splat = lhs.type == :splat
+              target = splat ? lhs.children[0] : lhs
+              # An anonymous splat (`a, * = arr`) has no assignment
+              # target to bind a type to.
+              next if target.nil?
+
+              location = get_node_location(target)
+              pin = if target.type == :lvasgn
                       # lvasgn is a local variable
                       locals.find { |l| l.location == location }
-                    elsif lhs.type == :ivasgn
+                    elsif target.type == :ivasgn
                       # ivasgn is an instance variable assignment
                       ivars.find { |iv| iv.location == location }
                     else
@@ -47,11 +55,11 @@ def process
               #   when a non-existant method is called on 'l'
               if pin.nil?
                 Solargraph.logger.debug do
-                  "Could not find local for masgn= value in location #{location.inspect} in #{lhs_arr} - masgn = #{masgn}, lhs.type = #{lhs.type}"
+                  "Could not find local for masgn= value in location #{location.inspect} in #{lhs_arr} - masgn = #{masgn}, target.type = #{target.type}"
                 end
                 next
               end
-              pin.mass_assignment = [mass_rhs, i]
+              pin.mass_assignment = [mass_rhs, i, splat]
             end
           end
         end

lib/solargraph/pin/base_variable.rb

@@ -19,7 +19,9 @@ class BaseVariable < Base
       #   that was made to this variable
       # @param assignments [Array<Parser::AST::Node>] Possible
       #   assignments that may have been made to this variable
-      # @param mass_assignment [::Array(Parser::AST::Node, Integer), nil]
+      # @param mass_assignment [::Array(Parser::AST::Node, Integer, Boolean), nil]
+      #   The mass assignment node, this variable's position in the
+      #   left-hand side, and whether this variable is a splat target.
       # @param assignment [Parser::AST::Node, nil] First assignment
       #   that was made to this variable
       # @param assignments [Array<Parser::AST::Node>] Possible
@@ -52,7 +54,7 @@ def initialize assignment: nil, assignments: [], mass_assignment: nil,
                      **splat
         super(**splat)
         @assignments = (assignment.nil? ? [] : [assignment]) + assignments
-        # @type [nil, ::Array(Parser::AST::Node, Integer)]
+        # @type [nil, ::Array(Parser::AST::Node, Integer, Boolean)]
         @mass_assignment = mass_assignment
         @return_type = return_type
         @intersection_return_type = intersection_return_type
@@ -102,7 +104,7 @@ def combine_with other, attrs = {}
 
       # @param other [self]
       #
-      # @return [Array(AST::Node, Integer), nil]
+      # @return [Array(AST::Node, Integer, Boolean), nil]
       def combine_mass_assignment other
         # @todo pick first non-nil arbitrarily - we don't yet support
         #   mass assignment merging
@@ -183,19 +185,48 @@ def probe api_map
         #   well so that we can do better flow sensitive typing with
         #   multiple assignments
         unless @mass_assignment.nil?
-          mass_node, index = @mass_assignment
+          mass_node, index, splat = @mass_assignment
           types = return_types_from_node(mass_node, api_map)
-          types.map! do |type|
-            if type.tuple?
-              type.all_params[index]
-            elsif ['::Array', '::Set', '::Enumerable'].include?(type.rooted_name)
-              type.all_params.first
+          # rubocop:disable Style/ConditionalAssignment
+          if splat
+            # A splat target (`*args`) captures every remaining
+            # element of the right-hand side, not just one position -
+            # take the source array's full parameter union rather
+            # than a single indexed/first parameter.
+            types = types.flat_map do |type|
+              if type.tuple? || splattable?(api_map, type)
+                type.all_params
+              else
+                []
+              end
             end
-          end.compact!
+          else
+            types = types.flat_map do |type|
+              if type.tuple?
+                # Array(Integer, String) gives each position its own type.
+                [type.all_params[index]].compact
+              elsif splattable?(api_map, type)
+                # Array<Integer, String> is a union: any position holds either.
+                type.all_params
+              else
+                []
+              end
+            end
+          end
+          # rubocop:enable Style/ConditionalAssignment
 
           return ComplexType::UNDEFINED if types.empty?
 
-          return adjust_type api_map, ComplexType.new(types.uniq).qualify(api_map, *gates)
+          element_type = ComplexType.new(types.uniq)
+          # A splat target (`*args`) captures the rest of the
+          # right-hand side as an array of the element type, not the
+          # element type itself.
+          if splat
+            rest_type = ComplexType::UniqueType.new('Array', [], [element_type], rooted: true, parameters_type: :list)
+            return adjust_type api_map, ComplexType.new([rest_type]).qualify(api_map, *gates)
+          end
+
+          return adjust_type api_map, element_type.qualify(api_map, *gates)
         end
 
         ComplexType::UNDEFINED
@@ -295,6 +326,19 @@ def visible_at? other_closure, other_loc
 
       private
 
+      # True for a type destructurable by a splat/multi-assignment target -
+      # Array/Set/Enumerable by name, or anything structurally Enumerable.
+      #
+      # @param api_map [ApiMap]
+      # @param type [ComplexType]
+      # @return [Boolean]
+      def splattable? api_map, type
+        return true if ['::Array', '::Set', '::Enumerable'].include?(type.rooted_name)
+
+        enumerable = ComplexType.parse('::Enumerable').qualify(api_map)
+        type.qualify(api_map).conforms_to?(api_map, enumerable, :assignment)
+      end
+
       # @param api_map [ApiMap]
       # @param raw_return_type [ComplexType, ComplexType::UniqueType]
       #

spec/parser/node_processor_spec.rb

@@ -16,6 +16,39 @@ class Foo
     end.not_to raise_error
   end
 
+  it 'does not raise when a masgn target has no matching variable pin' do
+    # `a.b, c = 1, 2` mixes an attribute-writer target (`a.b=`, not a
+    # local/ivar/cvar/gvar) with a plain local. The attribute-writer
+    # target has no BaseVariable pin to attach mass_assignment info to,
+    # which used to be handled by a debug log and a `next`.
+    node = parse(%(
+      class Attrs
+        def b=(v); end
+      end
+      a = Attrs.new
+      a.b, c = 1, 2
+    ))
+    expect do
+      described_class.process(node)
+    end.not_to raise_error
+  end
+
+  it 'names the unmatched target node type when a masgn target has no variable pin' do
+    node = parse(%(
+      class Attrs
+        def b=(v); end
+      end
+      a = Attrs.new
+      a.b, c = 1, 2
+    ))
+    logged = []
+    # Calling the block directly: the message is only built when the
+    # logger is at debug level, which earlier specs can leave raised.
+    allow(Solargraph.logger).to receive(:debug) { |*args, &block| logged << (block ? block.call : args.first) }
+    described_class.process(node)
+    expect(logged).to include(a_string_matching(/Could not find local for masgn= value.*target\.type = send/m))
+  end
+
   it 'orders optional args correctly' do
     node = parse(%(
       def foo(bar = nil, baz = nil); end

spec/pin/base_variable_spec.rb

@@ -47,6 +47,127 @@ def bar
     expect(type.simplify_literals.to_rbs).to eq('(::Integer | nil)')
   end
 
+  it 'infers a splat target in a multiple assignment as an array' do
+    source = Solargraph::Source.load_string(%(
+      class Repro
+        # @param mutator [Array<Symbol, BasicObject>]
+        # @return [void]
+        def call(mutator)
+          command, *args = mutator
+        end
+      end
+    ), 'test.rb')
+    api_map = Solargraph::ApiMap.new
+    api_map.map source
+    locals = api_map.source_map('test.rb').locals
+    command_pin = locals.find { |l| l.name == 'command' }
+    args_pin = locals.find { |l| l.name == 'args' }
+    expect(command_pin.probe(api_map).tag).to eq('Symbol')
+    expect(args_pin.probe(api_map).tag).to eq('Array<Symbol, BasicObject>')
+  end
+
+  it 'infers a splat target of a non-collection type as undefined' do
+    source = Solargraph::Source.load_string(%(
+      class Repro
+        # @param mutator [Integer]
+        # @return [void]
+        def call(mutator)
+          command, *args = mutator
+        end
+      end
+    ), 'test.rb')
+    api_map = Solargraph::ApiMap.new
+    api_map.map source
+    locals = api_map.source_map('test.rb').locals
+    args_pin = locals.find { |l| l.name == 'args' }
+    expect(args_pin.probe(api_map)).to be_undefined
+  end
+
+  it 'infers a non-splat masgn target of a non-collection type as undefined' do
+    source = Solargraph::Source.load_string(%(
+      class Repro
+        # @param mutator [Integer]
+        # @return [void]
+        def call(mutator)
+          command, args = mutator
+        end
+      end
+    ), 'test.rb')
+    api_map = Solargraph::ApiMap.new
+    api_map.map source
+    locals = api_map.source_map('test.rb').locals
+    args_pin = locals.find { |l| l.name == 'args' }
+    expect(args_pin.probe(api_map)).to be_undefined
+  end
+
+  it 'infers a splat target from a user-defined class that includes Enumerable' do
+    source = Solargraph::Source.load_string(%(
+      # @generic Elem
+      class MyBag
+        include Enumerable
+
+        # @yieldparam [generic<Elem>]
+        # @return [void]
+        def each; end
+      end
+
+      class Repro
+        # @param bag [MyBag<String>]
+        # @return [void]
+        def call(bag)
+          first, *rest = bag
+        end
+      end
+    ), 'test.rb')
+    api_map = Solargraph::ApiMap.new
+    api_map.map source
+    locals = api_map.source_map('test.rb').locals
+    first_pin = locals.find { |l| l.name == 'first' }
+    rest_pin = locals.find { |l| l.name == 'rest' }
+    expect(first_pin.probe(api_map).tag).to eq('String')
+    expect(rest_pin.probe(api_map).tag).to eq('Array<String>')
+  end
+
+  it 'gives every non-splat target the full element union of a non-tuple Array' do
+    # #to_s, not #tag, so a regression collapsing the union to one member fails here.
+    source = Solargraph::Source.load_string(%(
+      class Repro
+        # @param pair [Array<Integer, String>]
+        # @return [void]
+        def call(pair)
+          a, b = pair
+        end
+      end
+    ), 'test.rb')
+    api_map = Solargraph::ApiMap.new
+    api_map.map source
+    locals = api_map.source_map('test.rb').locals
+    a_pin = locals.find { |l| l.name == 'a' }
+    b_pin = locals.find { |l| l.name == 'b' }
+    expect(a_pin.probe(api_map).to_s).to eq('Integer, String')
+    expect(b_pin.probe(api_map).to_s).to eq('Integer, String')
+  end
+
+  it 'gives each non-splat target its own position from a tuple' do
+    pending 'https://github.com/castwide/solargraph/pull/1223'
+    source = Solargraph::Source.load_string(%(
+      class Repro
+        # @param pair [Array(Integer, String)]
+        # @return [void]
+        def call(pair)
+          a, b, c = pair
+        end
+      end
+    ), 'test.rb')
+    api_map = Solargraph::ApiMap.new
+    api_map.map source
+    locals = api_map.source_map('test.rb').locals
+    expect(locals.find { |l| l.name == 'a' }.probe(api_map).to_s).to eq('Integer')
+    expect(locals.find { |l| l.name == 'b' }.probe(api_map).to_s).to eq('String')
+    # The third target runs past the end of the tuple.
+    expect(locals.find { |l| l.name == 'c' }.probe(api_map)).to be_undefined
+  end
+
   it "understands proc kwarg parameters aren't affected by @type" do
     code = %(
       # @return [Proc]