Skip to content

Report the first invalid underscore in number literals consistently - #4372

Open
eitoball wants to merge 1 commit into
ruby:mainfrom
eitoball:fix-number-underscore-position
Open

eitoball wants to merge 1 commit into
ruby:mainfrom
eitoball:fix-number-underscore-position

Conversation

@eitoball

Copy link
Copy Markdown

What

Make every number literal path report the same invalid underscore, so the error location and type no longer depend on whether the literal has a base prefix. This PR picks "first invalid underscore, inner wins over trailing"; see the question below.

Why

Two code paths decide where an invalid underscore is reported, and they follow different rules:

  • The fast path for unprefixed decimal integers in lex_numeric (added in 20e626a) records the first invalid underscore. A trailing underscore is only recorded if nothing was found before it, so "inner" wins.
  • pm_strspn_number_kind_underscores in src/char.c overwrites the pointer on every hit. It is used for prefixed integers (0b/0o/0/0d/0x), fractions and exponents. It records the last invalid underscore, and a trailing underscore always wins.

So equivalent literals disagree:

Source Before After (this PR)
1__2__3 inner @ 2 inner @ 2
0d1__2__3 inner @ 7 inner @ 4
1__2_ inner @ 2 inner @ 2
0d1__2_ trailing @ 6 inner @ 4
0b1__0__1 inner @ 7 inner @ 4
1.2__3__4 inner @ 7 inner @ 4
1e1__2__3 inner @ 7 inner @ 4

The comments in src/char.c disagree with each other, too. All of them were added together in 35da3d1 (#1507):

  • pm_strspn_number_kind_underscores says it reports "the location of the last invalid underscore character", which matches its implementation.
  • pm_strspn_{binary,octal,decimal,hexadecimal}_number say "the invalid pointer is set to the index of the first invalid underscore".

As far as I can tell, the existing tests never pinned this down. Every fixture has exactly one invalid underscore. The inner/trailing distinction came later, in #2759, and only classifies the pointer that the scan already chose, so "trailing wins" looks like a side effect of the scan order rather than a design choice.

Only the reported location and error type change; whether a literal is accepted or rejected does not.

Question for maintainers

Which rule do you prefer?

  • A. First, inner wins (this PR). Matches the fast path, which most decimal literals go through, and the comments on the public pm_strspn_*_number functions. It does not change the current behavior for unprefixed decimals.
  • B. Last, trailing wins. Matches the helper's own comment and prism 1.8.x, where unprefixed decimals also went through the helper. This would change the fast path instead and update the four public comments.

If "last" was intentional, I'm happy to switch this PR to B. The new fixtures would only need their expected positions updated.

Changes

  • src/char.c: in pm_strspn_number_kind_underscores, only write *invalid when it is still NULL, both inside the loop and for the trailing underscore. All callers (pm_strspn_*_number_validate in src/prism.c) initialize it to NULL. Changed the helper's comment from "last" to "first".
  • test/prism/errors/invalid_number_underscores_13.txt – _22.txt: new fixtures in which each literal has two invalid positions. They cover each base (none, 0d, 0b, 0o, 0, 0x), fractions and exponents, and pair unprefixed and prefixed decimals to show that both now get the same relative position and type. Existing fixtures are unchanged.

🤖 Generated with Claude Code

The fast path for unprefixed decimal integers added in 20e626a reports
the first invalid underscore and prefers "inner" over "trailing", while
pm_strspn_number_kind_underscores (used for prefixed integers, fractions
and exponents) overwrote the pointer on every hit, reporting the last one
and preferring "trailing". As a result, 1__2__3 and 0d1__2__3 pointed at
different relative positions, and 1__2_ and 0d1__2_ even produced
different error types.

Only write the pointer when it is still unset, so every path follows the
rule already documented on the pm_strspn_*_number functions. All callers
initialize the pointer to NULL, so reading it is safe.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kddnewton

Copy link
Copy Markdown
Collaborator

I see that there is that inconsistency, but does it really matter? I'm struggling to see how this could impact anyone.

This branch has not been deployed

No deployments
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.

2 participants