Repository navigation
[2.x] Support recursive defined types in dynamic codecs - #1212
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds recursive defined type support to the dynamic codec visitor. visitDefinedType registers the type in a definedTypesInProgress map (keyed by node identity) for the duration of its creation; visitDefinedTypeLink now resolves the linked path itself and, when the target is in progress, returns a getLazyCodec that defers to the finished codec on first use. Before handing out a lazy codec, hasDefinedTypeFiniteValue (from @codama/visitors-core) rejects types with no finite value with the new CODAMA_ERROR__DEFINED_TYPE_HAS_NO_FINITE_VALUE (17). Link transforms still apply on top of the lazy codec via the existing intercept layer, so size prefixes etc. work at every depth.
I walked through the key paths and they hold up:
- Identity-keyed lookup via the resolved path's last node correctly distinguishes same-named types across programs (covered by test).
- Mutual recursion through intermediate types (
person↔friends) works because only the type whose codec is actually in progress gets the lazy treatment; intermediates are created normally. - The finite check is anchored on the back-linked type, so
usesLoop → loopcorrectly reportsloop, and a type likea = struct { x: link(b) },b = struct { z: link(a) }is correctly rejected asa. inProgressis a per-call object captured by the lazy closure, so it stays valid after the map entry is deleted infinally, and a throw mid-creation cleans up properly for reused visitors (e.g. indynamic-parsers).- Verified the expected hex in all four happy-path tests by hand.
Things to watch
- Changeset:
@codama/dynamic-codecsand@codama/errorsare both public and this is a user-facing feature + new error code, but there's no.changeset/*.mdin the diff. If a lower PR in the stack already covers this concern, ignore; otherwise one is needed before merge per CONTRIBUTING. - Finite check semantics:
hasDefinedTypeFiniteValuealso returnsfalsefor types containing a variant-less enum, so e.g.a = struct { next: option<link(a)>, e: enum {} }throwsDEFINED_TYPE_HAS_NO_FINITE_VALUEwith a message that blames nesting. The type genuinely has no values so the error code is defensible, just noting the message is slightly off in that corner — and that the non-recursivestruct { e: enum {} }doesn't throw at all. Not blocking. - Minor nits inline: optional caching of the finite check, a small readability tweak in
visitDefinedType, and the untested invariant-violation branch.
For subsequent reviewers
hasDefinedTypeFiniteValue / getHasFiniteValueVisitor are not in this diff — they land in a lower PR of the stack. Their rules (options/enums/non-fixed collections stop recursion; transforms don't) are what make the "lazy codecs never recurse at creation time" guarantee hold, so review those together with this one.
8ae8301 to
89c5b85
Compare
3b7d3df to
8618f69
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Re-fetched the diff. All three points from my previous pass are addressed:
- Finite check caching:
finiteDefinedTypesset on the visitor, keyed by node identity, populated only on success — so a failing type still throws on every attempt, which is the right behaviour. The check and the lazy codec creation are now neatly bundled ingetRecursiveCodec. visitDefinedTypereadability:inProgressis now explicitly typed and the codec is returned from a local rather than through narrowing across thetry/finally.- Invariant-violation test: the new
sentinelTransformNode(constantValueNode(definedTypeLinkNode('list'), …))case covers the branch and locks in the message. The option wrapper keeps the type finite so the check passes, and the sentinel'sgetConstantBytesthen hits the lazy codec beforeinProgress.codecis set — exactly the path the comment describes.
Logic in visitDefinedTypeLink / getRecursiveCodec is otherwise unchanged from what I verified last time. Still LGTM.
Still outstanding
- Changeset: still no
.changeset/*.mdin this PR.@codama/dynamic-codecsis public and gains a user-facing feature;@codama/errorsgains a new code (and is in thefixedgroup, so it'll bump with the core packages regardless). If the lowerdetect_cyclic_defined_typesPR already adds one that mentions dynamic-codecs, fine — otherwise it needs one before merge.
8618f69 to
92b3026
Compare
89c5b85 to
76c0dff
Compare
92b3026 to
a3e0b98
Compare
9d286f5 to
ae9c46e
Compare
a3e0b98 to
a9f4701
Compare
a9f4701 to
ea6f5df
Compare
c50d356 to
a9dfadc
Compare
6510101 to
f925e67
Compare
46a0398 to
f9b2018
Compare
f925e67 to
9e3d79c
Compare
f9b2018 to
bdccdf4
Compare
9e3d79c to
228ce02
Compare
228ce02 to
557ab3d
Compare

This PR lets dynamic codecs handle defined types that link back to themselves, such as linked lists and trees. Creating their codecs used to overflow the call stack.
loop = struct { next: link(loop) }, throws the newCODAMA_ERROR__DEFINED_TYPE_HAS_NO_FINITE_VALUE(17) with{ name, path }.dynamic-parsers,dynamic-address-resolutionanddynamic-instructionscreate their codecs through this visitor, so they handle recursive types too. A parser test covers a recursive account.