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

Improved string enum bindings #4137

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

RunDevelopment
Copy link
Contributor

Changes summary:

  1. Fixed a small oversight that caused the JS/TS type of Option<StringEnum> to be just StringEnum and not StringEnum | undefined.
  2. Small improvement for Option<StringEnum> WASM -> JS conversion.
  3. Fixed some edge cases when converting JS strings to WASM string enum indexes using a different conversion approach.
  4. Refactored the JS string enum <-> WASM code by extracting duplicates into functions.

1. Fixed outgoing Option<StringEnum> TS type

I noticed that the generated TS code of

fn foo() -> Option<StringEnum> { todo!() }

was

/**
* @returns {StringEnum}
*/
function foo(): StringEnum;

The TS return type should have been StringEnum | undefined.

The fix was very simple. Someone just forgot to call .option() when converting Descriptor to AdapterType.

Note that this was a pure TS type bug. The actual JS glue code was correct. Just the type annotations were wrong.

2. Improved WASM -> JS conversion for outgoing Option<StringEnum>.

The improvement is that the same conversion code is used for outgoing StringEnum and Option<StringEnum>.

The current WASM -> JS conversion for StringEnum uses the fact that the value of a variant of string enum is its index. This makes the conversion from WASM to JS as simple as an array lookup: e.g. ["a","b","c"][someIndex]. Note that this approach implicitly maps the __Invalid variant to undefined (since OOB array accesses in JS yield undefined). Since the user (likely) isn't supposed to return the invalid variant anyway, this doesn't matter. However, we can use this behavior to implicitly handle Option<StringEnum>. Since the hole used to represent None will an OOB index, the same code ["a","b","c"][someIndex] will correctly return undefined for None. So we can use the same conversion code for outgoing StringEnum and Option<StringEnum>.

> toJS = (someIndex) => ["a","b","c",][someIndex]
[Function: toJS]
> toJS(0)
'a'
> toJS(1)
'b'
> toJS(2)
'c'
> toJS(3) // __Invalid
undefined
> toJS(4) // hole/None
undefined

3. Fixed some edge cases for the JS -> WASM conversion of incoming StringEnum

The old JS -> WASM conversion code for StringEnum did not map certain strings to the __Invalid variant correctly.

This is the old code: {"a":0,"b":1,"c":2}[someEnumVal] ?? 3. It used an object like a map. And that's the problem. JS objects have 2 types of fields: fields directly on the objects and fields inherited through their prototype. {"a":0,"b":1,"c":2} defines the fields a,b,c on the object, but the object has more fields from the prototype, e.g. toString and __proto__:

> toWasmOld = (someEnumVal) => ({"a":0,"b":1,"c":2}[someEnumVal] ?? 3)
[Function: toWasmOld]
> toWasmOld("a")
0
> toWasmOld("b")
1
> toWasmOld("c")
2
> toWasmOld("d") // invalid value handled correctly
3
> toWasmOld("toString") // oh no
[Function: toString]
> toWasmOld("__proto__") // oh no again
[Object: null prototype] {}

To fix these edge cases, I used Array#indexOf like this: (["a","b","c"].indexOf(someEnumVal) + 1 || invalid+1) - 1. This is a bit more tricky, so here is how it works:

  • ["a","b","c"].indexOf(someEnumVal) will return the index of valid StringEnum variants and -1 for everything invalid.
  • I then use +1 to make it index+1 for all valid variants and 0 for invalid.
  • || invalid + 1 will then map 0 (which is falsey) to invalid + 1.
  • Lastly, -1 will bring down valid variants from index+1 to index and invalid values from invalid+1 to invalid.
> toWasm = (someEnumVal) => (["a","b","c"].indexOf(someEnumVal) + 1 || 3+1) - 1
[Function: toWasm]
> toWasm("a")
0
> toWasm("b")
1
> toWasm("c")
2
> toWasm("d") // invalid value handled correctly
3
> toWasm("toString") // invalid value handled correctly
3
> toWasm("__proto__") // invalid value handled correctly
3

Performance: Let me start by saying that I haven't benchmarked anything. My goal was correctness. Obviously, indexOf is O(n), but I believe that this doesn't matter, because creating the array (new approach) or object (old approach) is also O(n). Asymptotic runtime complexity isn't very useful here anyway, since most string enum will have <16 variants (like most enums). If improved performance is wanted, I would suggest to start with outlining the object/array to avoid creating a temporary object for each conversion.

(In general, I think it would be a good idea to outline the array.)


Let me know if further changes to the PR are necessary/wanted.

@RunDevelopment
Copy link
Contributor Author

Same as in #4135, wasm-bindgen/wasm-bindgen/crates/cli/tests/reference/raw.rs fails in CI.

However, this PR doesn't affect anything in raw.rs, so I can only assume that CI is broken in some way. I merged upstream/main just in case to get ffdf0de, but my branches are already up-to-date. So I have no idea why CI fails.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Yeah, the use of indexOf is a bit messed up, but it looks correct to me.
Thank you for the contribution!

RunDevelopment and others added 3 commits October 7, 2024 11:33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@daxpedda daxpedda merged commit 6505bf4 into rustwasm:main Oct 7, 2024
37 of 38 checks passed
@RunDevelopment RunDevelopment deleted the string-enum-impr branch October 7, 2024 11:41
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.

None yet

2 participants