Address PR review comments on #1223

apiology · claude · apiology · commit 42f564338dc6 · 2026-08-02T13:25:06.000-04:00
- Add UniqueType#singleton? predicate for nil/true/false, replacing the hardcoded name array in dispatch_literal? - Merge literal_param_arg_matches? into Pin::Parameter#compatible_arg? (its only caller) instead of threading a second, redundant typify call through Source::Chain::Call - Add ComplexType#without_redundant_literals, pulling the literal/non-literal union dedup out of Pin::BaseVariable#probe and into the type hierarchy - rbs_translator.rb: fix @param type [RBS::Types::Bases::Base] annotations that were actually too narrow (the real RBS type is RBS::Types::t, a union most RBS type classes do not inherit Bases::Base from). Removes 3 of the sg-ignore comments entirely. The remaining case/when-narrowing sg-ignores in type_to_tag now reference castwide/solargraph issue 1241, filed to track that the type checker does not narrow a case subject's type inside each branch - shell.rb: revert the unrelated cache_core rebuild-condition one-liner, out of scope for this PR - type_checker.rb: clarify that receiver_type generic resolution is currently restarg-specific, not yet generalized to fixed-arity params - chain_spec.rb: assert the non-literal (simplify_literals) type of true is Boolean, alongside the existing literal-type assertion Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zMih8CMx6rXoSkYehxFH3
diff --git a/lib/solargraph/complex_type.rb b/lib/solargraph/complex_type.rb
@@ -192,6 +192,18 @@ def downcast_to_literal_if_possible
       ComplexType.new(items.map(&:downcast_to_literal_if_possible))
     end
 
+    # Drop a literal item (e.g. `0`) when its non-literal base type
+    # (e.g. `Integer`) is also present in the same union - a wider
+    # type already subsumes it, so keeping both is redundant and
+    # reads as if the literal value were still specifically reachable.
+    #
+    # @return [ComplexType]
+    def without_redundant_literals
+      non_literal_names = items.reject(&:literal?).map(&:name)
+      new_items = items.reject { |item| item.literal? && non_literal_names.include?(item.non_literal_name) }
+      ComplexType.new(new_items)
+    end
+
     # @return [String]
     def desc
       rooted_tags
diff --git a/lib/solargraph/complex_type/type_methods.rb b/lib/solargraph/complex_type/type_methods.rb
@@ -58,6 +58,15 @@ def nil_type?
         @nil_type ||= name.casecmp('nil').zero?
       end
 
+      # Whether this type is one of Ruby's singleton values (nil,
+      # true, false) rather than a general class or a multi-valued
+      # literal (e.g. `0`, `:foo`).
+      #
+      # @return [Boolean]
+      def singleton?
+        nil_type? || %w[true false].include?(name)
+      end
+
       def tuple?
         @tuple ||= (name == 'Tuple') || (name == 'Array' && subtypes.length >= 1 && fixed_parameters?)
       end
diff --git a/lib/solargraph/pin/base_variable.rb b/lib/solargraph/pin/base_variable.rb
@@ -187,14 +187,10 @@ def probe api_map
         unless assignment_types.empty?
           # @type [Array<ComplexType::UniqueType>]
           items = assignment_types.flat_map(&:items).uniq
-          # Drop a literal item (e.g. `0`) when its non-literal base
-          # type (e.g. `Integer`) is also present in the same union -
-          # a later, wider assignment (`index += 1`) already
-          # subsumes it, so keeping both is redundant and reads as if
-          # the literal value were still reachable.
-          non_literal_names = items.reject(&:literal?).map(&:name)
-          items = items.reject { |item| item.literal? && non_literal_names.include?(item.non_literal_name) }
-          type_from_assignment = ComplexType.new(items)
+          # A later, wider assignment (e.g. `index += 1`) can leave a
+          # stale literal (e.g. `0`) alongside its own non-literal
+          # base type in the union - drop the redundant literal.
+          type_from_assignment = ComplexType.new(items).without_redundant_literals
         end
         return adjust_type api_map, type_from_assignment unless type_from_assignment.nil?
 
