Skip to content

feat(js): service-scoped type resolution + generic substitution on TypeResolver#1323

Open
ukint-vs wants to merge 5 commits intomasterfrom
feat/sails-js-type-resolver-scope
Open

feat(js): service-scoped type resolution + generic substitution on TypeResolver#1323
ukint-vs wants to merge 5 commits intomasterfrom
feat/sails-js-type-resolver-scope

Conversation

@ukint-vs
Copy link
Copy Markdown
Member

Closes #1317.

Summary

Adds three public methods on TypeResolver and a convenience accessor on SailsProgram so downstream consumers (wallets, explorers, schema tooling, custom codegen) stop re-implementing type-scope and generic-substitution out-of-band.

New API

class TypeResolver {
  // Constructor: ambient (program-level) types visible to every service,
  // shadowed by service-local `types` on name collision.
  constructor(types: Type[], ambientTypes?: Type[]);

  // Look up a named user Type from this resolver's scope.
  resolveNamed(typeDecl: TypeDecl): Type | undefined;

  // Recursively substitute type parameters through a TypeDecl tree.
  // Pure, idempotent, same-ref return on unchanged subtrees.
  // Throws on cyclic substitution maps ({T: T} or {T: U, U: T}).
  substituteGenerics(typeDecl: TypeDecl, substitutions?: Record<string, TypeDecl>): TypeDecl;

  // Zip user type's type_params with a concrete generics list.
  genericsSubstitutions(userType: Type, generics?: TypeDecl[]): Record<string, TypeDecl>;
}

class SailsProgram {
  // Direct AST lookup — safe to call in tight walker loops.
  resolveInService(serviceName: string, typeDecl: TypeDecl): Type | undefined;
}

SailsProgram now threads program.types into every SailsService (including the extends chain) as ambient types, so per-service TypeResolver instances see program-level types with service-local shadowing.

getTypeDeclString was also updated to call the new genericsSubstitutions helper instead of the inline copy at lines 229-233.

Why

Two correctness bugs in consumer code before this change:

  1. Name collisions across services. Two services each declaring Packet with different shapes silently collided when a consumer flattened types into a single map. No public API narrowed a lookup to a single service's scope.
  2. Generic substitution. Envelope<[u8]>'s payload field came through as a bare {kind:'named', name:'T'} leaf, causing walkers to silently pass hex strings through untouched or SCALE-encode with wrong bytes. The substitution algorithm already lived inside getTypeDeclString — this PR factors it out and exposes it.

vara-wallet had ~180 lines of workaround (coerceHexToBytesV2, getRegistryTypes, ad-hoc substitution recursion) that can now collapse to a few method calls.

Tests

21 new tests, 46 total passing in the resolver + parser-resolver suites (64 total offline).

Unit (js/test/idl-v2-type-resolver.test.ts):

  • substituteGenerics: bare-param substitution, recursion through slice/array/tuple/named-with-generics, pass-through for unknown names, idempotence, non-mutation, chain resolution (T → U → u32), self-referential throw, cyclic chain throw, unknown-kind throw.
  • resolveNamed: returns user Type for named decl (including generic), returns undefined for primitives/slices/arrays/tuples/unknown names/bare type_params.
  • genericsSubstitutions: zip, no type_params, extra generics truncated.
  • Ambient types: unshadowed lookup, local-shadows-ambient, registry reflects shadowing (collision test encodes [u8; 8] after overriding an [u8; 4] ambient shape).

Integration (js/test/idl-v2-parser-type-resolver.test.ts):

  • Two services each declaring Packet with different shapes → resolveInService('A', ...) vs 'B' return their own definitions.
  • Unknown service name → undefined.
  • Program-level Shared visible via service resolver (ctor arg path; the parser rejects program-level references from service signatures).
  • Envelope<[u8]> → walker feeds struct fields through substituteGenerics + genericsSubstitutions to resolve payload from T to [u8].

Notes for reviewers

  • substituteGenerics has a runtime cycle guard (visited set, throws with the full chain on detection) rather than just the JSDoc warning. Maps produced by genericsSubstitutions from parsed IDL can't produce cycles, but hand-merged or wire-sourced maps can.
  • SailsProgram.resolveInService walks the AST directly rather than going through this.services (which rebuilds SailsService + a fresh TypeRegistry on every access). Safe for hot walker loops.
  • substituteGenerics returns the same reference for subtrees that contained no substitutions — walkers traversing large trees with sparse type parameters don't allocate new nodes unnecessarily.
  • _substituteGenerics uses immutable visited-set copies on the recursion path so a throw mid-recursion leaves no poisoned shared state.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces service-scoped type resolution and generic type substitution for the Sails IDL v2 implementation. Key enhancements include the addition of resolveInService to SailsProgram for scoped type lookups and the introduction of ambientTypes in SailsService to support program-level types. The TypeResolver has been significantly updated with logic for shadowing ambient types, recursive generic substitution with cycle detection, and helper methods for mapping type parameters. Feedback was provided regarding the performance of resolveInService, suggesting the use of Maps for $O(1)$ lookups to prevent bottlenecks in large IDLs.

Comment thread js/src/sails-idl-v2.ts
ukint-vs added a commit that referenced this pull request Apr 23, 2026
- Build a lazy `Map<serviceName, Map<typeName, Type>>` index in
  `SailsProgram` on first `resolveInService` call. O(1) per call after
  that, no cost for consumers who never use the method. Addresses the
  gemini-code-assist comment on sails-idl-v2.ts:242.
- Swap `JSON.parse(JSON.stringify(input))` in the "does not mutate"
  test for `structuredClone(input)` to satisfy `unicorn/prefer-structured-clone`
  (fixes the lint job failure on the v1.6.1.0 JS CI).
