Skip to content

Commit 2190a35

Browse files
apiologyclaude
andcommitted
Replace generic nil-check suppressions with real fixes/explanations
Every "Need to add nil check here" ignore this PR had introduced is now either gone or replaced with a comment explaining why a real check is not needed: - Fixed the actual bug: type_name did not handle a :cbase root (the leading '::' in a fully-qualified constant like ::Integer), so `x.is_a?(::Foo)` guards never narrowed anywhere in this file -- parsing '::Foo' silently produced no type name at all. That is why the node.is_a?(::Parser::AST::Node) guard at the top of parse_receiver_chain was not narrowing node for the rest of the method. Fixing it made 7 of 9 ignores in that method unnecessary. - Added a real nil-check for the one Array#[range] slice that is legitimately nilable per its own type (children[2..].empty?). - The remaining two ignores (a node.children element, and Range.from_node(node).start) get explanatory comments instead of the generic placeholder -- both match an existing, already-accepted pattern elsewhere in this same file. Also added a regression spec for the type_name fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGu6zb5faStTC754PxMUSA
1 parent 9445d1f commit 2190a35

2 files changed

Lines changed: 36 additions & 11 deletions

File tree

lib/solargraph/parser/flow_sensitive_typing.rb

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -264,26 +264,24 @@ def process_expression expression_node, true_ranges, false_ranges
264264
# 'location'], or nil if `node` doesn't have this shape.
265265
def parse_receiver_chain node
266266
return unless node.is_a?(::Parser::AST::Node)
267-
# @sg-ignore Need to add nil check here
268267
return [node.children[0].to_s] if %i[lvar ivar].include?(node.type)
269-
# @sg-ignore Need to add nil check here
270268
return unless node.type == :send
271-
# no arguments
272-
# @sg-ignore Need to add nil check here
273-
return unless node.children[2..].empty?
269+
# no arguments -- children[2..] is only nil (rather than [])
270+
# if the start index is out of bounds, which can't happen here
271+
return unless (node.children[2..] || []).empty?
274272

275-
# @sg-ignore Need to add nil check here
276273
method_name = node.children[1]
277-
# @sg-ignore Need to add nil check here
278274
return unless method_name.is_a?(Symbol)
279275

280-
# @sg-ignore Need to add nil check here
281276
receiver = node.children[0]
282277
# bare call, e.g. `s(:send, nil, :foo)` - implicit self, so
283278
# 'foo' could be a local variable or a 0-arg method on self
284-
# @sg-ignore Need to add nil check here
285279
return [method_name.to_s] if receiver.nil?
286280

281+
# @sg-ignore a :send node's receiver (children[0]) is nil or an
282+
# AST::Node, never any of the other types children[] can hold
283+
# for other node shapes (e.g. Symbol, Array) -- and
284+
# parse_receiver_chain re-checks node.is_a?(...) itself anyway
287285
base = parse_receiver_chain(receiver)
288286
return unless base
289287

@@ -305,7 +303,10 @@ def parse_call call_node, method_name
305303
call_receiver = call_node.children[0]
306304
call_arg = type_name(call_node.children[2])
307305

308-
# @sg-ignore Need to add nil check here
306+
# @sg-ignore node.children is typed as a broad union (it can hold
307+
# nested arrays/symbols for other node shapes), but
308+
# parse_receiver_chain re-checks node.is_a?(::Parser::AST::Node)
309+
# itself and safely returns nil for anything else
309310
chain_words = parse_receiver_chain(call_receiver)
310311
return unless chain_words
311312

@@ -519,7 +520,9 @@ def process_call_chain node, true_presences, false_presences
519520
chain_words = parse_receiver_chain(node)
520521
return if chain_words.nil? || chain_words.length < 2
521522

522-
# @sg-ignore Need to add nil check here
523+
# @sg-ignore Range.from_node is nil only for a node without
524+
# source location info, which doesn't happen for real parsed
525+
# nodes reaching here (same as isa_position/nilp_position above)
523526
position = Range.from_node(node).start
524527

525528
pin = chain_pin(chain_words, node, position)
@@ -551,6 +554,9 @@ def type_name node
551554
class_node = node.children[1]
552555

553556
return class_node.to_s if module_node.nil?
557+
# e.g., the '::' in '::Baz' or '::Foo::Baz' -
558+
# s(:const, s(:cbase), :Baz)
559+
return "::#{class_node}" if module_node.type == :cbase
554560

555561
module_type_name = type_name(module_node)
556562
return unless module_type_name

spec/parser/flow_sensitive_typing_spec.rb

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1103,4 +1103,23 @@ def bundled_filename
11031103
clip = api_map.clip_at('test.rb', [19, 26])
11041104
expect(clip.infer.rooted_tags).to eq('::String')
11051105
end
1106+
1107+
it 'uses is_a? with a fully-qualified type name to refine types' do
1108+
source = Solargraph::Source.load_string(%(
1109+
# @param x [Object]
1110+
def verify_repro(x)
1111+
if x.is_a?(::Integer)
1112+
x
1113+
else
1114+
x
1115+
end
1116+
end
1117+
), 'test.rb')
1118+
api_map = Solargraph::ApiMap.new.map(source)
1119+
clip = api_map.clip_at('test.rb', [4, 10])
1120+
expect(clip.infer.rooted_tags).to eq('::Integer')
1121+
1122+
clip = api_map.clip_at('test.rb', [6, 10])
1123+
expect(clip.infer.rooted_tags).to eq('::Object')
1124+
end
11061125
end

0 commit comments

Comments
 (0)