Parse Amount string values with StrNumericLiteral, extended to accept "NaN" - #134
Open
jessealama wants to merge 10 commits into
Open
Parse Amount string values with StrNumericLiteral, extended to accept "NaN"#134jessealama wants to merge 10 commits into
jessealama wants to merge 10 commits into
Conversation
|
Collaborator
Author
|
Relates to (but does not close) #135 |
gibson042
suggested changes
Jul 12, 2026
Comment on lines
+79
to
+80
| or the string "NaN", in either case optionally surrounded by white space; | ||
| anything else (including the empty string and strings consisting solely of white space) throws a RangeError. |
Member
There was a problem hiding this comment.
cf. #133 (comment)
Numeric consumption of empty and whitespace-only strings as
0has been part of the language for a very long time. I'm only comfortable withAmountdeviating from that if it goes all the way by also rejecting leading and/or trailing whitespace.
jessealama
force-pushed
the
string-amount-literal-grammar
branch
from
July 15, 2026 12:37
692d979 to
df7f58b
Compare
The ~not-a-number~ branch was unreachable: |StringNumericLiteral| cannot match "NaN", so parsing threw a RangeError before the branch could run. Handle the exact String "NaN" explicitly, storing the Number value NaN. Remove the dead branch. Any other String, including "NaN" surrounded by white space, must parse as a numeric literal or a RangeError is thrown. The empty String, and Strings consisting solely of white space, were accepted as zero via the StrWhiteSpace-only production. Reject them with a RangeError by requiring a StrNumericLiteral Parse Node.
Suggested by Richard Gibson in review of #130.
jessealama
force-pushed
the
string-amount-literal-grammar
branch
from
July 15, 2026 13:12
df7f58b to
d1da52d
Compare
jessealama
changed the title
gibson042
suggested changes
Jul 15, 2026
gibson042
left a comment
gibson042
left a comment
Member
There was a problem hiding this comment.
If the changes are deemed appropriate (which is up for debate), I would support this PR as the mechanism for accomplishing them.
Collaborator
Author
I've added a comment to #132. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Amount constructor now parses String values with ECMA-262's
StrNumericLiteralproduction, which this proposal extends with aNaNalternative and matchingStringNumericValueandStringIntlMVrules. The string"NaN"is now accepted, while the empty string, whitespace-only strings (to which the runtime semantics ofStringNumericLiteralassign the value 0), and strings with leading or trailing white space all throw a RangeError.An earlier version of this PR defined a dedicated grammar for acceptable strings; review feedback simplified that to extending an existing ECMA-262 production.
Relates to #132. This change maintains support for
"NaN"as an argument to the constructor, just making clear that we don't support"NaN"with leading or trailing whitespace. If we want to drop support for"NaN"(and"Infinity"and"-Infinity"), which I think is reasonable, we can tackle that in a different PR.Closes #133