Skip to content

Bring Rust JSG unions to parity with kj::OneOf - #8026

Open
bartlomieju wants to merge 32 commits into
mainfrom
biwanczuk/rust-jsg-oneof
Open

bartlomieju wants to merge 32 commits into
mainfrom
biwanczuk/rust-jsg-oneof

Conversation

@bartlomieju

@bartlomieju bartlomieju commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

#[jsg_oneof] unions now convert, hold and check the same types as C++ JSG's kj::OneOf:

  • Conversion: variants are tried in the passes of OneOfWrapper::tryUnwrap(), so a value is coerced only when no variant takes it as it is. An error from a variant's conversion ends the conversion.
  • Variant types: strings, numbers, the integer types, booleans, SystemTime, resources, structs, sequences (Vec<T>, including Vec<u8>), Dict<V>, Nullable<T>, NonCoercible<T>, nested unions, and V8 handles, including typed array handles. A union of handles has one lifetime parameter, their scope. Each type converts in the pass of its C++ counterpart, quirks included: SystemTime, like kj::Date, takes a number before any numeric variant does.
  • Web IDL checks: a union that breaks Web IDL's rules for unions doesn't compile, with webidl::UnionTypeValidator's messages. That includes two string types, an object handle with a struct, two nullable types, an Option<T> variant, or a type used twice (counting Nullable<T> as T).
  • Tracing and output: unions trace their variant, so they can be resource fields unless they hold handles, and implement ToJS when every variant does.

NonCoercible<T> now requires the new sealed jsg::Coercible: String, bool, f64 and the integer types, whose is_exact is a precise type check. This matches C++ JSG, where NonCoercible<T> requires a CoercibleType. Otherwise NonCoercible<Union> or NonCoercible<jsg::Rc<R>> would coerce, since their is_exact is only a coarse check. No existing code wraps another type.

Handle parameters also gain the FromJS impls of C++ JSG's V8HandleWrapper: Local<String>, Local<Object>, Local<Function>, Local<BigInt> and Local<TypedArray> take a value of exactly that type.

No production code uses #[jsg_oneof] yet, so there is no JS-visible change.

A JS function that threw made Local<Function>::call panic and terminate
execution, unlike C++ JSG, where the exception reaches the caller. The
call now catches it and returns a jsg::Error holding the thrown value,
which Lock::throw_exception rethrows unchanged; a termination is not
caught and still propagates. Error stays Send, Sync and Clone, as the
value is kept in an Arc<Global>.
Describing a caught value through createTunneledException read its
overloaded and retryable properties, running guest getters and proxy
traps a second time and possibly replacing the pending exception.
jsg::Error now takes its name and message from V8's side-effect-free
ToDetailString instead. As an Error can hold a strong Global, it is no
longer Traced, so a resource cannot keep one in an uncollectable cycle.
Coercing a value to a string or number, reading or testing for a
property, and iterating an array can run JavaScript. When it threw,
the shims converted the exception to a tunneled kj::Exception, which
kept only its type and message, so JavaScript caught a new error
rather than what it threw, unlike C++ JSG.

These shims now catch the exception with catchException(), shared
with local_function_call, into an out-parameter, and Rust converts
the failure with v8::catching(): to Error::from_exception for what
JavaScript threw, to Error::terminated if execution is terminating,
or otherwise to the shim's own error.

A termination that a callback requests now takes effect the next
time JavaScript runs, as in C++ JSG, rather than as a side effect of
reading properties of the caught error, so the re-entrant panic test
runs JavaScript before checking a fresh context.
Since coercions, property access and array iteration keep what
JavaScript throws, the README's errors section no longer applies to
Local<Function>::call alone.
…ercion-exceptions

main has the version of #7954 that landed, which leaves what JavaScript
throws pending (Error::pending(), Lock::try_catch) instead of catching it
into the Error. This branch was built on an earlier version of #7954 that
caught it (Error::from_exception, Error::terminated), so the conflicts are
resolved by taking main's version of every file the branch changed. The
coercion changes are redone on top of main's design in the commits that
follow.
Coercing a value to a string or number, reading or testing for a
property, and iterating an array can run JavaScript (toString, valueOf,
Symbol.toPrimitive, getters, Proxy traps). When it threw, the shims
caught the exception with JSG_TRY and tunneled it through a
kj::Exception, which kept only its type and message: a Rust-backed
method's JavaScript caller caught a new error, not what it threw.

