[2.x] Adapt validators to Codama v2 - #1173
Conversation
|
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adapts @codama/validators to the v2 node model: identifier everywhere, single enumVariantTypeNode, origin gone, and the instruction checks now go through getResolvedInstructionInputsVisitor with the real ancestry/ProvidedScope so cycles, bad dependencies, invalid provided values and injected bumps are all reported through one path. Adds sibling-set identifier uniqueness (exact + camelCase-fold) across programs/instructions/structs/enums/PDAs, an error for unresolvable injections consumed within an instruction, and an info for plugin-less textNodes.
I verified the visitors-core plumbing this leans on and it lines up:
recordProvidedScopeVisitoronly pushes a frame whenprovidesis non-empty, so theouterScope.pop()guard is exactly right (though it's a coupling worth keeping in mind if that visitor ever changes).- The resolver's own
recordNodeStackVisitor/recordProvidedScopeVisitorre-push the instruction, so passingslice(0, -1)for both is correct — no double push. ProvidedScope.resolvewithREGISTERED_NODE_KINDScan never hit the invalid-kind throw, sovisitInjectedValueis exception-safe. TheisNestedexclusion forprovidedNode/injectedValueNodeancestors is sound: a nested injection is either unreachable (shadowed fallback) or surfaces as a dead-end when the consuming injection is checked.instructionRemainingAccountsNodedoes carry anidentifierin v2, so that collision scope is legitimate.
Things to watch out for
camelCase algorithm divergence (main point, inline). getCamelCaseForm disagrees with camelCase from @codama/nodes on SCREAMING_SNAKE_CASE: MAX_SIZE → maxSize in nodes vs mAXSIZE here. Since renderers emit via the nodes helper, MAX_SIZE + max_size constants would pass validation and then collide in generated code. Either the validator should reuse camelCase from nodes, or (if the spec rule deliberately defines the split-before-uppercase algorithm) the nodes helper is what needs aligning — but the two shouldn't disagree. A test with an all-caps identifier would pin the intended behaviour either way.
Changeset. CONTRIBUTING says user-facing changes need one and this changes messages and adds new error rules. I know the branch is in pre/rc mode with a seeded major changeset — if the convention on 2.x adaptation PRs is to skip per-package changesets, ignore this; otherwise npx changeset add --empty for @codama/validators.
Notes for other reviewers
- The "has no identifier" checks for instruction accounts are still anchored on the instruction node rather than the account (pre-existing; inline nit).
- The follow-ups in the description (injections inside linked defined types, cross-scope identifier coincidence) are real gaps but reasonable to defer — the resolver already tolerates missing providers inside linked data shapes by design.
- Tests cover each new rule with a positive and a negative case; the
it validates program nodestest now builds an invalid node via a cast, which is the honest way to do it now that constructors brand identifiers.
352452c to
cf343be
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Re-fetched the diff. The one substantive point from my first pass is resolved: getIdentifierWords now implements the spec#175 algorithm (underscore split, lower/digit→upper boundary, acronym-run boundary) rather than the ad-hoc split-before-uppercase, so MAX_SUPPLY/maxSupply and getURL/get_url collide as intended, and the parametrised tests pin both the colliding and coexisting pairs. I walked the regexes against every case in the two test.each tables plus a few extras (getURL2Go, ABCDef, _foo, foo__bar) and they behave as documented. Approving.
One follow-up (out of scope here)
The spec algorithm and camelCase in @codama/nodes now differ in the other direction: camelCase('getURL') yields getURL and camelCase('HTTPServer') yields hTTPServer in nodes, whereas the validator folds them to getUrl/httpServer. Consequences today are mild — the validator is stricter for acronym identifiers, and the only case where nodes collides but the validator doesn't is contrived (HTTPServer vs hTTPServer, which the coexist test explicitly permits). But since the spec now defines the canonical word-splitting, it'd be worth moving getIdentifierWords into @codama/nodes and building camelCase/pascalCase/snakeCase on top of it so renderers and the validator agree by construction. Not for this PR.
Not re-raising
Changeset question and the "instruction account has no identifier" anchoring nit from the previous review both stand as-is; neither blocks.
cf343be to
8a167b4
Compare
039e0b0 to
bf3f736
Compare
8a167b4 to
b91b06a
Compare
bf3f736 to
98f423f
Compare
b91b06a to
2e1217c
Compare
98f423f to
9c63d57
Compare
2e1217c to
6a242eb
Compare
6a242eb to
2e57b9c
Compare

This PR adapts
@codama/validatorsto the v2 node model and adds the validation rules v2 introduces. The package is fully green: it type-checks, builds, tree-shakes, and 72/72 tests pass.Adapted
enumVariantTypeNode; the duplicated "variant has no name" check invisitEnumTypeis dropped.origincheck is removed, asprogramNode.originno longer exists.getResolvedInstructionInputsVisitoronce, with the instruction's ancestry and enclosing providers. Its errors — cyclic dependencies, invalid dependencies, invalid provided values, optional PDA seed accounts — are reported as validation errors. Non-Codama errors are now rethrown instead of being swallowed.Added
getURL/get_urland_foo/foocollide whilstfoo_dart/food_artcoexist. Exact duplicates and casing collisions are both errors. Covered scopes: root programs; a program's accounts, instructions, defined types, PDAs, events, errors and constants; an instruction's accounts, remaining accounts,provideskeys and sub-instructions; struct fields; enum variants; variable PDA seeds.textNodes without plugins, which the spec considers non-canonical.Follow-ups