diff --git a/lib/solargraph/pin/parameter.rb b/lib/solargraph/pin/parameter.rb
@@ -227,11 +227,12 @@ def compatible_arg? atype, api_map
         ptype = typify api_map
         return true if ptype.undefined?
 
-        return true if atype.conforms_to?(api_map,
-                                          ptype,
-                                          :method_call,
-                                          %i[allow_empty_params allow_undefined])
-        ptype.generic?
+        return false unless atype.conforms_to?(api_map,
+                                               ptype,
+                                               :method_call,
+                                               %i[allow_empty_params allow_undefined]) || ptype.generic?
+
+        literal_arg_matches? ptype, atype
       end
 
       # @sg-ignore flow sensitive typing needs to handle attrs
@@ -247,6 +248,42 @@ def generate_complex_type
         nil
       end
 
+      # #compatible_arg? alone is too permissive for picking *which*
+      # overload to use for return-type inference: it treats any
+      # Integer as "compatible" with a literal-0-typed parameter
+      # (correct for general call-validity checking - you can call
+      # `array[i]` with any Integer `i` - but wrong for overload
+      # *selection*, where it would make the first literal-typed
+      # overload always win over the safe catch-all for any argument
+      # that merely happens to be assignable to it). When this
+      # parameter's type is a literal type, require every possible
+      # value of the argument to also be literal, so a non-literal
+      # (or not-entirely-literal) argument falls through to a less
+      # specific overload instead.
+      #
+      # @param ptype [ComplexType]
+      # @param atype [ComplexType]
+      # @return [Boolean]
+      def literal_arg_matches? ptype, atype
+        return true unless ptype.items.any? { |item| dispatch_literal?(item) }
+
+        atype.items.all?(&:literal?)
+      end
+
+      # nil/true/false are technically "literal" per
+      # ComplexType::UniqueType#literal? (their non_literal_name is
+      # NilClass/TrueClass/FalseClass), but they're singletons, not
+      # dispatch-relevant values the way `0` vs `1` are for tuple
+      # indexing - excluding them keeps ordinary `T?`/nilable params
+      # (extremely common, e.g. String#split's `(Regexp | string |
+      # nil pattern)`) from tripping #literal_arg_matches?.
+      #
+      # @param unique_type [ComplexType::UniqueType]
+      # @return [Boolean]
+      def dispatch_literal? unique_type
+        unique_type.literal? && !unique_type.singleton?
+      end
+
       # @return [YARD::Tags::Tag, nil]
       def param_tag
         # @sg-ignore Need to add nil check here
diff --git a/lib/solargraph/rbs_translator.rb b/lib/solargraph/rbs_translator.rb
@@ -12,7 +12,7 @@ module RbsTranslator
       'NilClass' => 'nil'
     }
 
-    # @param type [RBS::Types::Bases::Base]
+    # @param type [RBS::Types::t]
     # @return [ComplexType]
     def self.to_complex_type(type)
       tag = type_to_tag(type)
@@ -27,13 +27,10 @@ def self.to_complex_type(type)
     def self.to_parameter_pin(param_type, name, decl, closure)
       return_type = case decl
                     when :restarg
-                      # @sg-ignore RBS type understanding issue - see to_complex_type
                       RbsTranslator.to_restarg_return_type(param_type.type)
                     when :kwrestarg
-                      # @sg-ignore RBS type understanding issue - see to_complex_type
                       RbsTranslator.to_kwrestarg_return_type(param_type.type)
                     else
-                      # @sg-ignore RBS type understanding issue - to_complex_type's own param type is too narrow
                       RbsTranslator.to_complex_type(param_type.type)
                     end
       Solargraph::Pin::Parameter.new(decl: decl, name: name, closure: closure, return_type: return_type, source: :rbs, type_location: to_sg_location(param_type.location) || closure.type_location)
@@ -45,7 +42,7 @@ def self.to_parameter_pin(param_type, name, decl, closure)
     # element type isn't known (e.g. an untyped inline `#:`
     # annotation), falls back to a bare, unparameterized Array.
     #
-    # @param elem_rbs_type [RBS::Types::Bases::Base]
+    # @param elem_rbs_type [RBS::Types::t]
     # @return [ComplexType]
     def self.to_restarg_return_type elem_rbs_type
       elem_type = RbsTranslator.to_complex_type(elem_rbs_type)
