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

Use private constructors in type definitions of structs without an exported constructor #4282

Merged
merged 5 commits into from
Nov 28, 2024

Conversation

RunDevelopment
Copy link
Contributor

Motivation

A simple empty struct was previously typed as follows:

#[wasm_bindgen]
struct Foo;
export class Foo {
  free(): void;
}

This typing is incorrect. In JavaScript, a missing constructor is equivalent to an empty constructor.

Example:

// omitting the constructor...
class Example { }

// ... is the same as this
class Example {
  constructor() {}
}

So the above type for Foo is actually just a short form for this:

export class Foo {
  constructor();
  free(): void;
}

Obviously, this is not correct. The Foo class is not supported to be instantiate-able using the class constructor. In debug mode, WBG even generates a constructor that always throws.

Changes in this PR

This PR solves this problem by adding a private constructor for structs with a #[wasm_bindgen(constructor)]. E.g. the above Foo struct would get this type definitions:

export class Foo {
  private constructor();
  free(): void;
}

A private constructor in TS declares the constructor as inaccessible to anyone besides the class itself. In particular, a private constructor prevents users in TypeScript from:

  1. Instantiating the class directly. E.g. new Foo().
  2. Making derived classes. E.g. class Bar extends Foo {}.
  3. Using Foo in generic functions that require a constructor.

See this TS playground for proof.

With those changes, the type definitions now accurately represent the correct usage of class Foo and correctly cause errors on incorrect usage.

Is this a breaking change?

Probably not.

Since this is a type-only change, nothing will break at runtime (more than it already is). Users using the implicitly defined constructor most likely isn't intended by WBG since WBG even generates a constructor that always throws in debug mode to prevent exactly this.

So this PR will at most cause new TS compiler errors for incorrect code. From that perspective, I would argue that this PR is not a breaking change.

@RunDevelopment
Copy link
Contributor Author

So that's cool. Turns out, 2 of our TS tests used the unintended default constructors.

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.

Nice!

Fixing something that caused bugs when using it is not a breaking change in my book.

I was thinking that it might be reasonable to implement default constructors for unit structs, which would also mimic how it works in Rust. Something to think about for a follow-up.

Missing a changelog entry.
And please rebase as well, quite a lot has changed recently.

@daxpedda daxpedda added the waiting for author Waiting for author to respond label Nov 28, 2024
@daxpedda daxpedda merged commit 2fbee99 into rustwasm:main Nov 28, 2024
46 checks passed
@RunDevelopment RunDevelopment deleted the private-constructor branch November 29, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants