Skip to content

perf(ts): return a Result of an Option of a number in a slot - #496

Merged
kixelated merged 7 commits into
mainfrom
quest/m3/ts-option-slot
Nov 3, 2026
Merged

kixelated merged 7 commits into
mainfrom
quest/m3/ts-option-slot

Conversation

@kixelated

@kixelated kixelated commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

A crate function returning Result<Option<u64>, E>, such as Decoder::varint_opt, made an rt.Ok per call. #385's result slot refused any Ok payload containing an enum, and the scalar layer held an Option of a number as a flag plus the number, even though its JS value, T | undefined, is never an object.

Approach

  • Shape::Leaf now covers an Option of a number (an integer of any width, a float, bool, char, or a shared reference to one). That is one JS value, so a held local keeps it as one part, and number counts it as one.
  • As a result, a Result whose Ok is numbers alone, with such Options among them, qualifies for the slot. Examples: Result<Option<u64>, E>, Result<(Option<u8>, u64), E>, Result<(u64, Option<i128>), E>, and Result<(Option<u16>, &[u8]), E>.
  • When the payload is an Option, the callee writes it to out$Option_<T>.p and returns the error or undefined, as out$bool already does. When the first part is a plain number, the existing return path is unchanged.
  • Other snapshots: varintOpt and its caller in codec, made$Option_u8 and unset in cells and shapes, and an ok_or in splits whose Option<u8> error is now one part, so option_okOr and its rt.Result drop out.
  • Bench: expected/bench.ts and everything it calls are unchanged (no bench workload calls varint_opt). No just ts bench workload runs different code, so none can be slower.
  • Differential cases (returns::options) cover Option of u8 through u128, usize, i8 through i128 with their MIN/MAX, f32 (including -0) and f64, both including NaN, plus bool and char. Each has None, Some(0), an error, and arbitrary bytes. They also cover: a result returned as is and through ?, an Option first or last in a tuple and beside a slice, a drop that rewrites the slot, two results held at once, results kept in a Vec and through a fn pointer, and a loop.

Impact

  • The translator's output: a function only translated code calls, returning a Result of numbers in which an Option of a number counts as one, returns its error or undefined and leaves the Ok in a lib.ts slot such as out$Option_u64. ts/MIGRATE.md's existing entry is updated. Roots are unchanged.

Alternatives

  • Return the Option itself, with null meaning an error (the error goes in the slot), to skip the slot write on success, as a plain number's first part does. Dropped: it adds a second sentinel to every slot path, and no bench workload calls varint_opt to measure it against (moq-net calls it 8 times, against 249 for varint).
  • Keep Option<u64> as a flag plus its number inside the slot. Dropped: that writes two fields, and take_apart and build_plain would need enum support.

Follow-ups

None. An Option of a tuple or slice in the slot (its JS value is an object) has no consumer in moq-net.

Review

🤖 Generated with Claude Code

kixelated and others added 5 commits November 2, 2026 22:36
Quest-Claim: bdef938c748b0c1f
An `Option` of a number is one JS value, that or `undefined`, so a local holds it as
one part rather than a flag and the number. A `Result` whose `Ok` is numbers alone,
an `Option` of one among them, such as `Decoder::varint_opt`'s
`Result<Option<u64>, E>`, now hands back through the slot as `Result<u64, E>` does,
with no `rt.Ok` per call.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title An Option of numbers in the result slot perf(ts): return a Result of an Option of a number in a slot Nov 3, 2026
@kixelated
kixelated marked this pull request as ready for review November 3, 2026 07:00
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 42 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0f486037-32b4-446a-94b1-2155fbd54592
Commits

Reviewing files that changed from the base of the PR and between 8c7a301 and adab4b0.

Files selected for processing: 23
  • quest/m3/README.md
  • quest/m3/ts-option-slot.md
  • ts/MIGRATE.md
  • ts/README.md
  • ts/fixtures/codec/expected/coding/decode.Decoder.ts
  • ts/fixtures/codec/expected/harness.ts
  • ts/fixtures/codec/expected/lib.ts
  • ts/fixtures/lang/cells/expected/cells.ts
  • ts/fixtures/lang/cells/expected/lib.ts
  • ts/fixtures/lang/returns/expected/core/bool.ts
  • ts/fixtures/lang/returns/expected/core/option.ts
  • ts/fixtures/lang/returns/expected/core/result.ts
  • ts/fixtures/lang/returns/expected/lib.ts
  • ts/fixtures/lang/returns/expected/returns.Error.ts
  • ts/fixtures/lang/returns/expected/returns.Opted.ts
  • ts/fixtures/lang/returns/expected/returns.ts
  • ts/fixtures/lang/returns/src/lib.rs
  • ts/fixtures/lang/returns/src/returns.rs
  • ts/fixtures/lang/shapes/expected/lib.ts
  • ts/fixtures/lang/shapes/expected/shapes.ts
  • ts/fixtures/lang/splits/expected/core/option.ts
  • ts/fixtures/lang/splits/expected/splits.ts
  • ts/src/lower/mir/scalar.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@kixelated
kixelated added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit 3660c29 Nov 3, 2026
5 checks passed
@kixelated
kixelated deleted the quest/m3/ts-option-slot branch November 3, 2026 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant