bugfix: fix candid decoding at immutable array types to support opt d…

…efaulting (#4240)

Another Candid decoding bug, seems independent of #4238. 

I think the code for decoding immutable arrays was never extended to handle backtracking correctly.
When recovery is an option (haha) and the type descriptor does not match, the current code would continue to try to decode the array length and elements. When recovery is not an option, the previous trap would render this code unreachable. But when recovery is enabled, we need to bail and not try to read the array size and elements but just return the sentinel value instead.

- [ ] Consider applying the same refactoring to mutable array, if possible. 
      Since these aren't Candid, but extended Candid (for stable variables), and don't need to support recovery, the change is (probably) not necessary but nice for uniformity only.
- [ ] restore or delete original repro - I must have edited it while experimenting

Changelog.md

@@ -2,6 +2,8 @@
 
 * motoko (`moc`)
 
+  * bugfix: Unsuccessful Candid decoding of an optional array now default to null instead of crashing (#4240).
+
   * bugfix: Candid decoding of an optional, unknown variant with a payload now succeeds instead of crashing (#4238).
 
   * Implement Prim.textLowercase and Prim.textUppercase (via Rust) (#4216).

src/codegen/compile.ml

@@ -7062,6 +7062,9 @@ module MakeSerialization (Strm : Stream) = struct
           let (set_x, get_x) = new_local env "x" in
           let (set_val, get_val) = new_local env "val" in
           let (set_arg_typ, get_arg_typ) = new_local env "arg_typ" in
+          (* TODO: if possible refactor to match new Array t code,
+             (perhaps too risky and unnecessary for extended candid due to lack of fancy opt subtyping, see #4243)
+          *)
           with_composite_arg_typ get_array_typ idl_vec (ReadBuf.read_sleb128 env) ^^ set_arg_typ ^^
           ReadBuf.read_leb128 env get_data_buf ^^ set_len ^^
           get_len ^^ Arr.alloc env ^^ set_x ^^
@@ -7102,17 +7105,19 @@ module MakeSerialization (Strm : Stream) = struct
         let (set_x, get_x) = new_local env "x" in
         let (set_val, get_val) = new_local env "val" in
         let (set_arg_typ, get_arg_typ) = new_local env "arg_typ" in
-        with_composite_typ idl_vec (ReadBuf.read_sleb128 env) ^^ set_arg_typ ^^
-        ReadBuf.read_leb128 env get_data_buf ^^ set_len ^^
-        get_len ^^ Arr.alloc env ^^ set_x ^^
-        get_len ^^ from_0_to_n env (fun get_i ->
+        with_composite_typ idl_vec (fun get_typ_buf ->
+          ReadBuf.read_sleb128 env get_typ_buf ^^
+          set_arg_typ ^^
+          ReadBuf.read_leb128 env get_data_buf ^^ set_len ^^
+          get_len ^^ Arr.alloc env ^^ set_x ^^
+          get_len ^^ from_0_to_n env (fun get_i ->
           get_x ^^ get_i ^^ Arr.unsafe_idx env ^^
           get_arg_typ ^^ go env t ^^ set_val ^^
           remember_failure get_val ^^
           get_val ^^ store_ptr
         ) ^^
         get_x ^^
-        Tagged.allocation_barrier env
+        Tagged.allocation_barrier env)
       | Opt t ->
         check_prim_typ (Prim Null) ^^
         G.if1 I32Type (Opt.null_lit env)
@@ -7244,7 +7249,7 @@ module MakeSerialization (Strm : Stream) = struct
           Tagged.obj env Tagged.ObjInd [ compile_unboxed_const 0l ] ^^ set_result ^^
           on_alloc get_result ^^
           get_result ^^
-            get_arg_typ ^^ go env t ^^
+          get_arg_typ ^^ go env t ^^
           MutBox.store_field env
         )
       | Non ->

test/run/idl-decoding-bug.mo

@@ -0,0 +1,23 @@
+let b : Blob = to_candid (
+  ?[{a=?#a},
+    {a=?#b{}},
+    {a=?#b{}},
+    {a=?#a},
+    {a=?#c},
+    {a=?#a}] : ?[{a:?{#a;#b:{};#c}}]);
+
+let o1 = (from_candid b) : ?[{a:?{#a}}]; //note missing ?, forcing decoding at incorrect array type
+assert o1 == null;
+
+let o2 = (from_candid b) : ??[{a:?{#a}}]; //intended example, with embedded defaulting
+assert o2 ==
+  ??[{a=?#a},
+     {a=null},
+     {a=null},
+     {a=?#a},
+     {a=null},
+     {a=?#a}];
+
+//SKIP run
+//SKIP run-ir
+//SKIP run-low

test/run/idl-opt-tests.mo

@@ -0,0 +1,78 @@
+import P "mo:prim";
+
+do {
+  let b : Blob = to_candid(
+    (#a 1) :
+    ({#a:Int})
+  );
+  let o = (from_candid b) :?({#a:Int});
+  P.debugPrint(debug_show(o));
+  assert o == ? (#a 1);
+}; // ok
+
+do {
+  let b : Blob = to_candid(
+    (#a 1) :
+    ({#a:Int})
+  );
+  let o = (from_candid b) :?({#b:Int});
+  P.debugPrint(debug_show(o));
+  assert o == null;
+}; // ok
+
+do {
+  let b : Blob = to_candid(
+    (#b 1) :
+    ({#b:Int})
+  );
+  let o = (from_candid b) :?({#b:Nat});
+  P.debugPrint(debug_show(o));
+  assert o == null;
+};
+
+
+do {
+  let b : Blob = to_candid(
+    [#b 1] :
+    [{#b:Int}]
+  );
+  let o = (from_candid b) :?[{#b:Nat}];
+  P.debugPrint(debug_show(o));
+  assert o == null;
+}; // ok
+
+
+do {
+  let b : Blob = to_candid(
+    ?[1] :
+    ?[Int]
+  );
+  let o = (from_candid b) :??[Nat];
+  P.debugPrint(debug_show(o));
+  assert o == ?null;
+}; // ok
+
+do {
+  let b : Blob = to_candid(
+    ?[1] :
+    ?[Int]
+  );
+  let o = (from_candid b) :?[Nat];
+  P.debugPrint(debug_show(o));
+  assert o == null;
+}; // traps due to broken back-tracking in array decoding
+
+
+do {
+  let b : Blob = to_candid(
+    ?[#b 1] :
+    ?[{#b:Int}]
+  );
+  let o = (from_candid b) :?[{#b:Nat}];
+  P.debugPrint(debug_show(o));
+  assert o == null;
+}; // traps due to broken back-tracking in array decoding
+
+//SKIP run
+//SKIP run-ir
+//SKIP run-low

test/run/ok/idl-opt-tests.wasm-run.ok

@@ -0,0 +1,7 @@
+?(#a(+1))
+null
+null
+null
+?null
+null
+null