[nathanwhit]

jsg::Nullable<T> had three states (Some, Null, Undefined), so a single type represented T | null | undefined rather than matching C++ JSG nullable values. There was no way to express T | null alone.

Nullable<T> now represents T | null, with Some and Null variants. Input conversion follows Web IDL's nullable conversion and C++ kj::Maybe<T>: both null and undefined become Nullable::Null. Output conversion produces only T or null, matching the eventual T | null TypeScript type. A method argument of this type remains required: passing explicit undefined is accepted, but omitting the argument is a TypeError.

An optional nullable is written Option<Nullable<T>>, composing with Option<T> (T | undefined). It permits omission and keeps all three states distinct: undefined is None, null is Some(Nullable::Null), and a value is Some(Nullable::Some(value)). This covers Web IDL's optional T?, while Nullable<T> covers T? without changing runtime acceptance, arity checks, or typing when migrating C++ bindings.

API changes:

• Remove Nullable::Undefined, is_undefined(), is_null_or_undefined(), not_undefined() and coerce_null().
• not_null() returns the contained value instead of Self.
• Lenient converts to and from Option<Nullable<T>> rather than Nullable<T>, and to_parts() returns an Option<Nullable<T>>.

Nothing outside the JSG tests uses Nullable yet.

[clanky-the-clanker]

💬 Approved with comments — The conversion semantics, removal of the old API, tests and docs are correct and match each other. The only note is an optional SUGGESTION to mention the new Nullable mapping in src/rust/AGENTS.md.

Findings: 1 suggestion

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

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #8015.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## SUGGESTION (1)

### [AGENTS.md] Optional: add a short note to the JSG conventions in src/rust/AGENTS.md: `Nullable<T>` is `T | null`, and `Option<Nullable<T>>` is `optional T?`.

---

📖 AGENTS.md: 💬 1 suggestion

Optional: src/rust/AGENTS.md could mention the new Nullable semantics

Recommendation: 💬 Optional Nits

• [SUGGESTION] Optional: add a short note to the JSG conventions in src/rust/AGENTS.md: Nullable<T> is T | null, and Option<Nullable<T>> is optional T?.

View full review

docs/reference/rust-review-checklist.md now documents how Nullable<T> (T | null) differs from Option<Nullable<T>> (T | null | undefined, and the argument can be omitted). src/rust/AGENTS.md mentions Nullable only in its tracing notes. Adding a one-line type-mapping note to its JSG conventions would help people working on the Rust bindings use the right type when they port C++ T? and optional T? bindings.

---

✅ All other checks passed

🔒 Security: ✅ LGTM

No security concerns. The change narrows the Rust JSG type semantics and adds no new trust boundaries.

---

📋 Code Quality: ✅ LGTM

I found no problems in the changed code or in the code it interacts with.

• No leftover uses of the removed API. src/rust no longer uses Nullable::Undefined, is_undefined() on Nullable, coerce_null or not_undefined.
• Input conversion. FromJS for Nullable<T> in wrappable.rs maps both null and undefined to Null, in from_js and in try_from_js. is_optional() stays false, so leaving out a Nullable<T> argument is still a TypeError.
• Option<Nullable<T>>. Option<T> turns undefined into None and passes anything else to Nullable, so the three states stay distinct and the argument can be omitted.
• #[jsg_struct] fields. An Option<Nullable<T>> field is treated as optional. A bare Nullable<T> field reads as Null when it is missing, and the new tests cover this.
• Lenient. The new From<Option<Nullable<T>>> and the new return type of to_parts() agree with each other.
• Tests. The new nullable.rs covers input and output conversion, argument arity, struct fields and Lenient.
• Docs. The type table in wrappable.rs and docs/reference/rust-review-checklist.md describe the new behaviour correctly.

---

⚡ Performance: ✅ LGTM

No performance concerns.

---

📝 Documentation: ✅ LGTM

The updated checklist entry and the doc comments in wrappable.rs match the new Nullable<T> and Option<Nullable<T>> behaviour. The existing GC-tracing tables in src/rust/jsg/README.md mention Nullable only for its Some case, so they are still correct.

---

_{Reviewed commit: f7b0802c}

[ask-bonk]

Since last review: 0 resolved, 0 still open, 0 new.
LGTM!

Not re-run: api-compat, tests, docs, compat-flags, design-simplicity, jsg-gc, memory-safety, rust-first (no author changes in their files since the last review)

---

_{Reviewed commit: f7b0802c · github run}

[guybedford]

Thanks for exploring this space further here.

One issue here is that the migration path of a C++ Maybe, which today accepts both undefined and null, would then migrate to Option<Nullable<T>> under JSG Rust - but this would correspond to the type foo?: T | null instead of the type foo: T | null. Thus, this type could only be representable by Nullable<T> in an upgrade path that wants to spit out the same TypeScript types, but then the undefined runtime support would be lost.

If we want to maintain the migration path for both the runtime and typing, I think the right consolidation for this explicit migration might be to extend this change to also treat Nullable<T> as accepting T | null | undefined in the runtime but treating the undefined path as a null coercion, and expressing as the type T | null in the typing output just like the KJ Maybe.

This would then allow Option<Nullable<T>> to represent truly optional nullables, while maintaing the WebIDL convention compat and arity checks.

[nathanwhit]

Thanks for exploring this space further here.

One issue here is that the migration path of a C++ Maybe, which today accepts both undefined and null, would then migrate to Option<Nullable<T>> under JSG Rust - but this would correspond to the type foo?: T | null instead of the type foo: T | null. Thus, this type could only be representable by Nullable<T> in an upgrade path that wants to spit out the same TypeScript types, but then the undefined runtime support would be lost.

If we want to maintain the migration path for both the runtime and typing, I think the right consolidation for this explicit migration might be to extend this change to also treat Nullable<T> as accepting T | null | undefined in the runtime but treating the undefined path as a null coercion, and expressing as the type T | null in the typing output just like the KJ Maybe.

This would then allow Option<Nullable<T>> to represent truly optional nullables, while maintaing the WebIDL convention compat and arity checks.

Ah this is a good point, thanks for pointing it out! I hadn't considered the migration path and WebIDL angle enough.

Updated PR according to your feedback

[guybedford]

Thanks, this now matches kj::Maybe on both the runtime and the eventual T | null typing, with Option<Nullable<T>> covering optional T?.

Nits:

• The PR description still describes undefined as a TypeError and recommends Option<Nullable<T>> + unwrap_or for Web IDL T?; worth updating since it becomes the squash commit message.
• Branch is a few commits behind main, a rebase before landing would be good.