Skip to content
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

Store raw string for numbers in the AST #108

Open
jfmengels opened this issue Feb 2, 2021 · 2 comments
Open

Store raw string for numbers in the AST #108

jfmengels opened this issue Feb 2, 2021 · 2 comments

Comments

@jfmengels
Copy link
Collaborator

Currently, if we try to parse the following code

a = 100000000000000000000

we'll get an AST with the following node

Integer 9.999999999999999e+29

It's not much, but there is a precision loss. If we were to write that number using the writer, we would not get the same output as the input.

My thinking is that we could keep the original "text" in the same node, like

Integer 9.999999999999999e+29 "100000000000000000000"

and then the writer could re-use that original text from the AST. elm-review rules could also use this information for other reasons, though I can't think of any.

Downside: It's possible to have different values represented by a and b in Integer a b (though that is also true currently in a way...), but now it's even easier because you can do Integer 1 "2", or even Integer 1 "".

I think the Floatable variant should use the same approach.

I'm thinking we could remove the Hex variant in favor of Integer Int String. I think in most cases (at least in elm-review rules), we don't care how an integer is written but we care about the value more. If we care about integers written in hex, it's reasonably easy to check whether the string starts with 0x.

If Elm ever adds support for writing numbers like 10_000, then we will need to update the parser but we won't need to change the Expression type.

(This was inspired by issue avh4/elm-format#690, where the problem seems to appear for smaller numbers than this one.)

@MartinSStewart
Copy link
Collaborator

Will a user want to do math with the Integer or Floatable value? If it contains a string instead of an Int or Float then we've left it up to them to parse the string and they might forget to handle edge cases like hex values. Maybe we could instead have an opaque type that contains the number and helper functions like isHex: IntegerValue -> Bool, toInt : IntegerValue -> Maybe Int, toString : IntegerValue -> String, etc?

Viir added a commit to pine-vm/pine that referenced this issue Feb 26, 2022
Add tests to document what currently works and what doesn't.
Also, remove a precision loss in the function to map to JSON values: Encode integers as strings to avoid precision loss here.

The current representation in the Elm syntax parser causes a precision loss for integer literals: https://github.com/stil4m/elm-syntax/blob/3d4ee78c007ee8987b8cfe6c3ea07e5500632b6b/src/Elm/Syntax/Expression.elm#L115-L116
For discussion of this issue, see also stil4m/elm-syntax#108
Viir added a commit to pine-vm/pine that referenced this issue Feb 26, 2022
Add tests to document what currently works and what doesn't.
Also, remove a precision loss in the function to map to JSON values: Encode integers as strings to avoid precision loss here.

The current representation in the Elm syntax parser causes a precision loss for integer literals: https://github.com/stil4m/elm-syntax/blob/3d4ee78c007ee8987b8cfe6c3ea07e5500632b6b/src/Elm/Syntax/Expression.elm#L115-L116
For discussion of this issue, see also stil4m/elm-syntax#108
@jfmengels
Copy link
Collaborator Author

I don't think people will do math on the expressions no, but I think people will potentially want to pattern match on the numbers Integer 1 _ ->, which is not possible with an opaque type.

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

No branches or pull requests

2 participants