@@ -56,7 +53,7 @@ def self.to_restarg_return_type elem_rbs_type
     # Likewise, the type of the local variable a kwrestarg is
     # captured into - a wrapped Hash of Symbol to its per-value type.
     #
-    # @param elem_rbs_type [RBS::Types::Bases::Base]
+    # @param elem_rbs_type [RBS::Types::t]
     # @return [ComplexType]
     def self.to_kwrestarg_return_type elem_rbs_type
       elem_type = RbsTranslator.to_complex_type(elem_rbs_type)
@@ -152,19 +149,30 @@ def self.to_sg_location(location)
     class << self
       private
 
-      # @param type [RBS::Types::Bases::Base]
+      # @param type [RBS::Types::t]
       # @return [String]
       def type_to_tag type
+        # Every branch below narrows `type` by class via `when`, but
+        # the type checker doesn't propagate that narrowing to calls
+        # inside the branch body - it still sees the full RBS::Types::t
+        # union, so calls to members that only exist on the matched
+        # class (e.g. #type, #types, #literal, #name, #args) need an
+        # inline ignore comment. Tracked at
+        # https://github.com/castwide/solargraph/issues/1241
         case type
         when RBS::Types::Optional
+          # @sg-ignore https://github.com/castwide/solargraph/issues/1241 - case/when doesn't narrow type
           "#{type_to_tag(type.type)}, nil"
         when RBS::Types::Bases::Bool
           'Boolean'
         when RBS::Types::Tuple
+          # @sg-ignore https://github.com/castwide/solargraph/issues/1241 - case/when doesn't narrow type
           "Array(#{type.types.map { |t| type_to_tag(t) }.join(', ')})"
         when RBS::Types::Literal
+          # @sg-ignore https://github.com/castwide/solargraph/issues/1241 - case/when doesn't narrow type
           type.literal.inspect
         when RBS::Types::Union
+          # @sg-ignore https://github.com/castwide/solargraph/issues/1241 - case/when doesn't narrow type
           type.types.map { |t| type_to_tag(t) }.join(', ')
         when RBS::Types::Record
           # @todo Better record support
@@ -174,13 +182,15 @@ def type_to_tag type
         when RBS::Types::Bases::Void
           'void'
         when RBS::Types::Variable
+          # @sg-ignore https://github.com/castwide/solargraph/issues/1241 - case/when doesn't narrow type
           "#{Solargraph::ComplexType::GENERIC_TAG_NAME}<#{type.name}>"
         when RBS::Types::Bases::Self, RBS::Types::Bases::Instance
           'self'
         when RBS::Types::Bases::Top
           # `Top` is the most super superclass
           'BasicObject'
         when RBS::Types::Intersection
+          # @sg-ignore https://github.com/castwide/solargraph/issues/1241 - case/when doesn't narrow type
           type.types.map { |member| type_to_tag(member) }.join(', ')
         when RBS::Types::Proc
           'Proc'
@@ -192,9 +202,11 @@ def type_to_tag type
           # `Interface represents a mix-in module which can be considered a
           # subtype of a consumer of it
           #
+          # @sg-ignore https://github.com/castwide/solargraph/issues/1241 - case/when doesn't narrow type
           type_tag(type.name, type.args)
         when RBS::Types::ClassSingleton
           # e.g., singleton(String)
+          # @sg-ignore https://github.com/castwide/solargraph/issues/1241 - case/when doesn't narrow type
           type_tag(type.name)
         when RBS::Types::Bases::Any, RBS::Types::Bases::Bottom
           # `Bottom`` is used in contexts where nothing will ever return
diff --git a/lib/solargraph/shell.rb b/lib/solargraph/shell.rb
@@ -190,7 +190,7 @@ def gems *names
         names.each do |name|
           if name == 'core'
             # @sg-ignore cache_core and core? are dynamically defined
-            PinCache.cache_core(out: $stdout) if !PinCache.core? || options[:rebuild]
+            PinCache.cache_core(out: $stdout) # if !PinCache.core? || options[:rebuild]
             next
           end
 
