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

Added type annotations for a few glue code functions #4189

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

RunDevelopment
Copy link
Contributor

@RunDevelopment RunDevelopment commented Oct 12, 2024

I added type annotations to a few commonly-used JS glue code functions. This makes it a lot easier to understand the glue, because I have TS to help me out.

Those type annotations also make it easier to verify the correctness of glue. And just to prove that: TS found 2 minor bugs in the JS glue code of our tests:

Fixes moved to #4192.

  1. debugString (this is the JS glue code version of format!("{:?}", value)) had a bug where it checked for a regex mismatch incorrectly. This would lead to dereferencing null at runtime, which throws a TypeError.

  2. _assertClass returned instance.ptr. Here's the full code of the function:

    function _assertClass(instance, klass) {
        if (!(instance instanceof klass)) {
            throw new Error(`expected instance of ${klass.name}`);
        }
        return instance.ptr;
    }

    Returning instance.ptr is weird for 2 reasons:

    • It's not used anywhere in the code. _assertClass is only used in one place and its return value is ignored.
    • There is no ptr field. The author probably meant __wbg_ptr to get the internal pointer of a JS object.

    So I just removed the return statement and instance.ptr.


Aside from that, I did one additional change: I inline lTextDecoder:

// before

const lTextDecoder = typeof TextDecoder === 'undefined' ? (0, module.require)('util').TextDecoder : TextDecoder;

let cachedTextDecoder = new lTextDecoder('utf-8', { ignoreBOM: true, fatal: true });

// after

/** @type {TextDecoder} */
let cachedTextDecoder = new (typeof TextDecoder === 'undefined' ? (0, module.require)('util').TextDecoder : TextDecoder)('utf-8', { ignoreBOM: true, fatal: true });

This just made typing easier, but the expression something to get used to...

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.

I am in favor of this change, but I don't feel comfortable approving this without any further input, ideally approval from another maintainer.
Cc @Liamolucko.

In the meantime, I think it would be good to split off the bugfixes you made into a separate PR.
I would also appreciate if you could do some code archeology into instance.ptr and see where this originated and the context around it.

Didn't review it yet, but it needs a changelog entry.

@daxpedda daxpedda added the needs review Needs a review by a maintainer. label Oct 12, 2024
@daxpedda
Copy link
Collaborator

This is amazing work by the way, thank you!

@RunDevelopment
Copy link
Contributor Author

I think it would be good to split off the bugfixes you made into a separate PR.

Will do.

I would also appreciate if you could do some code archeology into instance.ptr and see where this originated and the context around it.

Seems like the pointer field changed names a few times. 240d3cd (7 years ago) changed it from __wasmPtr to ptr and dbea2a2 (1 year ago) changed it from ptr to __wbg_ptr. So it's not that the author of _assertClass used the wrong field name, the code of _assertClass just wasn't updated when the pointer field was changed to the current __wbg_ptr.

@daxpedda
Copy link
Collaborator

I also found 5d697c1, which was the point where the return value stopped being used anymore but didn't get removed.

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.

@RunDevelopment I'm happy to go ahead and merge this after a rebase.

Would that solve the requirements for #4311 to run type checks on JS as well?

@daxpedda daxpedda added waiting for author Waiting for author to respond and removed needs review Needs a review by a maintainer. labels Dec 8, 2024
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