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

Fix several edge cases with negative Int values in bit arrays on JavaScript #3784

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

richard-viney
Copy link
Contributor

@richard-viney richard-viney commented Nov 4, 2024

This PR fixes incorrect bit arrays being generated for negative Int values on JavaScript in two scenarios:

  1. When the size is greater than 48 bits (i.e. it goes beyond JavaScript's safe integer size). This makes <<-1:64>> give the correct result.

  2. When the value is less than the minimum representable integer for the specified size. This makes <<-80_000:16>> give the correct result.

Also fixed is pattern matching on signed Int values in bit arrays when the size is greater than 48 bits and the value being read is negative, e.g. let assert <<-1:64-signed>> = <<255, 255, 255, 255, 255, 255, 255, 255>> now passes on JavaScript the same as Erlang.


The principle now being followed is that, on JavaScript, one should only have to deal with unsafe integer behaviour when an Int's value is outside JavaScript's safe range, and that the size/width used when encoding into or pattern matching on a bit array shouldn't matter if the Int's value lies within the safe range. i.e. 128-bit wide integer segments are now supported in bit array expressions and patterns on JS, just so long as the value of the integer being encoded/matched is within the safe integer range.

Hope that makes sense!

I think this is the best we can do without changing Int to be a BigInt on JavaScript, which is an interesting idea that would better match Erlang, but AFAICT is a non-starter from an FFI backwards compatibility perspective.

Note that BigInt now appears in the JS prelude. An effort is made to avoid it where possible as it's slower, though this does come at the cost of duplicating most of the body of byteArrayToInt(). If BigInt was always used the code would be simpler, but presumably slower in cases where it wasn't required (I haven't done any performance measurements).

@richard-viney richard-viney marked this pull request as ready for review November 4, 2024 04:28
@richard-viney richard-viney marked this pull request as draft November 4, 2024 20:38
@richard-viney richard-viney changed the title Fix bit array encoding of negative Int values wider than 48 bits on JS Fix bit array encoding of certain negative Int values on JavaScript Nov 4, 2024
@richard-viney richard-viney force-pushed the fix-negative-wide-ints branch 2 times, most recently from ecf550c to 7f0882a Compare November 4, 2024 23:50
@richard-viney richard-viney marked this pull request as ready for review November 4, 2024 23:54
@richard-viney richard-viney changed the title Fix bit array encoding of certain negative Int values on JavaScript Fix several edge cases with negative Int values in bit arrays on JavaScript Nov 5, 2024
@richard-viney richard-viney force-pushed the fix-negative-wide-ints branch 3 times, most recently from 1b58d30 to e9ca816 Compare November 5, 2024 21:42
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@lpil lpil enabled auto-merge (rebase) November 6, 2024 14:16
@lpil lpil merged commit e99ad1d into gleam-lang:main Nov 6, 2024
12 checks passed
@richard-viney richard-viney deleted the fix-negative-wide-ints branch November 6, 2024 20:23
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