unwrap_string, unwrap_number, local_object_get_property,
local_object_has_property and local_array_iterate now catch nothing, as
local_function_call already doesn't: they return an empty result or
false and leave the exception pending. Rust turns that into
Error::pending(), which Lock::throw_exception doesn't throw, so the
original exception reaches the caller unchanged; Lock::try_catch catches
it when Rust wants to handle it. This removes the shims' remaining uses
of C++ JSG for exceptions (jsg::Lock::from, JSG_TRY/JSG_CATCH,
JsExceptionThrown, createTunneledException), and Result stays only where
C++ can still throw (key interning, building a rust::String, the
not-an-array KJ_REQUIRE); unwrap_number no longer returns one.

Every call into JavaScript, including Local<Function>::call, now goes
through call_into_js(), so that all of them decide alike: nothing is
called while execution is terminating, and an empty result is a pending
error. While terminating, a conversion therefore fails even for a value
that would convert without running JavaScript.

When Array::Iterate() fails partway, local_array_iterate now resets the
handles it has collected. ffi::Global has no destructor in Rust, so they
stayed strong roots until the isolate was disposed.

ffi.h names local_object_set_property, local_array_get and
local_array_set as the shims that still turn what JavaScript throws into
a panic (EW-11138).

Tests that read a coercion error's name and message now catch the thrown
value with try_catch instead. New tests check that what is thrown is the
value the caller catches (including from a Proxy get trap during
ToPrimitive), and that string, number, get, has and iterate neither
catch a termination nor run JavaScript while execution is terminating.
FromJS for jsg::Error reads `name` and `message`, which can run a getter,
a Proxy trap or a toString. It discarded their errors with .ok() and
went on, so it ran more JavaScript with an exception pending, and could
drop a termination. Now that those errors stand for a pending exception,
it passes them on with `?`.
ToJS for Vec<T> and for a #[jsg_struct] set each element or field with
Object::Set(), which looks up the prototype chain, so a setter a script
put on Array.prototype or Object.prototype ran, and could throw. ToJS
can't fail, and C++ JSG doesn't call Set() there either: it builds an
array with v8::Array::New(isolate, elements, length) and a struct from
an ObjectTemplate.

* Array::with_elements builds an array from its elements with
  v8::Array::New, as C++ JSG does, and ToJS for Vec<T> uses it.
* Object::with_data_properties creates an object and defines its
  properties with CreateDataProperty(), which runs no setter on a new
  ordinary object; #[jsg_struct]'s ToJS uses it.

A test defines throwing setters on Array.prototype and Object.prototype
and checks that converting a Vec of structs, directly and through a
method's arguments and return value, calls none of them and defines own
properties in field order. It fails with the previous Set()-based
conversions. ok-empty-struct checks that a struct without fields still
compiles without a warning.
Object::set and Array::set can run JavaScript, a setter on the object
or its prototype chain, or a Proxy trap, but local_object_set_property
and local_array_set used jsg::check() and weren't declared as returning
Result, so what JavaScript threw became a Rust panic, which terminates
execution (EW-11138).

Both shims now catch nothing: they return false and leave the
exception pending, as local_object_get_property does. Object::set and
Array::set go through call_into_js(), so they return an error for which
is_pending() is true, and run nothing while execution is terminating.
Array::set takes a u32 index, V8's, instead of truncating a usize.
local_array_get, which nothing called, is removed. This takes the last
three jsg::check() uses out of the shims that run JavaScript.

The Node.js exception helpers in node-exceptions set `code`, `syscall`,
`path` and `dest` on a new error, which runs a setter a script put on
the error's prototype chain. They now pass on its error, and the C++
caller throws JsExceptionThrown for the exception left pending, as
C++'s JsObject::set does.

New tests check that Object::set and Array::set throw what a setter on
the object, on its prototype chain, or a Proxy trap threw, and that
they neither catch a termination nor run JavaScript while execution is
terminating.
…t-exceptions

