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

Try to compile time error when trying to make non byte aligned array on js #3111

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Pi-Cla
Copy link
Contributor

@Pi-Cla Pi-Cla commented May 8, 2024

Try to detect when someone tries to make a non byte aligned array on Javascript. This compile time error still fails to trigger if the size value is a variable though. So we still need the runtime error in the Javascript template too.

Fixes #1591

@Pi-Cla Pi-Cla force-pushed the bit-array-byte branch 2 times, most recently from 69c73e8 to 3775b34 Compare May 8, 2024 18:23
@Pi-Cla
Copy link
Contributor Author

Pi-Cla commented May 8, 2024

We already de facto do not support this as seen in these lines of the Javascript template:

if (size % 8 != 0) {
const msg = `Bit arrays must be byte aligned on JavaScript, got size of ${size} bits`;
throw new globalThis.Error(msg);
}

So this change will just make that more explicit in certain circumstances

@Pi-Cla
Copy link
Contributor Author

Pi-Cla commented May 8, 2024

Also, where should I add tests for these?
(It is sometimes hard to tell where I can add tests for some of my changes such as this one...)

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.

return toBitArray([sizedInt(256, 4)]);
}

import { toBitArray } from "../gleam.mjs";
Copy link
Member

Choose a reason for hiding this comment

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

What happened to these two tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This snapshot correspond to this test:

fn explicit_sized() {
assert_js!(
r#"
fn go() {
<<256:size(4)>>
}
"#,
);
}

Since the size of this array is being set to 4 it would not be byte aligned and it would cause a runtime error. So instead this code marks that as unsupported and so removes the function it is contained in.

I guess I can rewrite these tests to have a size which is a multiple of 8 instead and also add in some new tests which use not byte aligned bit arrays.

…on js

Try to detect when someone tries to make a non byte aligned array on Javascript.
This compile time error still fails to trigger if the size value is a variable though.
So we still need the runtime error in the Javascript template too.

Fixes gleam-lang#1591
It seems our current behaviour is to assume a negative size really means a size of 0 so might as well document this with some tests
@Pi-Cla
Copy link
Contributor Author

Pi-Cla commented May 10, 2024

The tests have been added. I also added extra tests which demonstrate how negatively sized bit arrays are currently handled in both Erlang and Javascript. Which is that for both targets we just assume the developer meant a bit array of size 0.

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.

Currently bitstring segment size must be multiple of 8 when compiling to JavaScript, add compiletime check
2 participants