Skip to content

Fix unexpected empty source map stacks - #1625

Merged
lsegal merged 1 commit into
lsegal:mainfrom
waysact:first-el-of-beginless-range
Dec 5, 2025
Merged

Fix unexpected empty source map stacks#1625
lsegal merged 1 commit into
lsegal:mainfrom
waysact:first-el-of-beginless-range

Conversation

@jscheid

@jscheid jscheid commented Aug 25, 2025

Copy link
Copy Markdown
Contributor

Description

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.

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@jscheid
jscheid marked this pull request as draft August 25, 2025 18:27
@jscheid
jscheid force-pushed the first-el-of-beginless-range branch 2 times, most recently from ecd8785 to 537ae6c Compare August 25, 2025 19:31
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 lsegal#1603.

Co-Authored-By: Claude <noreply@anthropic.com>
@jscheid
jscheid force-pushed the first-el-of-beginless-range branch from 537ae6c to c6b0b16 Compare August 25, 2025 19:41
@jscheid
jscheid marked this pull request as ready for review August 25, 2025 19:43
@lsegal
lsegal merged commit 1a4f63d into lsegal:main Dec 5, 2025
21 checks passed
@lsegal

lsegal commented Dec 5, 2025

Copy link
Copy Markdown
Owner

Sorry for the delay, thank you for the fix!

@jscheid

jscheid commented Dec 5, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for the merge @lsegal. I think you've somehow stripped out my authorship though? I don't mind too much in this case but perhaps next time you could use git commit --author=...

Screenshot 2025-12-05 at 22 41 30

@jscheid
jscheid deleted the first-el-of-beginless-range branch December 5, 2025 21:42
@lsegal

lsegal commented Dec 5, 2025

Copy link
Copy Markdown
Owner

@jscheid you're looking at the merge commit. Your individual commits are retained: c6b0b16

@jscheid

jscheid commented Dec 6, 2025

Copy link
Copy Markdown
Contributor Author

@lsegal 🤦 sorry, I got thrown off because you've (or your tooling has) copied the commit message, which is nice actually - I'm used to seeing GitHub's Merge pull request #12345 from branch-name.

Thanks again!

@lsegal

lsegal commented Dec 6, 2025

Copy link
Copy Markdown
Owner

@jscheid this is actually GitHubs tooling, I just clicked merge from the UI as I usually do. It contains the issue data, although maybe this is new for them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

YARD rake task crash

2 participants