Repository navigation
Conversation
marc0olo
added this pull request to stack #200
September 28, 2026 15:08
A record field of type `opt opt T` is declared `field?: T | null`: undefined is absent, null is present but empty, and a value is set. Decoding returned a Some/None object instead, and encoding sent null as absent, so the empty state could not be sent. The same applies when the inner option is reached through a name.
The record field rule was only documented for a single option. Show the three states of an opt opt field, and that a deeper option continues with Some/None; the conversion test now covers the deeper case.
marc0olo
force-pushed
the
fix/nested-option-record-fields
branch
from
September 28, 2026 15:44
7d095b7 to
ec2e22b
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The implementation is coherent and tested, but intentionally changes wire semantics through a public type-mapping design decision.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes opt opt record-field conversion so runtime values match generated TypeScript types.
Changes:
- Distinguishes
undefined,null, and populated values during encoding and decoding. - Adds nested-option conversion tests and documentation.
| File | Description |
|---|---|
tests/conversion.test.ts |
Tests nested optional field states. |
tests/assets/nested_option_fields.did |
Adds the nested-option fixture. |
src/core/generate/rs/src/bindings/typescript_native/conversion_functions_generator.rs |
Corrects nested optional field conversions. |
docs/src/content/docs/structure.md |
Documents the three-state representation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
marc0olo
marked this pull request as ready for review
September 29, 2026 08:17
marc0olo
removed this pull request from stack #200
September 29, 2026 08:43
marc0olo
added this pull request to stack #202
September 29, 2026 08:43
This was referenced Sep 29, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #197.
A record field of type
opt opt Tis declaredfield?: T | null, but its values were converted as something else. This PR keeps the declared type and fixes the conversions, and documents the rule instructure.md. That is a design choice as much as a fix, so the alternatives are laid out below.Behaviour
record { f : opt opt Cfg }f?: Cfg | nullf?: Cfg | null, unchanged[]{ __kind__: "None" }undefined[[]]{ __kind__: "Some", value: null }null[[cfg]]{ __kind__: "Some", value: cfg }cfg{ f: undefined }or omitted, encoded[][]{ f: null }encoded[]: the state[[]]could not be sent[[]]{ f: cfg }encoded[[cfg]][[cfg]]This changes what goes on the wire for
null, and what a decoded field holds. The same applies when the inner option is reached through a name (type Inner = opt text; record { f : opt Inner }). A deeper option follows the same rule:opt opt opt textis declaredf?: Some<string | null> | None, and decoding[[[]]]gaveSome(Some(null)), one level too many; it now givesSome(null). Fields with a singleopt, andopt optin any other position, are unchanged.Where it matters: Internet Identity's config record uses
opt optfor set/clear fields (analytics_config,dummy_auth,dnssec_config,doh_config,mcp_official_url): absent keeps the stored value,opt nullclears it, a value sets it. Through the wrapper, a caller could not clear such a field, and a caller reading one got an object where the type promisesT | null.How this was found
Not from a user report: probing the generator with an
opt optrecord field, and typechecking the wrappers of real interfaces with@ts-nocheckremoved (Internet Identity fails with TS2322 there). No caller going through a bindgen wrapper for these fields turned up. The shape is common in real interfaces, though: besides Internet Identity's config, the asset canister'sSetAssetPropertiesArgumentshas fouropt optfields (max_age,headers,allow_raw_access,is_aliased) with the same keep/clear/set meaning. A JS client clearingmax_agethrough the wrapper would send "keep" instead, without an error.Where the rule comes from
The representation was designed in dfinity/candid#601 and #672 (neither merged) and ported here in 62fcb7b:
opt TisT | null.Some<…> | None, sinceT | null | nullwould merge two states.opt Xis optional,f?: <X>, withundefinedas the absent state.structure.mddocuments all three, but its record example has a singleopt, and neither candid's tests (optional.didnests only standalone options) nor this repo's covered anopt optfield. What such a field should be was never written down, and three sources disagree:[][[]][[v]]X = opt Tundefinednullvmetadata?: Option<Record>)undefinedNoneSome(v)mainNoneSome(null)Some(v)The runtime matches neither design: under both, an absent field is
undefined. It is the standalone nested conversion (Some<T | null> | None) applied to the whole field, whose resultrecord_opt_to_undefinedpasses through because it only mapsnull.The principle behind the mapping
Every option mapping in bindgen follows one rule: use a native JavaScript empty value where one is available, and the tagged
Some/Noneonly where JavaScript runs out of them.opt TT | nullopt Tf?: T(omitted)opt opt Tf?: T | null(omitted,null)opt opt Ton its own, or a third levelSome<…> | NoneVariants follow the same rule: a payload has no native form, so it is tagged with
__kind__. The declared type of anopt optfield already follows it; the conversions did not.Options
A. Keep the declared type, fix the conversions (this PR).
f?: <X>withX = opt T.nullfor the inner one.nullnow means "present but empty", unlike a plainopt Tfield wherenullis not a declared value. Anyone who worked around theSome/Noneobjects with casts gets different values.B. Declare
f?: Some<T> | None(candid#672's example).nullis not overloaded.opt optfield, a breaking change. It is not the field rule applied, so it needs a special case in both the declarations and the conversions. The runtime still needs fixing, because an absent field decodes toNonetoday, notundefined.C. Declare what the runtime does,
f?: Some<T | null> | None.None), so it isn't a clean mapping of three states. It still changes the declared type, and it is the standalone rule applied where the field rule is documented.D. Reject
opt optrecord fields. Not viable: Internet Identity uses them.A is the smallest change for callers and the only one that needs no special case in the rule, which is why this PR takes it. B is the alternative if reviewers prefer an explicit inner level over reusing
null.Why the conversions were wrong
The decoder converted the field with the standalone nested-option conversion, then with
record_opt_to_undefined. The encoder tested presence as for a singleopt, sonullread as absent. For a field whose type is itself an option, the outer level is nowlength === 0 ? undefined : …when decoding and!== undefinedwhen encoding, and the inner level goes through the standalone conversion.Testing
tests/conversion.test.tssends and receives all three states for a field that needs no conversion (opt opt text), one that does (opt opt Cfg), one reached through a name, and a deeperopt opt opt text. All four fail on #197 alone. The generated wrapper for the fixture typechecks understrictagainst@icp-sdk/core6.1.0 with@ts-nocheckremoved. No existing snapshot moves.🤖 Generated with Claude Code