Repository navigation
Bring Rust JSG unions to parity with kj::OneOf - #8026
bartlomieju wants to merge 32 commits into
Conversation
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
…s' into biwanczuk/rust-jsg-dict
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.
…s' into biwanczuk/rust-jsg-dict
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
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
# 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.
6dd0e76 to
3cb9556
Compare
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.
|
✅ 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
The decision is advisory: this comment never approves, blocks or requests changes on the pull request. ✅ All checks passed🔒 Security: ✅ LGTMThe handle conversions check the value's type with 📖 AGENTS.md: ✅ LGTMNo AGENTS.md issues found. 📋 Code Quality: ✅ LGTMEvery convertible type in ⚡ Performance: ✅ LGTMNo performance concerns. 📝 Documentation: ✅ LGTMNo documentation issues found. Reviewed commit: |
|
Since last review: 0 resolved, 2 still open, 0 new. The latest Reviewed commit: e32fb0d3 · github run |
| // ============================================================================= | ||
| // | ||
| // 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 |
There was a problem hiding this comment.
[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() { |
There was a problem hiding this comment.
[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
#[jsg_oneof]unions now convert, hold and check the same types as C++ JSG'skj::OneOf: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.SystemTime, resources, structs, sequences (Vec<T>, includingVec<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, likekj::Date, takes a number before any numeric variant does.webidl::UnionTypeValidator's messages. That includes two string types, an object handle with a struct, two nullable types, anOption<T>variant, or a type used twice (countingNullable<T>asT).ToJSwhen every variant does.NonCoercible<T>now requires the new sealedjsg::Coercible:String,bool,f64and the integer types, whoseis_exactis a precise type check. This matches C++ JSG, whereNonCoercible<T>requires aCoercibleType. OtherwiseNonCoercible<Union>orNonCoercible<jsg::Rc<R>>would coerce, since theiris_exactis only a coarse check. No existing code wraps another type.Handle parameters also gain the
FromJSimpls of C++ JSG'sV8HandleWrapper:Local<String>,Local<Object>,Local<Function>,Local<BigInt>andLocal<TypedArray>take a value of exactly that type.No production code uses
#[jsg_oneof]yet, so there is no JS-visible change.