Skip to content

fix: Candid repros and fixes for issues #5504, #5543 - #5505

Merged
ggreif merged 16 commits into
masterfrom
claudio/issue-5504
Nov 6, 2025
Merged

ggreif merged 16 commits into
masterfrom
claudio/issue-5504

Conversation

@crusso

@crusso crusso commented Sep 22, 2025 •

Copy link
Copy Markdown
Contributor

Implement Martin's revised deserialization for Candid options

This PR fixes coercion of values into nested optional types.

Before the fix from this PR, 5 : nat64 would coerce to opt 5 : opt na64 (as expected) and null : opt opt nat64 (unexpected). >This PR fixes the unexpected result to be opt opt 5 : opt opt nat64 (as expected).

Before the fix from this PR, 5 : int would coerce to null : opt opt nat. This PR fixes the result to be opt null : opt opt nat. The >motivation for this change is to enable using optional types as follows:

  • outer option characterizes if a value (of any type) is provided;
  • inner option expresses to optimistically deserialize the actual value into the given type (without failing to deserialize and without losing the information if a value of any type is provided).

This PR also adds multiple test cases covering the change to coercion (including coercion to nested optional types over null and reserved).

c.f.

Furthermore (from #5619) 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

TODO Tasks:

@crusso crusso changed the title repro for issue #5504 test: repro for issue #5504 Sep 22, 2025
@github-actions

github-actions Bot commented Sep 22, 2025 •

Copy link
Copy Markdown
Contributor

Comparing from 7012347 to b1b6869:
In terms of gas, no changes are observed in 5 tests.
In terms of size, no changes are observed in 5 tests.

@crusso crusso changed the title test: repro for issue #5504 fix: repro for issue #5504 Sep 22, 2025
@crusso crusso changed the title fix: repro for issue #5504 fix: repro and fix for issue #5504 Sep 22, 2025
@crusso crusso changed the title fix: repro and fix for issue #5504 experiment: repro and fix for issue #5504 Sep 22, 2025

@mraszyk mraszyk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to make the tests more compact (and better readable) using generics (analogously to the Rust tests)?

crusso and others added 6 commits September 30, 2025 14:17
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

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: pr-automation-bot-public[bot] <189003650+pr-automation-bot-public[bot]@users.noreply.github.com>
@ggreif ggreif changed the title experiment: repro and fix for issue #5504 fix: repros and fixes for issues #5504, #5543 Nov 4, 2025
@ggreif
ggreif marked this pull request as ready for review November 5, 2025 11:08
@ggreif
ggreif requested a review from a team as a code owner November 5, 2025 11:08
@ggreif
ggreif requested a review from Kamirus November 5, 2025 11:16
@ggreif ggreif self-assigned this Nov 5, 2025
@ggreif
ggreif enabled auto-merge (squash) November 6, 2025 12:52
@ggreif ggreif changed the title fix: repros and fixes for issues #5504, #5543 fix: Candid repros and fixes for issues #5504, #5543 Nov 6, 2025
@ggreif
ggreif force-pushed the claudio/issue-5504 branch from fd9ff85 to b1b6869 Compare November 6, 2025 13:13
@ggreif
ggreif merged commit 413223c into master Nov 6, 2025
21 checks passed
@ggreif
ggreif deleted the claudio/issue-5504 branch November 6, 2025 13:28
lwshang added a commit to dfinity/candid that referenced this pull request Oct 21, 2026
## Summary

`type Opt = opt Opt` is an *infinite option*: an option whose
constituent is again an option, forever. The spec's coercion rules don't
say what happens when a value whose type does not accept `null` (say
`true : bool`) is coerced at one. The implementations and the test suite
agree that decoding fails. This PR writes that into the spec. No
implementation changes.

## Why the spec has a gap here

Before #678, the rule via the constituent type had a premise `not (null
<: <t'>)`, so it never unwrapped an option inside an option. Coercion
was well founded, and `true : bool` at `Opt` gave `null`
(test/construct.test.did asserted exactly that, from #126).

#678 dropped the premise so that `5 : nat` coerces to `opt opt 5 : opt
opt nat`. At an infinite option, that rule now asks for a coercion of
the same value one level down, and the rule that goes to `null` applies
only if that coercion doesn't exist. So the answer depends on its own
negation, and the spec's own soundness property (`nat <: Opt`, so some
coercion should exist) contradicts any reading of it.

## What the implementations do

- **`candid` ≥ 0.10.20 (#679) rejects.** It keeps unwrapping the
expected type until its recursion limit. That error is not a `Subtype`
error, so the `opt` rule's recovery doesn't catch it, and the whole
message fails, even under an enclosing `opt`, in a record field, or in a
vector element. Probed directly; the new assertions cover those cases.
- **test/construct.test.did** has asserted `!:` for `true : bool` at
`Opt` since #679. The review comment there: `true : bool` "cannot coerce
into an _infinitely_ nested optional type".
- **Motoko** runs this repo's `test/` suite in CI with no
expected-failure list, and #679's approver ported the change there
(caffeinelabs/motoko#5505). The new assertions will check the
enclosing-`opt`, record and vector cases on Motoko's side when it picks
up this suite.

## The change

- **`spec/Candid.md`, Coercion › Options:**
  - Defines an infinite option.
- States that coercing a value whose type does not accept `null` at one
is *undefined*. That is distinct from failing to coerce: a `not (exists
<w>. ...)` premise only holds where coercion is defined.
  - So no enclosing option recovers, and deserialisation fails.
- **`spec/Candid.md`, Properties:** soundness states this as its
exception.
- **`test/construct.test.did`:** new assertions:
- the mutually recursive case (`type OptA = opt OptB; type OptB = opt
OptA`)
  - an enclosing option (`opt true : opt bool` at `Opt`)
  - a record field and a vector element at `Opt`
- an empty `vec bool` at `vec Opt`, which decodes, since there is
nothing to coerce.

Each rejected blob was checked to decode at a compatible type, so it
fails because of the cycle and not because the bytes are malformed.
- **`CHANGELOG.md`:** an `Unreleased` spec entry.

The spec's `Version: 0.1.8` line is unchanged, as with #678 and #748.

## Follow-up

The Lean model on `rewrite` (#777) currently answers `null` here. Once
this lands it will follow the spec: `coerce` gets an "undefined" outcome
that no `opt` absorbs, and the vector that is currently a known
divergence becomes an agreement.

## Test

```
cargo test -p candid_parser --test test_suite
```

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

candid: spec compliance

3 participants