diff --git a/lib/solargraph/source/chain/call.rb b/lib/solargraph/source/chain/call.rb
@@ -111,7 +111,7 @@ def inferred_pins pins, api_map, name_pin, locals
                                                         gates: name_pin.gates,
                                                         source: :chain)
                 atype = atypes[idx] ||= arg.infer(api_map, arg_name_pin, locals)
-                unless (param.compatible_arg?(atype, api_map) && literal_param_arg_matches?(param, atype, api_map)) || param.restarg?
+                unless param.compatible_arg?(atype, api_map) || param.restarg?
                   match = false
                   break
                 end
@@ -186,44 +186,6 @@ def inferred_pins pins, api_map, name_pin, locals
           end
         end
 
-        # nil/true/false are technically "literal" per
-        # ComplexType::UniqueType#literal? (their non_literal_name is
-        # NilClass/TrueClass/FalseClass), but they're singletons, not
-        # dispatch-relevant values the way `0` vs `1` are for tuple
-        # indexing - excluding them keeps ordinary `T?`/nilable params
-        # (extremely common, e.g. String#split's `(Regexp | string |
-        # nil pattern)`) from tripping #literal_param_arg_matches?.
-        #
-        # @param unique_type [ComplexType::UniqueType]
-        # @return [Boolean]
-        def dispatch_literal? unique_type
-          unique_type.literal? && !%w[nil true false].include?(unique_type.name)
-        end
-
-        # Pin::Parameter#compatible_arg? alone is too permissive for
-        # picking *which* overload to use for return-type inference: it
-        # treats any Integer as "compatible" with a literal-0-typed
-        # parameter (correct for general call-validity checking - you
-        # can call `array[i]` with any Integer `i` - but wrong for
-        # overload *selection*, where it would make the first
-        # literal-typed overload always win over the safe catch-all for
-        # any argument that merely happens to be assignable to it).
-        # When the candidate overload's parameter is a literal type,
-        # require every possible value of the argument to also be
-        # literal, so a non-literal (or not-entirely-literal) argument
-        # falls through to a less specific overload instead.
-        #
-        # @param param [Pin::Parameter]
-        # @param atype [ComplexType]
-        # @param api_map [ApiMap]
-        # @return [Boolean]
-        def literal_param_arg_matches? param, atype, api_map
-          ptype = param.typify(api_map)
-          return true unless ptype.items.any? { |item| dispatch_literal?(item) }
-
-          atype.items.all?(&:literal?)
-        end
-
         # @param docstring [YARD::Docstring]
         # @param context [ComplexType]
         # @return [ComplexType, nil]
diff --git a/lib/solargraph/type_checker.rb b/lib/solargraph/type_checker.rb
@@ -434,10 +434,15 @@ def argument_problems_for chain, api_map, closure_pin, locals, location
     # @param sig [Pin::Signature]
     # @param pin [Pin::Method]
     # @param receiver_type [ComplexType] the type of the object the
-    #   method is being called on, used to resolve the restarg's
-    #   declared type (e.g. `Elem` for `Array#push`) against the
-    #   receiver's actual generic parameters (e.g. `Integer` for an
-    #   `Array<Integer>` receiver)
+    #   method is being called on. Resolving a signature's generics
+    #   (e.g. `Elem`) against the receiver's actual generic
+    #   parameters (e.g. `Integer` for an `Array<Integer>` receiver)
+    #   is a general problem, but this is currently only plumbed
+    #   through to the restarg path below (see #restarg_problems_for)
+    #   - fixed-arity params still get their types from `params`
+    #   (built by #param_details_from_stack), which doesn't resolve
+    #   against the receiver. Generalizing that is tracked as a
+    #   follow-up, not attempted here.
     #
     # @return [Array<Problem>]
     def signature_argument_problems_for location, locals, closure_pin, params, arguments, sig, pin, receiver_type
diff --git a/spec/source/chain_spec.rb b/spec/source/chain_spec.rb
@@ -236,6 +236,7 @@ class NotCorrect; end
     chain = Solargraph::Parser.chain(node, 'test.rb')
     type = chain.infer(api_map, Solargraph::Pin::ROOT_PIN, [])
     expect(type.tag).to eq('true')
+    expect(type.simplify_literals.tag).to eq('Boolean')
   end
 
   it 'infers self from Object#freeze' do
