[lorisleiva]

This PR adds helpers to detect cyclic defined types and fixes the existing cycle guards that got them wrong.

// list = struct { next: option<link(list)> }, loop = struct { next: link(loop) }
isDefinedTypeCyclic([root, program, list], linkables);       // true
hasDefinedTypeFiniteValue([root, program, list], linkables); // true: None stops the recursion
hasDefinedTypeFiniteValue([root, program, loop], linkables); // false

• New visitors: getDefinedTypeLinksVisitor lists the defined types a node links to, and getHasFiniteValueVisitor tells whether a type has a finite value.
• New helpers: isDefinedTypeCyclic and hasDefinedTypeFiniteValue are built on those visitors and leave LinkableDictionary unchanged. Both throw the new CODAMA_ERROR__NODE_PATH_PROGRAM_MISSING (16), via the new assertNodePathHasProgram, when the path has no program to resolve links from.
• Byte-size visitors: they track the types being visited by node rather than by name, so a link to a same-named type of another program no longer counts as a cycle.
• getInstructionDataFields: sibling fields linking the same type are now all expanded. Previously only the first one was.
• unwrapDefinedTypesVisitor: keeps defined types in a cycle made only of types to inline, and the links to them. Previously it recursed forever. A cycle going through a type that is not inlined ends at its link, so its other types are still inlined.

[lorisleiva]

• [2.x] Format decoded nodes into formatted node trees #1218
• [2.x] Add formatters for booleans, bytes, enums and public keys #1217
• [2.x] Return decoded nodes from dynamic parsers #1216
• [2.x] Add formatters for decoded nodes #1215
• [2.x] Decode Codama nodes into decoded node trees #1214
• [2.x] Report defined types without a finite value in validators #1213
• [2.x] Support recursive defined types in dynamic codecs #1212
• [2.x] Detect cyclic defined types #1211 👈 (View in Graphite)
• [2.x] Port the dynamic client to Codama v2 #1209
• [2.x] Reject duplicate set items in dynamic codecs #1208
• [2.x] Upgrade v1 instructions, programs and roots to v2 #1207
• [2.x] Keep the cause of validation items and drop their node #1206
• [2.x] Make NodePath non-distributive over node unions #1205
• [2.x] Use the published v1 node types in @codama/upgrade #1201
• [2.x] Add v1 to v2 upgrade converters for types and values #1200
• [2.x] Adapt to spec 2.0.0-rc.1 #1196
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[changeset-bot]

⚠️ No Changeset found

Latest commit: bdccdf4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[lorisleiva]

@trevor-cortex

[trevor-cortex]

Summary

Adds first-class cycle detection for defined types and fixes the places that got it wrong:

• getDefinedTypeLinksVisitor (lists resolved link targets one hop out, skipping enumValueNode.enum) and getHasFiniteValueVisitor (inhabitedness check that treats options / non-fixed collections / alternative enum variants as recursion exits).
• isDefinedTypeCyclic (reachability search over the links visitor) and hasDefinedTypeFiniteValue, both guarded by a new CODAMA_ERROR__NODE_PATH_PROGRAM_MISSING when the path can't resolve links.
• getByteSizeVisitor / getMaxByteSizeVisitor now key their memo and in-progress set by DefinedTypeNode identity instead of identifier, so a link to a same-named type in another program is no longer mistaken for a cycle.
• getInstructionDataFields scopes its "followed" set to the current branch so sibling fields linking the same type all expand.
• unwrapDefinedTypesVisitor keeps cyclic types (and links to them) instead of recursing forever.

I traced the visitor plumbing (extendVisitor → interceptVisitor this propagation, NodeStack.visitPath + recordNodeStackVisitor pushing the last node back) and the self/stack usage in the new visitors is correct. The byte-size memoization-under-cycle reasoning still holds with node keys: anything memoized as null because it hit an in-progress type is itself on that cycle, so the cached value is genuinely right in every context. Tests are thorough, including the cross-program same-name cases and the "recorded later" cases that confirm LinkableDictionary isn't mutated.

Things to check before merging

• Changeset. CONTRIBUTING.md asks for one on any user-facing change, and this PR adds public API (four exports + an error code) and changes the behavior of three existing visitors/helpers. I don't see a .changeset/*.md in the file list. If the 2.x line handles this differently (e.g. everything rolls into a seeded major changeset), ignore this.
• Doc nuance in hasDefinedTypeFiniteValue: "Only cyclic types can lack one" isn't quite true — visitEnumType returns false for an enum with zero variants (correctly: it's uninhabited). Tiny wording thing, but worth a tweak if you want the doc to be exact.

Notes for other reviewers / non-blocking

• getHasFiniteValueVisitor has no memoization, unlike the byte-size visitors. Correctness-wise that's fine (a false is context-dependent on the in-progress set, so it can't be cached naively), but it means repeated shared sub-types get re-walked on every branch — exponential in the pathological "every level has two fields linking the next level" case. Not a problem today since nothing hot calls it; I left an inline note on the safe memoization option if it becomes one.
• unwrapDefinedTypesVisitor with an explicit type list is more conservative than strictly necessary (see inline comment on isCyclic). Behaviour for '*' is exactly right; the explicit-list case is a judgment call and the new test enshrines the conservative choice, so just flagging it as deliberate-or-not.
• Nothing else of substance. Error code 16 sits in the general range alongside LINKED_NODE_NOT_FOUND (4), which is the existing precedent for errors thrown from visitors-core.

[trevor-cortex]

Re-review

Re-fetched everything. All five points from the first pass are addressed, and the new code checks out:

• getHasFiniteValueVisitor now caches true results only, with a comment explaining why false can't be cached. The new "first reached within a cycle it does not need" test (forest → tree → branch → tree) is exactly the case that would break a naive false cache, so it's good to have it pinned.
• Empty-enum nuance is now in the docs (hasDefinedTypeFiniteValue, README) and covered by a test.
• assertNodePathHasProgram moved to NodePath.ts with its own tests; pending replaces queue.
• unwrapDefinedTypesVisitor now computes cycles over types-to-inline only (isInInlinedCycle), so ['a'] on a ↔ b inlines a and stops at link(b), as the new test shows. I traced the tricky interactions: the search uses its own linksStack, so it doesn't disturb the main stack; the cache is keyed by original DefinedTypeNode identity, which is what both linkables and the filtered program.definedTypes carry; and a kept link to a cyclic type inside a type inlined across programs still gets qualified, because the early return link happens inside visit(definedType.type, self) and inlineDefinedType qualifies the result afterwards.

Nothing blocking. Two small non-blocking notes below (one inline). Still no .changeset/*.md in the diff — given this targets a stacked 10-05-… 2.x branch I'm assuming it rolls into the 2.x release changeset; ignore if so.

[trevor-cortex]

Re-review (3)

Re-fetched all diffs. The only change since my last pass is a two-line addition to the getHasFiniteValueVisitor doc comment, explaining why a link back to a type being visited yields false on its own and calling out the empty-enum case. That reads well and matches the behaviour and tests.

Everything else is byte-for-byte what I approved last time, so nothing new to add — the open non-blocking notes (optional shouldFollow on isDefinedTypeCyclic to dedupe isInInlinedCycle, changeset if the 2.x stack needs one per PR) still stand as take-it-or-leave-it. Good to merge from my side.