# Conflicts:
#	src/rust/jsg-test/tests/coercion_safety.rs
#	src/rust/jsg/README.md
#	src/rust/jsg/ffi.c++
#	src/rust/jsg/ffi.h
#	src/rust/jsg/lib.rs
#	src/rust/jsg/v8.rs
#	src/rust/jsg/wrappable.rs
The FFI comments for object_new_with_data_properties and
local_object_set_property said keys are interned with jsg::check, which
the interning helpers no longer use; they now say what can fail. The
review checklist and the README's first safety rule list Object::set and
Array::set among the calls that run JavaScript.
A jsg::Rc is strong until a GC visit makes it traced, so a resource
field that nothing traces leaks its children but never frees them
early. The collection GC tests checked only that the children of a
reachable parent survive, which they do whether or not the field is
traced: with tracing disabled for Vec, HashMap, BTreeMap, HashSet,
BTreeSet or Cell, none of them failed.

Each child now keeps its parent's wrapper, which makes a cycle that
GC can collect only by tracing the parent's field, so the tests'
checks that the parent and children are collected fail if the field
is not traced.
Rust JSG had no counterpart to C++ JSG's jsg::Dict<V>, Web IDL's
record<DOMString, V>: a plain object used as a map from strings to
values. jsg::Dict<V> holds its entries in order, in a Vec, and
converts like C++ JSG's DictWrapper. From JavaScript, it takes any
object other than an array and reads its own enumerable properties
that are not symbols, as Object.keys() lists them, converting each
value before reading the next. A value that is not a V is a
TypeError naming its entry, with C++ JSG's message, and what a Proxy
trap, getter or conversion throws reaches the caller unchanged. To
JavaScript, it is a new plain object.

Local<Object> gains keys(), which lists properties that way, and
get_by_name(), which reads a property by a string or symbol handle.
A Dict reads each value with the name V8 listed, so even a key that
does not convert losslessly to a Rust string gives its value.
Converting a Dict to JavaScript sets its properties with set(), so
what a setter throws stays pending, as in C++ JSG.
…t-exceptions

# Conflicts:
#	src/rust/jsg-macros/lib.rs
#	src/rust/jsg-test/tests/jsg_struct.rs
#	src/rust/jsg/cpp-jsg-allowlist.txt
#	src/rust/jsg/wrappable.rs
C++ JSG converts a JSG_STRUCT with a DictionaryTemplate when
fast_jsg_struct is on (from 2025-12-03), and with Set() for each
field before that, which runs a setter a script put on
Object.prototype. Rust JSG defined the fields as data properties at
every compatibility date, so a Worker with an older date could tell
the difference through node:dns's resolveCaa() and resolveNaptr().

Now that ToJS can fail, a #[jsg_struct] without fast_jsg_struct sets
its fields with Object::set, as C++ JSG does, and passes on what a
setter throws; with the flag, it still defines them as data
properties. A Vec is still built from its elements, as C++ JSG builds
arrays at every date.

The conversion moves to struct_to_js_body() to keep jsg_struct under
the function length limit. New tests check that without the flag a
setter on Object.prototype takes the field's value, and that what it
throws reaches the caller, from Rust and from a method's return
value; the existing test now turns the flag on.
…t-exceptions

# Conflicts:
#	src/rust/jsg/README.md
With fast_jsg_struct, an absent optional field of a #[jsg_struct] is
now an own property whose value is undefined, as C++ JSG's
DictionaryTemplate gives every field a value; only the Set() path
omits it. Object::with_data_properties returns the error from
interning a key instead of panicking, since a caller can pass a key
that is too long.

The ok- compile tests pass on the error from Object::set with `?`, or
throw it, rather than dropping it, and the review checklist names
.is_ok() among the ways to discard one. Array::set's docs no longer
mention a Proxy trap, which a Local<Array> can't be.

The fast_jsg_struct test records which setters ran in a string, so a
setter on Array.prototype that ran would fail its assertion rather
than recurse through push(). New tests check optional fields in both
flag states, that converting a Vec stops at the first element that
fails, and that what a setter on Error.prototype throws while
node-exceptions sets code, syscall, path or dest reaches the caller.
@codecov-commenter

