-
Notifications
You must be signed in to change notification settings - Fork 563
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
Extend CoreFn's Int literal encoding with a stringified value #4506
Comments
Not sure if this helps you, but |
Reviver functions don’t help here because it’s already a JSON Number. Reviver functions can’t recover the original JSON syntax in order to load a Number as a BigInt without losing precision. |
@natefaubion You may be interested in the Stage 3 proposal JSON.parse source text access which makes this possible through revivers. Stage 3 means recommended for implementation, and it is already available in Chrome stable. |
Very cool! |
I still think it would be easier to just encode the value as a String. |
@natefaubion While this issue is for an Integer value, should the same be done on a Number value, too? |
I don't think so. Number values are |
This might be a slightly different incarnation of this issue I just came across while compiling the |
Summary
CoreFn's integer encoding encodes the
Integer
(unbounded) as a JSON Number literal. This is technically valid as JSON does not require Number bounds, but makes tooling across languages complicated. For example, stock JSON tooling for JS will only support a 64bit double (52 bit max integer), and anything outside that range is difficult to parse.Motivation
It's not possible for tools like
purescript-backend-optimizer
to parse CoreFn JSON for backends which use unbounded integers with any of the stock JSON tooling available in the PureScript ecosystem as they all rely on builtinJSON.parse
.Proposal
purescript/src/Language/PureScript/CoreFn/ToJSON.hs
Lines 63 to 66 in 8ede652
Extend this with a
stringValue
field which prints the value as a String, making it easier for tooling to parse this as a BigInt of some kind. The additional field is necessary to keep CoreFn backwards compatible.The text was updated successfully, but these errors were encountered: