fix: catch up with Candid spec changes (`reserved` ↛ `null`) #5619

ggreif · 2025-10-29T12:21:14Z

Reproduce issue #5543. Fixes #5543.

The spec says that every Candid value is a subtype of opt T, and gives opt t when t <: T. But the null type has no subtypes. I.e. we only get out a null : Null when the binary Candid has a 0x7F there. In all other cases the coercion must fail, either trapping or indicating a coercion failure to the caller (when recoverable). However null : ?T can always be materialised when an argument is missing, similarly null : Null. So we have to treat missing arguments and coercion failures differently when the requested type is Null (or null respectively, in Candid).

This PR corrects the reading of (e.g. \x70, <reserved>) in the non-recoverable case and makes it a coercion failure.

Also adds improvements to the candid-tests runner to

not ignore the case when a field type can be inferred, but not in the record type (eliminating M0164, ignored test)
adds null values to top-level argument tuples where the type requests them
adds null fields to record values (in argument sequences) where the type requests them

Flake lock file updates: • Updated input 'candid-src': 'github:dfinity/candid/1725ea211e928a4dc18c4147d27ed5764f00df5c?narHash=sha256-7/aBuCJUtH5nofVfgyedqI6t7a21H%2B14kwmYOGFbO8g%3D' (2025-10-02) → 'github:dfinity/candid/4e8357d17198e53b4d426636a0b70d1433b40390?narHash=sha256-lRjdOwUF06g04qjUhFVU19sjYZooSHEZa3cKcpiZHYQ%3D' (2025-10-16) • Updated input 'ic-wasm-src': 'github:dfinity/ic-wasm/d61f15ea363eebb952195f57cf9c5db3b2cade21?narHash=sha256-PTwtdFVGGofm934tlikb2cNq932yuBjXYtN5TKXfwmg%3D' (2025-10-01) → 'github:dfinity/ic-wasm/10f1b5965c8c4b7fd1532734b60dcb2a2fcb2673?narHash=sha256-9vwXW4NTJzdwQUPk5%2BvHp0uWFJr6o3/Pj/wZ26gTyfM%3D' (2025-10-22) • Updated input 'motoko-core-src': 'github:dfinity/motoko-core/c49a060df3e066463d81664f27065238ca7cc23f?narHash=sha256-INchg3S6uo8qKUvShjHmGVhgKq2FdQkcYKJkE2GLDoY%3D' (2025-10-14) → 'github:dfinity/motoko-core/c707f40fe514bbde9a33869e71a86c6d36ce7e32?narHash=sha256-knLQ0jGi0OcZTEmhRFSaUjqQkrOJZ4eGKJhGc3o%2B8IE%3D' (2025-10-28)

`env OCAMLRUNPARAM=b candid-tests -p construct:82` starts to work still stuff to do

> warning [M0215], field `_1_` is provided but not expected in record of type {_0_ : Null}

> assert blob "DIDL\00\02\7d\7d\05\06" == "(5, 6)" : (nat, nat, null) "parsing a top-level tuple into a longer top-level tuple"; current failure: > construct:215 parsing reserved as null ... not ok (unwanted pass)!

github-actions · 2025-10-29T12:32:31Z

Comparing from 60f59eb to 775cdb1:
In terms of gas, no changes are observed in 5 tests.
In terms of size, no changes are observed in 5 tests.

To wit: ``` let ?((null, 5), null) : ?((Null, Nat), Null) = from_candid "DIDL\01\6C\02\00\70\01\7D\02\00\7C\05\7A" else break good; ``` the first `Null` gets derailed by `\70` (`Reserved` singleton), but the second `Null` is unaffected by the `int` (-6). Changing `\70` back to `\7F` (i.e. `null`), gives acceptance.

src/mo_idl/idl_to_mo_value.ml

ggreif · 2025-11-04T12:35:37Z

src/mo_idl/idl_to_mo_value.ml


-let args vs ts = parens_comma (List.map2 value vs.it ts)
+let rec args vs = function
+  | ts when List.(compare_lengths vs.it ts < 0 && for_all null (Lib.List.drop (length vs.it) ts)) ->


