Skip to content

Add ffi support for JS null and switch to wasm-gc struct for error signaling#5719

Merged
hoodmane merged 35 commits into
pyodide:mainfrom
hoodmane:js-null
Jul 1, 2025
Merged

Add ffi support for JS null and switch to wasm-gc struct for error signaling#5719
hoodmane merged 35 commits into
pyodide:mainfrom
hoodmane:js-null

Conversation

@hoodmane

@hoodmane hoodmane commented Jun 27, 2025

Copy link
Copy Markdown
Member

We need JS null to represent actual null and not an error. Instead, we use wasm-gc except on safari iOS
where wasm-gc is broken or on old runtimes where wasm-gc support is missing. If we can't use wasm-gc,
we use a JS object and a JS trampoline. There is maybe a 3-4% performance hit associated with
this fallback compared to before this PR. Using wasm-gc to signal errors leads to no measured performance
change.

Resolves #3968.

  • Add a CHANGELOG entry
  • Add / update tests
  • Add new / update outdated documentation

Left to a followup to keep diff down:

  • Rename APIs that are no longer null related.

Comment thread Makefile Outdated
Instead, we use wasm-gc except on safari iOS. On iOS, use a JS
object and a JS trampoline.

@ryanking13 ryanking13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am curious what's the benefit of using actual null instead of JS null (not opposed to this PR, just curious).

Comment thread Makefile.envs
Comment thread Makefile Outdated
Comment thread src/js/emscripten-settings.ts Outdated
Comment thread src/core/sentinel.wat Outdated
Comment thread src/core/sentinel.wat
Comment thread src/js/emscripten-settings.ts Outdated
@hoodmane

Copy link
Copy Markdown
Member Author

The point is to allow JS APIs that distinguish between null and undefined. For instance @ambv tells me that three.js treats them differently. Some Cloudflare APIs also treat null and undefined differently. At least three different people have complained to me about this in the last six months actualy. Related bug report:
#3968

@hoodmane

Copy link
Copy Markdown
Member Author

@ryanking13 I changed a bunch of stuff since your first review so if you could rereview and let me know what you think now. There still seem to be some crashes to iron out but I think they are minor find-and-replace style changes.

@ryanking13 ryanking13 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks. There is a build failure, but codewise looks fine to me.

Comment thread Makefile.envs
Comment thread src/tests/test_typeconversions.py
Comment thread src/core/sentinel.ts Outdated
Comment thread src/core/sentinel.ts Outdated
Comment thread src/core/sentinel.ts Outdated
@hoodmane

Copy link
Copy Markdown
Member Author

422 tests failed out of 1439

So not everything is broken =)

@hoodmane hoodmane changed the title Stop using JavaScript null as an error value Add ffi support for JS null and switch to wasm-gc struct for error signaling Jul 1, 2025
@hoodmane hoodmane merged commit 074948f into pyodide:main Jul 1, 2025
30 of 36 checks passed
@hoodmane hoodmane deleted the js-null branch July 1, 2025 08:37
hoodmane added a commit to hoodmane/pyodide that referenced this pull request Jul 1, 2025
I separated this from pyodide#5719 to keep the diff from getting too large
hoodmane added a commit to hoodmane/pyodide that referenced this pull request Jul 1, 2025
I separated this from pyodide#5719 to keep the diff from getting too large
hoodmane added a commit to hoodmane/pyodide that referenced this pull request Jul 1, 2025
I separated this from pyodide#5719 to keep the diff from getting too large
hoodmane added a commit that referenced this pull request Jul 1, 2025
I separated this from #5719 to keep the diff from getting too large.
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.

Provide a way to convert None to null in to_js

2 participants