-
Notifications
You must be signed in to change notification settings - Fork 5
Fix create
#134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix create
#134
Changes from all commits
93e4674
9d4281e
ed07aec
3d5a189
1740ef4
d9fea4f
734256d
ac8152b
636322f
3899581
b0293b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| /** | ||
| * Type-level regression guard for the static `create` / `_create` contract. | ||
| * | ||
| * `yarn types` compiles `src/**` but NOT `test/**` (the sample simulators import | ||
| * generated, gitignored contract artifacts), and `vitest` strips types without | ||
| * checking them — so a regression in the `create` / `_create` typing would | ||
| * otherwise pass CI. This file reproduces the subclass-override pattern against a | ||
| * synthetic contract type, so `yarn types` fails on a regression. | ||
| * | ||
| * It exports nothing and is imported by nothing (inert at runtime). The build | ||
| * config (`tsconfig.json`) excludes `*.type-test.ts` from emit, so it never | ||
| * reaches `dist`; the type check runs it via `tsconfig.types.json`. | ||
| * | ||
| * Typechecking the real test simulators (which need the generated artifacts) is a | ||
| * separate, larger effort; this is the minimal guard for the contract that | ||
| * actually regressed. | ||
| */ | ||
| import { createSimulator } from './factory/createSimulator.js'; | ||
| import type { SimulatorConfig } from './factory/SimulatorConfig.js'; | ||
| import type { IMinimalContract } from './types/Contract.js'; | ||
|
|
||
| type GuardPrivateState = { readonly value: number }; | ||
| type GuardArgs = readonly [a: number, b: string]; | ||
|
|
||
| // A synthetic config — never executed, only used to instantiate the factory's | ||
| // generics for the type-level checks below. | ||
| const config = {} as unknown as SimulatorConfig< | ||
| GuardPrivateState, | ||
| unknown, | ||
| unknown, | ||
| IMinimalContract, | ||
| GuardArgs | ||
| >; | ||
|
|
||
| class Guard extends createSimulator(config) { | ||
| // A concrete `Promise<Guard>` return must stay assignable to the base static | ||
| // side. If `create`'s return goes generic again, this fails the static-side | ||
| // `extends` check (TS2417). | ||
| static async create(a: number, b: string, options = {}): Promise<Guard> { | ||
| // biome-ignore lint/complexity/noThisInStatic: super._create must keep the subclass `this` | ||
| return super._create([a, b], options) as Promise<Guard>; | ||
| } | ||
|
|
||
| // `_create` must type its args tuple against `GuardArgs`. If that check | ||
| // regresses (e.g. `_create` widens to `...args: unknown[]`), the wrong-arity | ||
| // call below stops erroring and the `@ts-expect-error` becomes unused → CI red. | ||
| static async _argCheck(): Promise<unknown> { | ||
| // Call via the class name (not `super`) so this negative assertion needs | ||
| // only the `@ts-expect-error` below — no `noThisInStatic` ignore to stack. | ||
| // @ts-expect-error a 1-element tuple must not satisfy `[number, string]`. | ||
| return Guard._create([1], {}); | ||
| } | ||
|
|
||
| // A subclass-only member. Without it, `Guard`'s instance type would equal the | ||
| // base `Simulator`'s and the call-site check below would pass even if `create` | ||
| // stopped narrowing to the subclass. This mirrors real subclasses, which add | ||
| // circuit methods that a base-typed return would hide. | ||
| public marker(): string { | ||
| return 'guard'; | ||
| } | ||
| } | ||
|
|
||
| // The override must narrow the call-site return to the subclass, not widen to | ||
| // the base `Simulator` (which lacks `marker`). | ||
| async function _callSiteSubtype(): Promise<void> { | ||
| const instance: Guard = await Guard.create(1, 'x'); | ||
| void instance.marker(); | ||
| } | ||
|
|
||
| void Guard; | ||
| void _callSiteSubtype; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -191,21 +191,52 @@ export function createSimulator< | |
| } | ||
|
|
||
| /** | ||
| * Constructs a simulator. In dry, deploys from `contractArgs` to fresh | ||
| * in-memory state. In live, the caller already deployed; the args seed only | ||
| * the local pure-eval context, never an on-chain deploy. | ||
| * Public entry point. Permissive params so subclasses can override `create` | ||
| * with contract-specific signatures without tripping TS's static-side | ||
| * `extends` check. Subclass overrides should assemble the typed args tuple | ||
| * and call {@link _create} (`super._create([...args], options)`) so the | ||
| * tuple is checked against `TArgs` — delegating back to this permissive | ||
| * `create` would silently skip that check. | ||
| * | ||
| * @param args - `[contractArgs?, options?]`, validated by `_create`. | ||
| * @returns The constructed simulator, typed as the base `Simulator`; subclass | ||
| * `create` overrides narrow the return to their own type. | ||
| */ | ||
| static async create( | ||
| this: new ( | ||
| deps: BackendDeps<P, L>, | ||
| ) => Simulator, | ||
| ...args: unknown[] | ||
| ): Promise<Simulator> { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 followup — a subclass that does not override class NoOverride extends Base {
bump() { return this.circuits.impure.setVal(1n); }
}
await (await NoOverride.create([1n])).bump();
// this PR: TS2339: Property 'bump' does not exist on type '...Simulator'
// main: fine, `T` was inferred from `this`Overriding added by claude (dev3-code-review)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indirectly addressed above. Improved readme doc d9fea4f |
||
| const [contractArgs, options] = args as [TArgs?, SimulatorOptions<P, W>?]; | ||
| // biome-ignore lint/complexity/noThisInStatic: keep the caller's `this` so a non-overriding subclass constructs its own type, not the base `Simulator` (the autofix rewrites `this`->`Simulator`, breaking that at runtime). | ||
| return (this as unknown as typeof Simulator)._create( | ||
| contractArgs, | ||
| options, | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Typed construction path. In dry, deploys from `contractArgs` to fresh | ||
| * in-memory state; in live the caller already deployed and the args seed | ||
| * only the local pure-eval context. Subclass `create` overrides call this | ||
| * (`super._create([...typedArgs], options)`) so their args tuple is checked | ||
| * against `TArgs`. Underscore-public to match the `_backend`/`_signers` | ||
| * convention for declaration emit. | ||
| * | ||
| * @param contractArgs - Constructor args for the contract. | ||
| * @param options - Backend selection, witnesses, private state, live world. | ||
| * @returns The constructed simulator (subclass-aware via `this`). | ||
| * @returns The constructed simulator, typed as the base `Simulator`; subclass | ||
| * `create` overrides narrow the return to their own type. The runtime | ||
| * instance is the caller's class (constructed via `this`). | ||
| */ | ||
| static async create<T extends Simulator>( | ||
| static async _create( | ||
| this: new ( | ||
| deps: BackendDeps<P, L>, | ||
| ) => T, | ||
| ) => Simulator, | ||
| contractArgs: TArgs = [] as unknown as TArgs, | ||
| options: SimulatorOptions<P, W> = {}, | ||
| ): Promise<T> { | ||
| ): Promise<Simulator> { | ||
| const deps = await prepareBackend(contractArgs, options); | ||
| return new this(deps); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| "exclude": [ | ||
| "node_modules", | ||
| "dist", | ||
| "tests" | ||
| "tests", | ||
| "src/**/*.type-test.ts" | ||
| ] | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| { | ||
| // Typecheck-only project used by `yarn types`. | ||
| // | ||
| // Identical to the build config (`tsconfig.json`) except it (a) never emits and | ||
| // (b) re-includes the `*.type-test.ts` regression guards, which the build config | ||
| // excludes from emit so they don't ship in `dist`. Keeping them in the type | ||
| // check is the whole point: they fail CI if the guarded contracts regress. | ||
| "extends": "./tsconfig.json", | ||
| "compilerOptions": { | ||
| "noEmit": true | ||
| }, | ||
| "include": ["src/**/*"], | ||
| "exclude": ["node_modules", "dist", "tests"] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 followup —
super.create(tuple, options)is no longer checked againstTArgs.Every subclass has exactly one call site that assembles the tuple, and it is now unchecked:
main:TS2345: Source has 1 element(s) but target requires 2config.contractArgs(...contractArgs)Direct calls on the base class lose the same protection.
Base.create({ backend: 'dry' })and five-argument calls both type-check now.mainrejected both.A typed sibling keeps override compatibility and the arg checking. I compiled this against all three sample simulators (clean), and the bad call above errors again:
Subclasses then call
super._create([owner, salt], options). Underscore-public matches the existing_backend/_signersconvention for declaration emit. Purely additive, so it can also land after this PR.added by claude (dev3-code-review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is better 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1740ef4