codecov-commenter commented Oct 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 150 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.32%. Comparing base (47cde3f) to head (e32fb0d).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/rust/jsg/oneof.rs 0.00% 80 Missing ⚠️
src/rust/jsg/wrappable.rs 0.00% 64 Missing ⚠️
src/rust/jsg/resource.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8026      +/-   ##
==========================================
- Coverage   69.38%   69.32%   -0.06%     
==========================================
  Files         696      697       +1     
  Lines      181864   182003     +139     
  Branches    29213    29213              
==========================================
- Hits       126185   126182       -3     
- Misses      38612    38754     +142     
  Partials    17067    17067              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

A Dict built in Rust can hold a key twice, which no object read from
JavaScript can, so no test covered which entry get() returns. Also
list Dict<V> in the README's table of traced field types.
Keeping the entries private leaves room to change how a Dict stores
them, for example to add a faster lookup. The lookup is a linear
search, which find() says better than get(). Add iter(), len() and
is_empty() for code that read the field.
# Conflicts:
#	src/rust/jsg-macros/lib.rs
#	src/rust/jsg-test/tests/coercion_safety.rs
bartlomieju and others added 4 commits October 28, 2026 10:12
# Conflicts:
#	src/rust/jsg/README.md
#	src/rust/jsg/lib.rs
Converting a jsg::Dict to JavaScript set each entry, as C++ JSG's
DictWrapper does, so a setter on Object.prototype ran, and a
__proto__ entry replaced the prototype or was dropped. Define the
entries as own data properties instead, as Web IDL does for a
record: no JavaScript runs, and __proto__ is an own property, as
with Object.fromEntries().
V8 doesn't allow CreateDataProperty while execution is terminating:
a debug check fails. Object::with_data_properties, which converting
a Dict and, with fast_jsg_struct, a #[jsg_struct] use, called it
regardless, which crashed the debug build when a method returned
either after a termination. Check for termination first, through
call_into_js, and return a pending error, as Object::set does.
A #[jsg_oneof] union took only a value of exactly one of its variants'
types, in declaration order, so it never coerced: a `String | Number`
union rejected `true`, which C++ JSG converts to "true". Unions now
follow C++ JSG's OneOfWrapper::tryUnwrap(), the Web IDL union
conversion: variants are tried in passes (resources, other non-
primitive types, exact booleans and numbers, then strings, numbers and
booleans with coercion), in declaration order within a pass, and an
error from a variant's conversion ends the conversion.

Unions also implement Traced, so a union holding a jsg::Rc can be a
resource field, and ToJS when every variant does. jsg::Rc<R> and
NonCoercible<T> implement Type so that they can be variants.

With unions converting this way, NonCoercible<T> of a union or a
resource would have coerced: it checks Type::is_exact, which for those
is only a coarse check of the JavaScript type (any object, for a
resource), and then runs T's conversion, string coercion included. So
NonCoercible<T> now requires the new sealed jsg::Coercible: String,
bool, Number and the integer types, whose is_exact is precise, as C++
JSG's NonCoercible<T> requires a CoercibleType. No existing use wraps
another type.

No production code uses #[jsg_oneof] yet, so no Worker-visible API
changes.
A #[jsg_oneof] union now takes the variant types that C++ JSG's
kj::OneOf can hold and Rust JSG couldn't: the integer types,
SystemTime, Nullable<T>, Dict<V> and V8 handles. Each converts in the
pass of its C++ counterpart. Variants need the new hidden
jsg::UnionVariant, which every Type implements, and which Option<T>
and Lenient<T> implement only so that a union can reject them with an
explanation. A union of handles takes their scope as its one lifetime
parameter. Handle parameters gain the FromJS impls that C++ JSG's
V8HandleWrapper has: Local<String>, Local<Object>, Local<Function>,
Local<BigInt> and Local<TypedArray> take a value of exactly that type.

