-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct interpretation of mathematical value #72
Conversation
I wrote this based on https://262.ecma-international.org/12.0/#sec-runtime-semantics-mv-s to extract the "private" MV variable out of that abstract operation, but it appears that the abstract operation has since changed. I'm not exactly sure the right way to write this part of the spec text; I would appreciate feedback from @bakkot @waldemarhorwat et al. |
I'm happy to suggest a fix here, but to do that I'd need to know the resolution of the normative question in #54 so I know which grammar to use for the strings. Also, do you intend to allow leading and trailing whitespace in the string? If you don't intend to accept hex or octal literals, and you don't intend to accept leading or trailing whitespace, and you do intend to accept the literal strings "Infinity", "+Infinity", and "-Infinity" (as well as such oddities as "+0" and "-0"), then you can do
If you want to accept a different set of strings, you'll need to use a different grammar or modify the algorithm. For example, if you want to accept leading and trailing whitespace, you could either define a new grammar along the lines of the |StringIntegerLiteral| grammar now used for StringToBigInt, or you could add an initial |
We discussed this in the ECMA-402 meeting this week and decided to (1) accept all grammars of numbers that are accepted in the |
To be clear: we don't want to change what we currently do in |
OK, sounds good. In that case, |StringNumericLiteral| is the right grammar to use, but some additional work will be needed because ECMA-262 does not currently produce arbitrary-precision mathematical values from string literals (plus you need to represent You can pretty much just copy-paste
Unfortunately I don't see a good way for 262 to rewrite |
I think |
I was thinking about that, but there's a couple awkward parts:
For both reasons, I don't see a straightforward way to make StringNumericValue defer to StringIntlMV - you end up needing most of the same cases anyway, I think. I'll bring it up at the next editor call, though. |
If 262 does something special with hex/octal/binary, then we should do that same special thing in Intl. The opinion of the ECMA-402 delegates call yesterday was basically to align as closely as possible to the existing behavior. We don't want to introduce anything novel, except for retaining infinite precision on decimals (and hex/octal/binary if and only if that makes sense in the context of the grammar). I would really like to avoid if possible copying over the whole grammar, specifically because the purpose is to have the grammar be identical to 262 except for the precision. |
The special thing that it does for hex/octal/binary vs for decimal is require implementations to use the exact nearest possible float, whereas for decimal literals with more than 20 significant digits implementations are permitted to choose either of the nearest two. This isn't relevant to this proposal, which requires arbitrary precision and which does not produce floats.
You shouldn't need to copy the grammar, just the operation which interprets it. And copying that part is hard to avoid, because this proposal has a different interpretation (arbitrary precision) than 262 does (floats, with specific rules around which floats are allowed for which forms). But like I said, I'll bring it up at our call on Wednesday and see if the other editors have a better suggestion. |
Talked about it with the other 262 editors today and we didn't see a better option than just copy-pasting StringNumericValue and modifying it to return an Intl math value. I do want to emphasize that you do not need to copy the grammar; |
@@ -1018,7 +1018,9 @@ | |||
1. Else, | |||
1. Let _str_ be ! Number::toString(_x_). | |||
1. If the grammar cannot interpret _str_ as an expansion of |StringNumericLiteral|, return *NaN*. | |||
1. Let _mv_ be the MV, a mathematical value, of ? ToNumber(_str_), as described in <emu-xref href="#sec-runtime-semantics-mv-s"></emu-xref>. | |||
1. Let _text_ be ! StringToCodePoints(_str_). | |||
1. Let _literal_ be ParseText(_text_, |StringNumericLiteral|). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @jugglinmike thanks for your PR, it might be nice to update grammar according to @bakkot suggestion and also add this check?
1. Let _literal_ be ParseText(_text_, |StringNumericLiteral|). | |
1. Let _literal_ be ParseText(_text_, |StringNumericLiteral|). | |
1. If _literal_ is a List of errors, let _mv_ be *NaN*. |
Thanks again 🙏🙏
To avoid confusion, I am closing this PR, because it is superseded by #82. |
Step 6 in ToIntlMathematicalValue reads:
If I understand correctly, this is the critical operation to address gh-334, but if so, I don't understand how it achieves this.
The reference to "mathematical value" confuses me because neither of ToNumber nor sec-runtime-semantics-mv-s (i.e. StringNumericValue) produce a mathematical value. It may be that this is intended to describe a conversion, but it seems like converting from a Number value to a mathematical value will not address the motivating use case because the undesirable rounding has already taken place.
This might be addressed by the phrase
as described in <emu-xref href="#sec-runtime-semantics-mv-s"></emu-xref>
since I admittedly don't understand that, either. It appears to be applied to the result of ToNumber, but as an syntax-directed operation, I don't know how that works. On the one hand, the StringToNumber abstract operation seems like a more conventional way to use the SDO, but the description of that operation asserts, "It returns a Number."I'm not convinced that this patch is correct or even necessary, but I figured it might at least help folks understand my confusion.