Adapt errors to Codama v2 - #1167
lorisleiva wants to merge 1 commit into
Conversation
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
068dda2 to
9d5c372
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adapts @codama/errors to the v2 node model: swaps CamelCaseString → IdentifierString across the identifier-bearing context fields, narrows the two resolver-only error contexts (CYCLIC_DEPENDENCY_... and INVALID_INSTRUCTION_DEFAULT_VALUE_DEPENDENCY) to the accounts-only / accountValueNode | dataValueNode shapes, and updates the typetest's example node kinds (numberTypeNode → integerTypeNode).
I verified the imported types against @codama/node-types at head: IdentifierString and PathString exist in brands.ts, DataValueNode.path is a PathString, AccountValueNode.identifier is an IdentifierString, and StructFieldTypeNode.defaultValue is ValueNode | undefined (so the reasoning that only account defaults can carry contextual references, hence accounts-only cycle/parent, holds). The mechanical CamelCaseString → IdentifierString sweep is correct throughout, and CamelCaseString still exists in brands.ts so nothing else breaks from dropping the import here.
Things to watch
- Description vs. code mismatch on
parentKind/messages.ts. The description saysparentKindwas dropped and theINVALID_...message template was reworded to remove$parentKind, but the diff only touches two files:context.tsstill hasparentKind: 'instructionAccountNode'(narrowed to a literal, not removed) andmessages.tsat head still interpolates[$parentKind]. The code as it stands is internally consistent, and keeping the now-constant field arguably respects the "Don't change or remove members of an error's context" note incontext.ts— so this is fine to land as-is. Just reconcile one way or the other: either update the description, or amend to actually drop the field + reword the template so the resolver PR builds against what the description promises. Inline comment below. - Cyclic-dependency message wording (nit, not in diff).
messages.tsstill says "resolving the accounts and arguments' default values" forCYCLIC_DEPENDENCY_.... Since the cycle is now accounts-only by construction, "arguments" is slightly misleading. Not blocking — could be folded into the resolver PR if you prefer to keep this one type-only. - Changeset.
@codama/errorsis in the fixed core group and this is a user-facing type change, but I'm assuming the v2 stack is covered by the seeded major changeset described in CONTRIBUTING.md rather than a per-PR one. Flagging only so a subsequent reviewer doesn't need to re-check.
Notes for subsequent reviewers
- The
dependencyName: IdentifierString | PathStringunion is intentionally not correlated withdependencyKindat the type level (context types are flat records). The resolver PR's throw sites will need to pick the right branch; worth a glance there. - The open question in the description about where instruction bump defaults live in v2 (
accountBumpValueNodecan't be a struct-field default anymore) is real and belongs to the resolver rewrite, not this PR.
9d5c372 to
34df2cc
Compare
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Re-fetched the diff and node-types at head. This revision supersedes my earlier pass and, importantly, corrects a gap in my own reasoning: I previously said struct-field defaults "can't hold contextual references" because StructFieldTypeNode.defaultValue is ValueNode | undefined. That's true on the surface, but InjectedValueNode is a member of StandaloneValueNode (hence ValueNode), and InstructionNode.provides → ProvidedNode.node: Node is a transparent pipe. So a data field can indeed reach a dataValueNode / accountValueNode / accountBumpValueNode via injectedValueNode.key → provides. The widened (InstructionAccountNode | StructFieldTypeNode)[] cycle and the structFieldTypeNode parent branch are the right model, and the "where do bump defaults live" question from the previous description is answered by the same mechanism.
What I verified this round:
InjectedValueNode ∈ StandaloneValueNode,InstructionNode.provides?: ProvidedNode[],ProvidedNode.node: Node— the inject/provide path described in the PR body exists innode-typesas-is.messages.ts: theCYCLIC_...rewording drops the now-inaccurate "accounts and arguments'" qualifier while keeping the message's meaning (per the "Don't change the meaning" note). TheINVALID_...template is untouched and still lines up with the context fields ($dependencyName/$dependencyKind/$parentName/$parentKind/$instructionName), so the description/code mismatch I flagged earlier is resolved — the unionparentKindwas the right call.- The
CamelCaseString→IdentifierStringsweep is unchanged from last time and still correct;PathStringandIdentifierStringboth exist inbrands.ts. - Typetest
numberTypeNode→integerTypeNodeis straightforward.
Notes for the resolver PR (not blocking here)
- A
StructFieldTypeNodeincycle/parentdoesn't carry its ownPathString—identifieris just the leaf segment. The resolver will need to thread the full path from theinstructionNode.datawalk to populateparentNameandformattedCyclefor nested fields (e.g.config.fees[0]). Inline note on thecycleline. - Since
datamay be adefinedTypeLinkNode, the resolver's graph walk has to follow links before it can enumerate struct fields, and lexical shadowing inprovides(sub-instructions) means the same field type can resolve differently per instruction. Both are resolver concerns; the error shapes here accommodate them fine.
| }; | ||
| [CODAMA_ERROR__VISITORS__CYCLIC_DEPENDENCY_DETECTED_WHEN_RESOLVING_INSTRUCTION_DEFAULT_VALUES]: { | ||
| cycle: (InstructionAccountNode | InstructionArgumentNode)[]; | ||
| cycle: (InstructionAccountNode | StructFieldTypeNode)[]; |
There was a problem hiding this comment.
Widening to StructFieldTypeNode checks out — InjectedValueNode is part of StandaloneValueNode, so a field default can reach contextual nodes through instructionNode.provides. (My earlier accounts-only reasoning missed that indirection.)
One thing for the resolver PR: a StructFieldTypeNode here only carries its leaf identifier, not the PathString relative to instructionNode.data. For nested fields the resolver will have to track the path itself when it builds formattedCycle / parentName, since the node in cycle won't be enough on its own to disambiguate e.g. two bump fields in different sub-structs.

Phase D, per-package adaptation (dependency order). Adapts
@codama/errorsto the v2 node model. Stacked on #1162.Changes
src/context.ts:CamelCaseString→IdentifierStringacross the ~42 error-context fields that hold node identifiers (v2 identifiers areIdentifierString, not casing-branded).instructionNode.data, and a data field can carry a contextual default via the inject/provide pattern:structFieldTypeNode.defaultValuemay be aninjectedValueNodewhose key is fulfilled byinstructionNode.provides→providedNode.node(a contextual value such asdataValueNode,accountValueNode,accountBumpValueNode, …). So a default-value dependency graph — and therefore a cycle — can involve accounts, data fields, or both.defaultValuelives onInstructionAccountNodeandStructFieldTypeNodeonly, so those are the participant types.CYCLIC_DEPENDENCY_DETECTED_WHEN_RESOLVING_INSTRUCTION_DEFAULT_VALUES:cycleretyped(InstructionAccountNode | InstructionArgumentNode)[]→(InstructionAccountNode | StructFieldTypeNode)[].INVALID_INSTRUCTION_DEFAULT_VALUE_DEPENDENCY:dependency→AccountValueNode | DataValueNode,dependencyKind→'accountValueNode' | 'dataValueNode'(dataValueNodereplaces the removedargumentValueNode);dependencyName→IdentifierString | PathString(adataValueNodecarries apath);parent→InstructionAccountNode | StructFieldTypeNode,parentKind→'instructionAccountNode' | 'structFieldTypeNode',parentName→IdentifierString | PathString.ArgumentValueNode,InstructionArgumentNode,CamelCaseString; addedDataValueNode,IdentifierString,PathString,StructFieldTypeNode.src/messages.ts:CYCLIC_...reworded to drop the participant qualifier (was "accounts and arguments' default values"), since a cycle may run through accounts, data, or both: "resolving the default values of the [$instructionName] instruction".INVALID_...keeps the generic$dependencyName/$dependencyKind/$parentName/$parentKind/$instructionNameform — no hardcoded participant kind.test/error.typetest.ts:'numberTypeNode'→'integerTypeNode'in example node-kind arrays (removed kind → v2 successor).Follow-ups for the visitors-core resolver PR
These two errors are thrown only by
getResolvedInstructionInputsVisitor(still v1, rewritten later in the stack). When adapted, its throw sites must build these v2 context shapes: an account input →InstructionAccountNode(parentName= identifier); a data-field input →StructFieldTypeNode(parentName= itsPathString);dependencyKind/Namefrom theaccountValueNode | dataValueNodeunion. The graph walksinstructionNode.provides+injectedValueNode+dataValueNode+ account defaults.The old v1
isPdadetection (which scanned instruction arguments for anaccountBumpValueNodedefault) maps onto this same model in v2: a data-field bump default isprovidedNode('bump', accountBumpValueNode(...))feeding astructFieldTypeNodeviainjectedValueNode.Verification
tscclean (src + test),pnpm buildsuccess, 27/27 tests pass, lint clean.