Add null to argument types of optional parameters

[RunDevelopment]

All functions with Option<T> arguments take both undefined and null and generally treat them as equivalent (see isLikeNone). Despite both undefined and null being accepted, the generated type definitions only allowed undefined. Example:

#[wasm_bindgen]
pub fn echo(a: Option<String>) -> Option<String> {
    a
}

export function echo(a?: string): string | undefined

This is unnecessarily restrictive, as echo(null) would also work (see isLikeNone) and return undefined just like echo(undefined). This also results in worse ergonomics as many DOM APIs return value | null (e.g. textContent), which currently requires dev to unnecessarily map null to undefined (e.g. echo(element.textContent ?? undefined)).

To bring the generated type definitions in line with the actual runtime behavior of the JS glue code, I changed the type of optional parameters from T | undefined to T | undefined | null. E.g. the function signature of echo is the following with this PR:

export function echo(a?: string | null): string | undefined

This accurately represent the runtime behavior of the function and results in better ergonomics.

Note: This PR only affects function arguments. The return type is still T | undefined, as it should be.

Tests

To test that this (and other features I have planned) are implemented correctly, I added a single large test file: echo.rs. This test file just contains echo/identity functions for various types to check the glue code of various input/output types.

[daxpedda]

LGTM!

Missing a changelog entry.

[daxpedda]

Small nit left, otherwise this LGTM.

[RunDevelopment]

I just realized that return and argument types not being the same is a problem for generated getter-setter pairs.

[RunDevelopment]

Fixed it in #4202. After #4202 is in, I need to update this PR again, so I'll mark it as a draft for the time being.

[RunDevelopment]

Alright, getters and setters are now correct, so this PR is pretty much finished.

CI doesn't pass, because CI is currently broken. See #4210, #4213, and #4216.

[RunDevelopment]

@daxpedda You know when we talked about unreachable!()? We both made the mistake of thinking that Options were always handled in a special branch. We should have read the code more carefully :) See that if omittable? Yeah, it's super reachable, but our tests didn't cover it. I'll add a test for it.

[RunDevelopment]

Okay, there is one more thing I need to put out there. I noticed that one of our TS tests failed. This test defined Rust struct like this:

#[wasm_bindgen]
pub struct Fields {
    pub hallo: Option<bool>,
    pub spaceboy: bool,
}

And checked whether this type checks:

import * as wbg from '../pkg/typescript_tests';

const fields: wbg.Fields = { spaceboy: true, free: () => { } };

Note that the assigned object has no hallo field.

This previously worked, because the generated type of the Fields class was this:

class Fields {
    hallo?: boolean;
    spaceboy: boolean;
    free(): void;
}

Note that hallo used to be an optional field, meaning that it could be omitted. This is why the above TS code compiled.

This is no longer the case. Since fields of type Option<T> now have properly typed getters and setters, the type definition of the fields class now looks like this:

class Fields {
    get hallo(): boolean | undefined;
    set hallo(value: boolean | null | undefined);
    spaceboy: boolean;
    free(): void;
}

The important bit here is that getters and setters can never be omitted. This means that the above TS code no longer compiles, since the hallo field is missing. So I adjusted the tests to make the CI pass.

I am brining this up here, because this might break (type-checked) user code, just like it broke the test. However, I am not too concerned about this. Fields is a class, but the old test used it as an interface. So I can only hope that current user code doesn't abuse TS's structural type system to misuse classes as interfaces.

I just wanted to mention all this in case someone opens an issue about this.

[daxpedda]

This is no longer the case. Since fields of type Option<T> now have properly typed getters and setters, the type definition of the fields class now looks like this:

class Fields {
    get hallo(): boolean | undefined;
    set hallo(value: boolean | null | undefined);
    spaceboy: boolean;
    free(): void;
}

I'm a bit confused, why is this happening? I can't seem to find the code changes that cause these to be getters/setters instead of fields. If this is caused by another PR then which and how?

[RunDevelopment]

#4202 implemented this. You can see similar behavior in the tests of that PR in the Foo#weird property. Previously, WBG always emitted fields to type properties, but that was plain wrong.

In JS, getters and setter can return/accept data of different types, so the general way to write a getter-setter pair is

    get prop(): GetType;
    set prop(value: SetType);

But since GetType and SetType are often the same, it's simpler to write it such properties with the field syntax

    prop: GetAndSetType; // assumes GetAndSetType == GetType == SetType

So when it comes to typing properties, the field syntax is just a common special case.

Since the getter type and setter type of Option<T> properties is no longer the same, they can no longer use the field syntax and fall back to the general form.

[daxpedda]

Since the getter type and setter type of Option<T> properties is no longer the same, they can no longer use the field syntax and fall back to the general form.

