Skip to content

Commit c6b0b16

Browse files
jscheidclaude
andcommitted
Fix unexpected empty source map stacks
This fixes two unrelated issues that both previously presented with a similar symptom, namely the following error: RangeError: cannot get the first element of beginless range The first issue is that some constructs, such as `{} => {}` and `foo in bar` get parsed by Ripper as a `case` AST node. However, without a matching `case` keyword, the YARD parser doesn't know where it starts. The second issue is that tokens following the `def` keyword are (correctly) never treated as keywords themselves, because they are used as the method name instead. However, this logic erroneously also applied to keywords following the `:def` _symbol_ - again making the YARD parser unable to determine where the keyword (following the `:def` symbol) starts. In both cases, the problematic constructs by themselves did not cause a crash but merely provided wrong source ranges. However, in specific parser states, the corresponding `@map` entry is the empty array rather than nil (because a similar, valid, construct was encountered before, which initializes the map entry). This was causing crashes since the beginning of `line_range` and `source_range` becomes nil as a result. Here we're fixing the underlying issue and we're also adding specs to ensure that source ranges are correct, and that crashes in the presence of pre-initializing constructs are eliminated. Note that another fix would have been to treat empty `@map` entries the same as nil `@map` entries in `visit_event`. Doing so would also prevent the crashes, and improve robustness more generally, however it would mask potential similar problems with other constructs. Therefore I have refrained from making that change -- a debatable decision that might be revisited in a separate change. However, I've added a better error message in this case. DISCLOSURE: Claude wrote large parts of the specs and helped me understand the problems and create the fixes, but I'm fully adopting its output as my own (after polishing it manually and reviewing it.) Fixes #1603. Co-Authored-By: Claude <[email protected]>
1 parent e96ae99 commit c6b0b16

File tree

2 files changed

+112
-2
lines changed

2 files changed

+112
-2
lines changed

lib/yard/parser/ruby/ruby_parser.rb

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,18 @@ def on_sp(tok)
236236

237237
def visit_event(node)
238238
map = @map[MAPPINGS[node.type]]
239-
lstart, sstart = *(map ? map.pop : [lineno, @ns_charno - 1])
239+
240+
# Pattern matching and `in` syntax creates :case nodes without 'case' tokens,
241+
# fall back to the first child node.
242+
if node.type == :case && (!map || map.empty?) && (child_node = node[0])
243+
lstart = child_node.line_range.first
244+
sstart = child_node.source_range.first
245+
else
246+
lstart, sstart = *(map ? map.pop : [lineno, @ns_charno - 1])
247+
end
248+
249+
raise "Cannot determine start of node #{node} around #{file}:#{lineno}" if lstart.nil? || sstart.nil?
250+
240251
node.source_range = Range.new(sstart, @ns_charno - 1)
241252
node.line_range = Range.new(lstart, lineno)
242253
if node.respond_to?(:block)
@@ -259,7 +270,10 @@ def visit_event_arr(node)
259270
def visit_ns_token(token, data, ast_token = false)
260271
add_token(token, data)
261272
ch = charno
262-
@last_ns_token = [token, data]
273+
274+
# For purposes of tracking parsing state, don't treat keywords as such
275+
# where used as a symbol identifier.
276+
@last_ns_token = [@last_ns_token && @last_ns_token.first == :symbeg ? :symbol : token, data]
263277
@charno += data.length
264278
@ns_charno = charno
265279
@newline = [:semicolon, :comment, :kw, :op, :lparen, :lbrace].include?(token)

spec/parser/ruby/ruby_parser_spec.rb

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -553,5 +553,101 @@ def add(x) = x + 1
553553

554554
expect(Registry.at('A#add').docstring).to eq('Adds two numbers')
555555
end if RUBY_VERSION >= '3.'
556+
557+
it "doesn't crash with pattern matching following a case statement" do
558+
code = <<-RUBY
559+
case value
560+
when 1
561+
"number"
562+
end
563+
564+
{} => {}
565+
RUBY
566+
567+
expect {
568+
YARD::Parser::Ruby::RubyParser.new(code, nil).parse
569+
}.not_to raise_error
570+
end if RUBY_VERSION >= '3.'
571+
572+
it "doesn't crash with `next` following `:def` symbol after initial `next`" do
573+
code = <<-RUBY
574+
foo do
575+
next
576+
if :def
577+
next
578+
end
579+
end
580+
RUBY
581+
582+
expect {
583+
YARD::Parser::Ruby::RubyParser.new(code, nil).parse
584+
}.not_to raise_error
585+
end
586+
587+
it "doesn't crash with mixed pattern matching syntaxes" do
588+
code = <<-RUBY
589+
case foo
590+
in bar
591+
return
592+
end
593+
return if foo && foo in []
594+
RUBY
595+
596+
expect {
597+
parser = YARD::Parser::Ruby::RubyParser.new(code, nil)
598+
parser.parse
599+
}.not_to raise_error
600+
end if RUBY_VERSION >= '2.7'
601+
602+
it "provides correct range for various pattern matching statements" do
603+
patterns = [
604+
"{} => {}",
605+
"{a: 1} => {b: 2}",
606+
"{x: 'test'} => result",
607+
"foo in []"
608+
]
609+
610+
patterns.each do |pattern|
611+
parser = YARD::Parser::Ruby::RubyParser.new(pattern, nil)
612+
ast = parser.parse.root
613+
614+
case_node = nil
615+
ast.traverse do |node|
616+
if node.type == :case
617+
case_node = node
618+
break
619+
end
620+
end
621+
622+
expect(case_node).not_to be_nil, "Pattern #{pattern} should create a case node"
623+
actual_text = pattern[case_node.source_range]
624+
expect(actual_text).to eq(pattern),
625+
"Pattern #{pattern} should have source range covering the full expression, got #{actual_text.inspect}"
626+
end
627+
end if RUBY_VERSION >= '3.'
628+
629+
it "provides correct range for `next` statement following `def` _symbol_" do
630+
code = <<-RUBY
631+
foo do
632+
if :def
633+
next
634+
end
635+
end
636+
RUBY
637+
638+
parser = YARD::Parser::Ruby::RubyParser.new(code, nil)
639+
ast = parser.parse.root
640+
641+
next_node = nil
642+
ast.traverse do |node|
643+
if node.type == :next
644+
next_node = node
645+
break
646+
end
647+
end
648+
649+
expect(next_node.line_range).to eq(3..3)
650+
expect(code[next_node.source_range]).to eq('next')
651+
end
556652
end
557653
end if HAVE_RIPPER

0 commit comments

Comments
 (0)