[2.x] Adapt structure visitors to Codama v2 - #1175
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Ports the structure visitors of @codama/visitors to the v2 node model: instructionNode.data replaces arguments, enumVariantTypeNode.data replaces the three variant kinds, wrapper nodes become transforms, and identifiers are matched exactly. Along the way it extracts inlineDefinedType / qualifyDefinedTypeLinks as a shared helper (used by four visitors), fixes a good batch of latent bugs (dangling links in PDAs/events/constants, cross-program link resolution, wrong histogram keys, dedupe not repointing links, dropped PDA programId/plugins, dropped variant discriminator/display), and adds twelve new test files.
I read every changed file and cross-checked the factory signatures the code relies on (structTypeNode(fields, { ...node }), enumVariantTypeNode(id, { ...variant }), addTypeNodeTransforms, fieldDiscriminatorNode(path, { offset }), bytesTypeNode({ transforms, plugins }), EnumTypeNode.size being required, PdaNodeInput). They all line up. The variant.discriminator ?? variantIndex fallback matches the spec's "index of the variant, starting at 0" rule.
Things worth knowing (not blockers)
getDefinedTypeHistogramVisitornow counts every link intotal, including those in instruction account default values, PDA seeds and constants. In v1,visitInstructiononly walkedarguments/extraArguments, so those uses were invisible. Downstream,unwrapInstructionDataDefinedTypesVisitorwill now inline fewer types than before in IDLs where a type is used both as instruction data and, say, in aninstructionAccountNode.defaultValue. That is the correct outcome (v1 would have inlined and removed the type, leaving the default value's link dangling), but it is a behaviour change anyone comparing v1/v2 output should expect.- Sub-instruction naming is now
${instruction}_${variant}verbatim (e.g.transferTokens_V1) instead of camelCased. Valid per the identifier grammar and consistent with the exact-identifier policy, but it produces mixed-style identifiers when the inputs use different conventions; renderers will re-case anyway. flattenStructconflict detection is now camelCase-based, somax_supply+maxSupplythrows where v1 silently accepted. Correct per the spec's collision rule; just noting it sinceflattenInstructionDataVisitorruns on every instruction in the Anchor default pipeline.deduplicateIdenticalDefinedTypesVisitor: the "unqualified links must be deduplicated across the same programs" fixpoint plus the repointing pass is the right design. I walked the P0/P1/P2 cases (referenced type present in a superset of programs, kept copy in an earlier program than the referrer) and repointing handles them.unwrapDefinedTypesVisitorwith'*'on a self-referential defined type would still recurse forever — pre-existing, not introduced here.- Changeset: none in this PR. If the 2.x branch already carries the seeded major changeset for all public packages that's fine; otherwise a changeset for the renamed exports / behaviour changes would be expected per CONTRIBUTING.md.
Notes for subsequent reviewers
createSubInstructionsFromEnumDataVisitoris the most logic-dense file: the doubleflattenStructat the end relies on the first pass inlining the{ discriminator, payload }wrapper only when the enum has no transforms, and the second pass inlining the payload struct only when it has none. The test "keeps the discriminator and payload grouped when the enum has transforms" covers the no-op path; worth eyeballing.getLinkUsage(histogram) derives "direct" from path shape:[…, instruction, link]or[…, instruction, struct, field, link]withfield.type === link. That excludes a link used as a top-level field'sdefaultValue, which is intended.- The
as StructTypeNodecast inunwrapTupleEnumWithSingleStructVisitoris safe becauseaddTypeNodeTransformspreserveskind.
A few small inline nits below.
5febf52 to
68455c3
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Re-fetched the files touched since my last pass (errors/*, addPdasVisitor, createSubInstructionsFromEnumDataVisitor, flattenStructVisitor, and their tests). Everything I raised has been addressed:
resolveTypenow goes throughlinkables.getPathOrThrow, so an unresolvabledata/field link surfaces asLINKED_NODE_NOT_FOUNDinstead of the misleading "field not found" — covered by a new test. The docblock now listsUNEXPECTED_NODE_KINDfor non-struct data too.- Payload field carries
plugins: field.pluginsagain, with a test. flattenStructthrows the newCANNOT_FLATTEN_STRUCT_WITH_PLUGINS(1200014) rather than silently dropping plugins, with context/message/imports wired up in@codama/errors(StructFieldTypeNodewas already imported incontext.ts), and both the throwing path and the "not selected → no throw" path are tested.addPdasVisitoruses aSetof camelCase keys.- Test import order fixed.
Still approved.
One thing to keep in mind (not blocking)
The transforms/plugins asymmetry in flattenStruct — transforms skip the field, plugins throw — is a deliberate choice and defensible (transforms have a clear "leave it alone" fallback; plugins don't). But it does make two '*' callers stricter than before:
flattenInstructionDataVisitorruns on every instruction in the Anchor default pipeline. Any nested struct that ends up withplugins(e.g. from a future Anchor visitor or a user pre-pass) will now make the whole pipeline throw with no per-field opt-out.- The second
flattenStructincreateSubInstructionsFromEnumDataVisitorinlines the variant's struct payload. A variant withdata: structTypeNode([...], { plugins })will now throw there, and the user has no knob to keep it grouped instead.
If that's the intended contract ("plugins on a struct you asked to flatten is a user error, fix your input"), it's fine as-is. If you'd rather these pipeline visitors degrade gracefully, an option like { onPlugins: 'throw' | 'skip' } on FlattenStructOptions (defaulting to 'throw') would give the default pipelines a way to opt into skipping. Can be a follow-up if it ever bites.
68455c3 to
599fa34
Compare
4354390 to
fc393d4
Compare
82d679f to
e48e185
Compare
42578fd to
7d45051
Compare
66f05b8 to
c4f8eac
Compare
7d45051 to
be3fca0
Compare
c4f8eac to
ae160c2
Compare
be3fca0 to
26fcebe
Compare
ae160c2 to
be4576d
Compare

This PR adapts the structure visitors of
@codama/visitorsto the Codama v2 node model: the visitors that flatten, unwrap, transform or deduplicate types, plus the account, struct-default and PDA helpers. Theupdate*visitors and the remaining instruction visitors are adapted separately, so the package does not fully type-check yet.Renamed exports
flattenInstructionDataArgumentsVisitorflattenInstructionDataVisitorunwrapInstructionArgsDefinedTypesVisitorunwrapInstructionDataDefinedTypesVisitorcreateSubInstructionsFromEnumArgsVisitorcreateSubInstructionsFromEnumDataVisitorDefinedTypeHistogramkeysdirectlyAsInstructionArgs/inInstructionArgsdirectlyAsInstructionData/inInstructionDataflattenInstructionArguments/FlattenInstructionArgumentsConfigflattenStruct/FlattenStructOptionsCODAMA_ERROR__VISITORS__INSTRUCTION_ENUM_ARGUMENT_NOT_FOUND(contextargumentName)CODAMA_ERROR__VISITORS__INSTRUCTION_ENUM_DATA_FIELD_NOT_FOUND(contextfieldName), same code1200009CODAMA_ERROR__UNEXPECTED_NESTED_NODE_KINDis marked as deprecated since nested type node wrappers no longer exist. A newCODAMA_ERROR__VISITORS__CANNOT_FLATTEN_STRUCT_WITH_PLUGINSerror (1200014) is added.Behaviour
NodeSelector. Conflict checks (flattenStruct,addPdasVisitor) compare camelCase forms, following the spec's casing-collision rule.transformsandplugins, and inlining adefinedTypeLinkNodelayers the link's owntransformson top of the inlined type.flattenStructno longer inlines structs that carry transforms, since that would change their wire format, and throws the newCODAMA_ERROR__VISITORS__CANNOT_FLATTEN_STRUCT_WITH_PLUGINSerror when a struct to inline carries plugins, since they would be lost.instructionNode.data. A "direct" use in the histogram is now an instruction'sdataitself or the type of one of its top-level data fields, sounwrapInstructionDataDefinedTypesVisitoralso inlinesdata: definedTypeLinkNode(...).unwrapTupleEnumWithSingleStructVisitorturnsdata: tupleTypeNode([struct])intodata: struct, keeping the tuple's transforms and the variant's discriminator.createSubInstructionsFromEnumDataVisitorsupports linked data and linked enums, and names sub-instructions${instruction}_${variant}without changing their casing. The discriminator field (${instruction}_${variant}_discriminator) uses the enum'ssizeand the variant's discriminator rather than a hard-codedu8index, and only the enum field is flattened.transformU8ArraysToBytesVisitoremitsbytesTypeNodewith afixedSizeTransformNode, only for plainu8items, keeping the array's transforms outside the fixed size.setAccountDiscriminatorFromFieldVisitorand skipped byflattenInstructionDataVisitor, since changing it would affect every user of the defined type.transformDefinedTypesIntoAccountsVisitoraccepts any type, not just structs.Bug fixes
unwrapDefinedTypesVisitorleft links inside PDAs, events and constants dangling.unwrapTupleEnumWithSingleStructVisitorlooked defined types up by the wrong histogram key, removing types that were still used, and mixed up same-named types across programs.getDefinedTypeHistogramVisitorkeyed cross-program links under the enclosing program.deduplicateIdenticalDefinedTypesVisitornow repoints links to removed copies at the kept one, and no longer merges types whose unqualified links resolve to different types.createSubInstructionsFromEnumDataVisitorcopied the parent's sub-instructions into every new sub-instruction; it now also reports unresolved links asCODAMA_ERROR__LINKED_NODE_NOT_FOUND.addPdasVisitornever matched programs with non-camelCase identifiers and droppedprogramIdandplugins.discriminatoranddisplay.Tests
Twelve new test files cover the visitors that had none, including regression tests for each fix above, and the existing tests are ported to v2 nodes. The README is updated accordingly.