Ah, completely overlooked that, thank you!

---

I am brining this up here, because this might break (type-checked) user code, just like it broke the test. However, I am not too concerned about this. Fields is a class, but the old test used it as an interface. So I can only hope that current user code doesn't abuse TS's structural type system to misuse classes as interfaces.

I'm concerned that this is exactly whats happening. I wish I could get more sign-offs on this, but unfortunately I don't expect any.

I'm gonna ask around and see if I get any responses before merging it.
Sorry for the continuous delays!

[RunDevelopment]

I'm concerned that this is exactly whats happening.

Yeah, the fact that we had a test for this is a bad omen. So a third opinion would be nice.

However, the good news is that this at most causes type errors in code that misuses classes as interfaces. And we recently talked about type errors not being breaking changes on the side, so maybe it's not an issue at all.

[daxpedda]

In theory this is definitely not an issue, especially because it would be an API misuse.

But my concern is in practice, I don't want to break maybe a whole ecosystem out there without even a warning.
E.g. what happened with wasm-bindgen-rayon (#3330 (comment)).

[RunDevelopment]

But my concern is in practice, I don't want to break maybe a whole ecosystem out there without even a warning.

In general, we should probably add docs clarifying what guarantees WBG makes.

Also, maybe something like an RC/nightly/canary process could help? If nothing else, maybe a week or two before a new release, make an RC and a public call for testing, so people can report bugs before the actual release is made. No idea to what level cargo/crates.io supports this though (I'm still somewhat new to Rust). I'm just speaking from my experience in the npm ecosystem.

[daxpedda]

But my concern is in practice, I don't want to break maybe a whole ecosystem out there without even a warning.

In general, we should probably add docs clarifying what guarantees WBG makes.

These kind of docs are in great need for improvement indeed, any help here would be appreciated!

Also, maybe something like an RC/nightly/canary process could help? If nothing else, maybe a week or two before a new release, make an RC and a public call for testing, so people can report bugs before the actual release is made. No idea to what level cargo/crates.io supports this though (I'm still somewhat new to Rust). I'm just speaking from my experience in the npm ecosystem.

Would be nice for releases like this. Unfortunately the maintainer bandwidth isn't really there for that.
But yes, crates.io support is there for RC releases.

[RunDevelopment]

These kind of docs are in great need for improvement indeed, any help here would be appreciated!

Then I'll try to write down TypeScript guarantees somewhere the next few days.

Would be nice for releases like this. Unfortunately the maintainer bandwidth isn't really there for that.

Are there no automated tools for that in Rust?

E.g. there's changesets which automates changelog creating, publishing, versioning etc. for JS/TS monorepos. Admittedly, I only worked on projects that use changesets but haven't used to release anything myself yet. I just heard from people that do use it that it makes releasing a new version as simple as running a single command.

[daxpedda]

Would be nice for releases like this. Unfortunately the maintainer bandwidth isn't really there for that.

Are there no automated tools for that in Rust?

E.g. there's changesets which automates changelog creating, publishing, versioning etc. for JS/TS monorepos. Admittedly, I only worked on projects that use changesets but haven't used to release anything myself yet. I just heard from people that do use it that it makes releasing a new version as simple as running a single command.

While making a new wasm-bindgen release is not a single command, its an effort of a couple minutes, its not the issue. The concern is more general, I'm more concerned with finding the time to review PRs and fixing issues then making beta releases and collecting feedback. But maybe I should get my priorities straight 😅!

[RunDevelopment]

The concern is more general, I'm more concerned with finding the time to review PRs and fixing issues then making beta releases and collecting feedback.

Wouldn't a beta release process ease that burden? I mean, if something breaks, you'll get reports anyway, but there is less pressure to fix it in a beta release (and fewer duplicates), since it doesn't affect the whole ecosystem. As I see it, the main thing you buy yourself with a beta release is time and ease of mind. If the beta is broken and you're on vacation, most users won't even notice since they're on the latest stable release. Just fix it a month later (or wait until someone does a PR), make another beta, and repeat until it passes the scream test. This might slow down releases, but people that need the latest features can use beta and bear the cutting edge. But maybe my view on this all is a bit naive since I've been in the position of maintaining a project of the size and reach of WBG.

[daxpedda]

No you are absolutely right, which is why I said earlier I should get my priorities straight 😅!

But just as a reality check: I don't believe a lot of users will get reached by a beta. Unfortunately is is incompatible with any other dependency. So in practice, users will have to use [patch.crates-io] to try it out, I suspect few will. In any case, it should still be worth a shot!

I sent you a private Email about further discussing stuff like that, did you get it?

[RunDevelopment]

I sent you a private Email about further discussing stuff like that, did you get it?

Straight to spam. Thanks gmail 😆