Extends values to be checked when shorter than types and those are all Null.

ggreif · 2025-11-04T12:37:07Z

src/mo_idl/idl_to_mo_value.ml

+  | ts when List.(compare_lengths vs.it ts < 0 && for_all null (Lib.List.drop (length vs.it) ts)) ->
+    let vs' = vs.it @ Lib.List.replicate { vs with it = NullV } List.(length ts - length vs.it) in
+    args {vs with it = vs'} ts
+  | ts when List.(exists (fun (t, v) -> apart t v.it) (combine ts vs.it)) ->


Adds null-ed fields when the object type calls for them.

In

assert blob "DIDL\01\6c\01\01\7d\01\00\05" == "(record { 1 = 5 })" : (record { 0 : null }) "parsing into record with expected field that is less than extra field on the wire";

The record { 1 = 5 } fragment need an augmenting to record { 1 = 5; 0 = null } so that the Motoko generation creates type-correct code.

ggreif · 2025-11-04T12:39:57Z

src/idllib/idlHash.ml

  List.fold_left
    (fun s c -> add (mul s (of_int 223)) (of_int (Char.code c)))
    zero
-    (* TODO: also unescape the string, once #465 is approved *)


This was actually resolved back then.

crusso · 2025-11-04T13:07:47Z

src/codegen/compile_classical.ml

-            | Prim Null | Opt _ | Any ->
-              (Bool.lit true, fun msg -> Opt.null_lit env)
+            | Prim Null ->
+              (get_can_recover, null_result, fun _ -> compile_unboxed_const (coercion_error_value env))


I don't quite grok why this case is different. Can you explain?

Yes, the spec says that every Candid value is a subtype of opt T, and gives opt t when t <: T. But the null type has no subtypes. I.e. we only get out a null : Null when the binary Candid has a 0x7F there. In all other cases the coercion must fail, either trapping or indicating a coercion failure to the caller (when recoverable). However null : ?T can always be materialised when an argument is missing, similarly null : Null. So we have to treat missing arguments and coercion failures differently when the requested type is Null (or null respectively, in Candid).

These are coercion failures:

assert blob "DIDL\00\01\70" !: (null) "parsing reserved as null"; assert blob "DIDL\00\02\70\70" !: (null, null) "parsing (reserved, reserved) as (null, null)";

Though, I must admit, I cannot really wrap my head around this one. @mraszyk can you give a hint? (The test always passed, so just quenching my curiosity.)

// parsing reserved as null assert blob "DIDL\00\01\70" == "(null)" : (reserved) "reserved";

These test the null-materialisation for missing top-level args, i.e. argument_default_or_trap:

assert blob "DIDL\00\00" == "()" : (null) "parsing an empty top-level tuple into a longer top-level tuple"; assert blob "DIDL\00\02\7d\7d\05\06" == "(5, 6)" : (nat, nat, null) "parsing a top-level tuple into a longer top-level tuple";

// parsing reserved as null assert blob "DIDL\00\01\70" == "(null)" : (reserved) "reserved";

Just to clarify, it think the comment should reading // parsing reserved as null value of type reserved, if that helps any.

\70 is a value of type reserved, so I don't see how a null appears suddenly in the game...

crusso

Would be good to add a PR description.

ggreif · 2025-11-04T13:33:00Z

test/run-drun/ok/candid_opts.drun-run.ok

 debug.print: {o1 = {x = null}; o2 = {x = null}; o3 = {x = null}}
 debug.print: {o1 = {x = null}; o2 = {x = ?null}; o3 = {x = ?(?null)}}
 ingress Completed: Reply: 0x4449444c0000
+ingress Err: IC0503: Error from Canister rwlgt-iiaaa-aaaaa-aaaaa-cai: Canister called `ic0.trap` with message: 'IDL error: unexpected IDL type when parsing Null'.


These are totally expected. A <reserved> marker (i.e. \x70 = -16) is not digestible by null : Null pattern, similarly a 5 : nat (here x7D\x05).

ggreif · 2025-11-04T13:40:25Z

src/exes/candid_tests.ml

            Unix.putenv "MOC_UNLOCK_PRIM" "yesplease";
            write_file "tmp.mo" src;
-            match run_cmd "moc -Werror -wasi-system-api tmp.mo -o tmp.wasm" with
+            match run_cmd "moc -A M0215 -Werror -wasi-system-api tmp.mo -o tmp.wasm" with


The Motoko code generated by these

assert blob "DIDL\01\6c\01\00\7d\01\00\05" == "(record { 0 = 5 })" : (record { 1 : null }) "parsing into record with expected field that is greater than extra field on the wire"; assert blob "DIDL\01\6c\01\01\7d\01\00\05" == "(record { 1 = 5 })" : (record { 0 : null }) "parsing into record with expected field that is less than extra field on the wire";

would contain dead fields, which cause these warnings. I couldn't come up with a massaging so that the warning goes away, that doesn't change the meaning of the test. So I have just suppressed the warning so that -Werror won't kill compilation.

ggreif · 2025-11-04T14:11:20Z

src/mo_idl/idl_to_mo_value.ml

+    args {vs with it = vs'} ts
+  | ts when List.(exists (fun (t, v) -> apart t v.it) (combine ts vs.it)) ->
+    args {vs with it = List.map2 enrich ts vs.it} ts
+  | ts -> parens_comma (List.map2 value vs.it ts)


This is the vanilla case, and we should exit here after all massaging from above is done.

ggreif · 2025-11-04T14:14:32Z

src/mo_idl/idl_to_mo_value.ml

+  | ts when List.(exists (fun (t, v) -> apart t v.it) (combine ts vs.it)) ->
+    args {vs with it = List.map2 enrich ts vs.it} ts
+  | ts -> parens_comma (List.map2 value vs.it ts)
+and null t = t = T.(Prim Null)


rename to is_null

ggreif · 2025-11-04T14:28:57Z

src/mo_idl/idl_to_mo_value.ml

+  | RecordV fs, T.(Obj (Object, tfs)) ->
    "{" ^ String.concat "; " (List.map (fun f ->
-      Idl_to_mo.check_label (fst f.it) ^ " = " ^ value (snd f.it) (find_typ tfs f)
+      Idl_to_mo.check_label (fst f.it) ^ " = " ^ value (snd f.it) (find_typ ~infer:infer_typ tfs f)


This deals with previously errored (and ignored) test cases

construct:233 parsing into record with expected field that is greater than extra field on the wire ... ignored ((string):1.11-1.16: import error [M0164], unknown record or variant label in textual representation ) construct:234 parsing into record with expected field that is less than extra field on the wire ... ignored ((string):1.11-1.16: import error [M0164], unknown record or variant label in textual representation

arising from

assert blob "DIDL\01\6c\01\00\7d\01\00\05" == "(record { 0 = 5 })" : (record { 1 : null }) "parsing into record with expected field that is greater than extra field on the wire"; assert blob "DIDL\01\6c\01\01\7d\01\00\05" == "(record { 1 = 5 })" : (record { 0 : null }) "parsing into record with expected field that is less than extra field on the wire";

crusso · 2025-11-04T15:27:42Z

src/mo_idl/idl_to_mo_value.ml

+  | ts when List.(exists (fun (t, v) -> apart t v.it) (combine ts vs.it)) ->
+    args {vs with it = List.map2 enrich ts vs.it} ts
+  | ts -> parens_comma (List.map2 value vs.it ts)
+and is_null t = t = T.(Prim Null)


Suggested change

and is_null t = t = T.(Prim Null)

and is_null_opt_reserved t =

Should this maybe allow Null, Opt t an Any (after normalizing)?

crusso · 2025-11-04T15:29:41Z

src/mo_idl/idl_to_mo_value.ml

  | _ -> raise (UnsupportedCandidFeature
    (Diag.error_message v.at "M0165" "import" "odd expected type"))

-let args vs ts = parens_comma (List.map2 value vs.it ts)


I wonder if this code might be simpler if you just traversed the typs in ts and only allow trailing defaultable types?

crusso · 2025-11-04T15:32:35Z

src/mo_idl/idl_to_mo_value.ml

+    args {vs with it = List.map2 enrich ts vs.it} ts
+  | ts -> parens_comma (List.map2 value vs.it ts)
+and is_null t = t = T.(Prim Null)
+and apart t v = match t, v with


Ditto, this code might be simpler if you sorted the value and type fields by hash and then traverse the (type) fields in order, inserting preserving fields and inserting null fields when the type is defualtable.

Not sure.

crusso

Looks good to me. Thanks for tackling this!

github-actions bot and others added 24 commits October 29, 2025 00:27

test passes

623e61f

these will exist

25007d0

tweak

b6337ef

WIP: apply band-aid

035ff07

prep

98f454e

tweaks

93845e4

WIP

52326b5

improve

d1a5a75

add Lib.List.replicate

2d7b94b

add more cases

7b9e658

WIP: fix

e73c69c

`env OCAMLRUNPARAM=b candid-tests -p construct:82` starts to work still stuff to do

resolve a TODO

7d06ce3

WIP: refine

869a8f0

WIP: more

c5ed1f2

tweak

489e2ed

works: use unescape_hash

52b9dea

fix comments

2f4d30c

simplify

b2afd3d

format

7036369

some record fields can be inferred

1993eea

clever

36d6fed

accept warning M0215

9b373aa

> warning [M0215], field `_1_` is provided but not expected in record of type {_0_ : Null}

reproduce issue #5543

0ccce85

> assert blob "DIDL\00\02\7d\7d\05\06" == "(5, 6)" : (nat, nat, null) "parsing a top-level tuple into a longer top-level tuple"; current failure: > construct:215 parsing reserved as null ... not ok (unwanted pass)!

ggreif changed the base branch from master to claudio/issue-5504 October 29, 2025 12:22

ggreif added 4 commits October 29, 2025 14:00

WIP

d58b69a

WIP: prepare

c6d4545

WIP: progress

64d489f

ggreif changed the title ~~fix: catch up with Candid spec changes (reserved to null)~~ fix: catch up with Candid spec changes (reserved ↛ null) Nov 3, 2025

Merge commit '9b373aa' into gabor/reserved

137f83f

ggreif self-assigned this Nov 4, 2025

ggreif added the Bug Something isn't working label Nov 4, 2025

ggreif added 2 commits November 4, 2025 12:30

when parsing null, consider the can_recover flag

c5c51e8

backport to classical

81191e3

ggreif marked this pull request as ready for review November 4, 2025 12:21

ggreif requested a review from a team as a code owner November 4, 2025 12:21

ggreif requested a review from crusso November 4, 2025 12:23

ggreif commented Nov 4, 2025

View reviewed changes

src/mo_idl/idl_to_mo_value.ml Outdated Show resolved Hide resolved

ggreif commented Nov 4, 2025

View reviewed changes

crusso reviewed Nov 4, 2025

View reviewed changes

ggreif commented Nov 4, 2025

View reviewed changes

ggreif requested review from Kamirus and mraszyk November 4, 2025 13:41

ggreif commented Nov 4, 2025

View reviewed changes

ggreif added 2 commits November 4, 2025 15:55

infer Int when the field value is negative

567bd9d

Update idl_to_mo_value.ml

775cdb1

crusso reviewed Nov 4, 2025

View reviewed changes

crusso self-requested a review November 4, 2025 15:40

crusso approved these changes Nov 4, 2025

View reviewed changes

	and is_null t = t = T.(Prim Null)
	and is_null_opt_reserved t =

fix: catch up with Candid spec changes (reserved ↛ null) #5619

Are you sure you want to change the base?

fix: catch up with Candid spec changes (reserved ↛ null) #5619

Uh oh!

Conversation

ggreif commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreif Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreif Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreif Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusso left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fix: catch up with Candid spec changes (`reserved` ↛ `null`) #5619

fix: catch up with Candid spec changes (`reserved` ↛ `null`) #5619

ggreif commented Oct 29, 2025 •

edited

Loading

github-actions bot commented Oct 29, 2025 •

edited

Loading

ggreif Nov 4, 2025 •

edited

Loading

ggreif Nov 4, 2025 •

edited

Loading

ggreif Nov 4, 2025 •

edited

Loading

crusso Nov 4, 2025 •

edited

Loading