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 primitive slice js #722

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

jcrist1
Copy link
Contributor

@jcrist1 jcrist1 commented Nov 4, 2024

This PR attempts to address #699. According to the generated ts files primitive slices are returned as an Array<number> on the js side, but the generated js code looks like it's trying to return types of Float64Array. I personally think it makes more sense to return typed arrays which have a perfectly ergonomic API and allow the caller to decide what to do with, but don't really want to challenge the existing implementation. Just checking that I understand it correctly. I had to add an Array.from wrap to get that to work. However, it appears there is a bug in the actual interpretation of the raw bytes that causes the first byte to not be interpreted correctly, I tried to get to the bottom of it, but couldn't quickly figure it out, so I thought I'd open the PR, and solicit help. Does anyone know what might be going on, and why the wasm backed array seems to be incorrect?

});

test("Float64Vec", (t) => {
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 test fails on my machine:

  slices-ts › Float64Vec

  test/slices-ts.mjs:14

   13:     let data = Float64Vec.newFromOwned(input);
   14:     t.is(data.borrow(), input);
   15: });

  Difference (- actual, + expected):

    [
  -   2.26666497653787e-308,
  +   1,
      2,
      3,
      4,
      5,
    ]

  › file://test/slices-ts.mjs:14:7

@jcrist1
Copy link
Contributor Author

jcrist1 commented Nov 4, 2024

I'm going to open this up as ready for review despite the failing test, in the hope that a reviewer can point me out to where the byte interpretation goes wrong. I got as far as seeing that, the value - 2.26666497653787e-308, seems to come from the uninitialized bytes, but as far as I could see the bytes get correctly written to when the array is passed to the native call.

@jcrist1 jcrist1 marked this pull request as ready for review November 4, 2024 20:59
@jcrist1
Copy link
Contributor Author

jcrist1 commented Nov 4, 2024

Okay I think I realised the issue. The method newFromOwned creates a slice and which is the backing Vec of the Float64Vec, but we free this slice as part of the method call, in

...
    } finally {
      functionCleanupArena.free();
    }

I didn't actually check if the js backend supports owned slices in parameters. I've got a better test now.

Copy link
Member

@ambiguousname ambiguousname left a comment

Choose a reason for hiding this comment

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

Generally looks good to me!

Thanks for adding new tests as well, very much appreciated.

feature_tests/js/test/slices.mjs Show resolved Hide resolved
Co-authored-by: Tyler K <ambiguousemails@proton.me>
@Manishearth Manishearth merged commit 7a2fbdc into rust-diplomat:main Nov 5, 2024
20 checks passed
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.

3 participants