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

Struct Object Constructors #723

Merged
merged 19 commits into from
Nov 7, 2024

Conversation

ambiguousname
Copy link
Member

Fixes #719.

@ambiguousname ambiguousname added the B-js JS backend label Nov 6, 2024
Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

much nicer, thanks!!

@Manishearth
Copy link
Contributor

In particular I appreciate moving the from-ffi logic out of the constructor and making it purely internal. Good call.

@ambiguousname
Copy link
Member Author

Something to test: JS tests are passing on the CI, but not on my machine

@ambiguousname
Copy link
Member Author

ambiguousname commented Nov 7, 2024

Uh oh. Don't have a lot of time to test right now, but @Manishearth looks like there's a bug in the padding logic.

But it seems not to be an issue on the CI?

Here's the error I get:

  Error {
    message: `wasm panicked at feature_tests\\src\\structs.rs:233:13:␊
    assertion \`left == right\` failed␊
      left: 0␊
     right: 414`,
  }

And if I adjust the following line in ScalarPairWithPadding.mjs:

_intoFFI(
        functionCleanupArena,
        appendArrayMap,
        forcePadding
    ) {
        return [this.#first, ...diplomatRuntime.maybePaddingFields(forcePadding, 3 /* x i8 */), this.#second]
    }

So that there is no padding, then no error is returned and all assertions pass. I'm not sure what the difference is between the CI and my machine is, except maybe the WASM target version? I'll have to do some digging on this though when I have time later.

@Manishearth
Copy link
Contributor

@ambiguousname does main fail for you too, locally?

I think we just run on latest stable on CI, and on MSRV. Maybe nightly changed something

@ambiguousname
Copy link
Member Author

ambiguousname commented Nov 7, 2024

It could be something to do with nightly? I'll have to look into it when I have some more time.

I'll also have to test on main, haven't done that yet.

@ambiguousname
Copy link
Member Author

This is really strange. After uninstalling a bunch of toolchains and switching my default toolchain a few times, the error is totally gone. Even when setting it back to the default toolchain I was using before.

Weird! It's fixed now though, so I think we're okay.

@Manishearth
Copy link
Contributor

Hmm, I'm also having trouble reproducing it

@ambiguousname
Copy link
Member Author

I figured out what's causing it. New issue: #725

@ambiguousname ambiguousname marked this pull request as ready for review November 7, 2024 20:56
@Manishearth Manishearth merged commit b1ae9e7 into rust-diplomat:main Nov 7, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-js JS backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS2 constructors: Struct ctors should take an object, not a list of fields
2 participants