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

Fixed 2 minor bugs in JS glue code #4192

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

RunDevelopment
Copy link
Contributor

This separates the 2 bug fixes from #4189 into their own PR.

Changes:

  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. As @daxpedda found out, the return value hasn't been used for 6 years now.
    • There is no ptr field. The ptr field was renamed to __wbg_ptr one year ago and this function wasn't updated.

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

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.

Thank you!
LGTM.

But missing a changelog entry for the regex issue.

@daxpedda daxpedda added the waiting for author Waiting for author to respond label Oct 13, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@daxpedda daxpedda merged commit d6406e1 into rustwasm:main Oct 14, 2024
41 checks passed
@RunDevelopment RunDevelopment deleted the 2-minor-bugs-in-glue branch October 14, 2024 11:55
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