…peResolver

Closes #1317.

Adds three public methods on `TypeResolver` and a convenience accessor on
`SailsProgram` so downstream consumers (wallets, explorers, custom codegen)
stop re-implementing type-scope + generic-substitution out-of-band.

- `TypeResolver.resolveNamed(typeDecl)` — look up a named user `Type` from
  the resolver's scope, honoring ambient-vs-local shadowing.
- `TypeResolver.substituteGenerics(typeDecl, substitutions)` — pure,
  idempotent tree walk that substitutes bare `{kind:'named', name:'T'}`
  leaves through chains (`{T: U, U: u32}` resolves `T` to `u32`). Runtime
  cycle guard on self-referential or cyclic maps. Same-reference return
  on unchanged subtrees avoids pointless allocation in walkers.
- `TypeResolver.genericsSubstitutions(userType, generics)` — zip a user
  type's `type_params` with a concrete generics list into a sub map.
  `getTypeDeclString` now uses this helper instead of an inline copy.
- `TypeResolver` constructor gained an `ambientTypes` parameter;
  `SailsProgram` threads `program.types` into every `SailsService`
  (including the `extends` chain) so service-local types shadow
  program-level ambients inside each per-service resolver.
- `SailsProgram.resolveInService(serviceName, typeDecl)` — direct AST
  lookup that skips `SailsService`/`TypeResolver` construction, safe to
  call in tight walker loops.

Tests: 21 new (17 unit + 4 parser-driven integration) covering all five
`TypeDecl` variants, shadowing with registry-level collision, chain
resolution, cycle detection, unknown-kind rejection, and service-scoped
collision across two services declaring the same type name with
incompatible shapes.
- Build a lazy `Map<serviceName, Map<typeName, Type>>` index in
  `SailsProgram` on first `resolveInService` call. O(1) per call after
  that, no cost for consumers who never use the method. Addresses the
  gemini-code-assist comment on sails-idl-v2.ts:242.
- Swap `JSON.parse(JSON.stringify(input))` in the "does not mutate"
  test for `structuredClone(input)` to satisfy `unicorn/prefer-structured-clone`
  (fixes the lint job failure on the v1.6.1.0 JS CI).
…cl::Generic

Per maintainer feedback on #1323:

- `TypeResolver` constructor reverts to `(types: Type[])`. Callers that want
  ambient-vs-local shadowing pass `[...ambientTypes, ...localTypes]` — the
  merge is the caller's concern, not the resolver's. Moved the merge to
  `SailsService`, which is the only caller that needs it.
- `substituteGenerics` and `resolveNamed` now target the explicit
  `TypeDecl::Generic` AST variant (landed in #1314). A bare `{kind:'named'}`
  decl is always a concrete user-type reference and is no longer treated as
  a type-parameter fallback; substitution only fires on `{kind:'generic'}`
  leaves. Cycle detection, chain resolution, and same-reference returns on
  unchanged subtrees all unchanged.
- Tests updated to use `{kind:'generic', name:'T'}` for type parameters.
  Ambient-types describe block renamed to "last-write-wins merge (shadowing
  via call-site merge)" to reflect the new API shape.
@ukint-vs ukint-vs force-pushed the feat/sails-js-type-resolver-scope branch from 8155c74 to dd74a92 Compare April 23, 2026 14:32
@ukint-vs
Copy link
Copy Markdown
Member Author

Rebased on master and addressed maintainer feedback:

  1. TypeResolver constructor reverted to (types: Type[]). [...ambientTypes, ...localTypes] merging now happens at the call site in SailsService. The resolver no longer knows about the ambient-vs-local distinction; callers pass a single pre-merged array and rely on last-write-wins semantics for shadowing.
  2. Adapted to TypeDecl::Generic (from feat: Migrate type registry to IDL AST, add explicit generic type declarations #1314). substituteGenerics now targets the explicit {kind:'generic'} variant for type-parameter leaves; a bare {kind:'named'} decl is always a concrete user-type reference and is no longer used as a type-param fallback. resolveNamed returns undefined for kind:'generic'. Tests updated accordingly.

Cycle detection, chain resolution (T → U → u32), and same-reference returns on unchanged subtrees are preserved. 64/64 offline tests + lint green locally.

Graph review surfaced SailsService.extends as a test gap — it propagates
ambient types to child services at sails-idl-v2.ts:612, but no offline
test exercised that path. Adds a parser-driven test: Child extends Base,
program declares Shared, verify baseThroughExtends.typeResolver resolves
Shared through the propagated ambient types.
Comment thread js/src/type-resolver-idl-v2.ts
Comment thread js/src/type-resolver-idl-v2.ts Outdated
Comment thread js/src/type-resolver-idl-v2.ts Outdated
…solveGenerics

- Rename `substituteGenerics` → `resolveGenerics`.
- Hide `genericsSubstitutions` (now `_genericsSubstitutions`). Consumers no longer
  manually zip `type_params` with a `TypeDecl.generics` list — that's resolver logic.
- Extend `resolveNamed` with a second overload `(name: string, generics?: TypeDecl[])`.
  When concrete generics are provided (via either overload or via `typeDecl.generics`),
  the result is a concrete substituted `Type` with `type_params` stripped:
    `Envelope<u32>` → `{ kind: 'struct', fields: [{id, u32}, {payload, u32}] }`.
  Without generics, the raw user `Type` is returned unchanged.

Per vobradovich feedback on #1323.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sails-js v2: TypeResolver needs service-scoped lookup + generic substitution for downstream walkers

2 participants