Unions are now checked at compile time against Web IDL's rules, as
webidl::UnionTypeValidator checks a kj::OneOf. Each type declares its
Web IDL categories, a union sums its variants' in a constant that
fails to evaluate when a rule is broken, counting nested unions and
nullable types, and a type used twice gets conflicting impls of a
local trait, with a Nullable<T> variant counted as T, as C++ JSG
flattens kj::Maybe<T>. Since Vec<u8> is a sequence, as C++ JSG's
kj::Array<T> is for any T but kj::byte, Rust JSG has no buffer source
types: a typed array handle is in no category, like C++ JSG's
v8::Local<v8::Uint8Array>. A test union of two structs, which C++
rejects, is removed. The union support code moves to its own modules.
@bartlomieju
bartlomieju force-pushed the biwanczuk/rust-jsg-oneof branch from 6dd0e76 to 3cb9556 Compare October 28, 2026 11:47
Base automatically changed from biwanczuk/rust-jsg-dict to main October 28, 2026 16:27
Adapts the union work to #8054, which replaced jsg::Number with
f64: f64 is now Coercible and a numeric union member, the tests use
f64, and oneof-variant-type uses f32, which still cannot be a
variant, since f64 now can. Also fixes the Dict tests from #7998,
which were merged still using jsg::Number.
@bartlomieju
bartlomieju marked this pull request as ready for review October 28, 2026 16:43
@bartlomieju
bartlomieju requested review from a team as code owners October 28, 2026 16:43
@clanky-the-clanker

clanky-the-clanker Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

✅ Approved — No findings. The merge into wrappable.rs keeps union conversion consistent with C++ JSG's OneOfWrapper, and nothing found in the earlier review needs another look.

Not opened by the correctness review: 6 changed files it was meant to read
  • src/rust/jsg-macros/lib.rs
  • src/rust/jsg-macros/oneof.rs
  • src/rust/jsg-macros/resource.rs
  • src/rust/jsg/lib.rs
  • src/rust/jsg/oneof.rs
  • src/rust/jsg/resource.rs

The decision is advisory: this comment never approves, blocks or requests changes on the pull request.


✅ All checks passed

🔒 Security: ✅ LGTM

The handle conversions check the value's type with try_from, and they neither panic nor coerce on a mismatch. Limiting NonCoercible<T> to Coercible types narrows what it accepts.


📖 AGENTS.md: ✅ LGTM

No AGENTS.md issues found.


📋 Code Quality: ✅ LGTM

Every convertible type in wrappable.rs sets UNION_MEMBER and UNION_TYPES. The &T impl forwards try_from_js_in_union_pass. The handle FromJS impls return Ok(None) on a type mismatch, as the trait's doc comment requires. Nothing still uses the removed try_from_js_exact.


⚡ Performance: ✅ LGTM

No performance concerns.


📝 Documentation: ✅ LGTM

No documentation issues found.


Reviewed commit: e32fb0d3

@ask-bonk

ask-bonk Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Since last review: 0 resolved, 2 still open, 0 new.

The latest wrappable.rs update leaves both prior union-support and final boolean-pass test gaps unresolved.


Reviewed commit: e32fb0d3 · github run

Comment thread src/rust/jsg/wrappable.rs
// =============================================================================
//
// These types only implement `Type` (used by `NonCoercible<T>` for type checking); their
// These markers implement `Type` so that their handles can be `#[jsg_oneof]` variants; their

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[WARNING] Local<Uint8ClampedArray> already has exact FromJS support in v8.rs, but impl_typed_array_type! omits its marker. Since the new generic Type for Local<M> requires M: Type, this otherwise-supported typed-array handle cannot be used as a union variant, contrary to the typed-array support added here. Add Uint8ClampedArray => is_uint8clamped_array to that marker list and cover it with a compile or integration test.

assert_eq!(result, "string:hello");
Ok(())
#[test]
fn number_or_bool_coerces_to_number() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[WARNING] The boolean assertions only pass an actual boolean, which is handled by the earlier exact-boolean pass. Every non-boolean input in these unions is claimed by a string or numeric variant before the final boolean-coercion pass, so removing or breaking pass 7 in try_union_from_js() leaves this suite green. Add an unambiguous bool-only union case invoked with a non-boolean value, such as 0, to cover the final coercion pass.

# Conflicts:
#	src/rust/jsg-test/tests/dict.rs
#	src/rust/jsg/dict.rs
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.

4 participants