Repository navigation
perf(types): match object schemas structurally in object methods - #1676
fabian-hiller wants to merge 3 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 39 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
commit: |
commit: |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No blocking issues were identified; only minor test-coverage nits remain.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Refactors object-method result types to use structural schema matching, reducing TypeScript variance-analysis overhead while preserving inferred types.
Changes:
- Updated
partial,required,pick, andomittype branches. - Preserved sync/async and object-with-rest behavior.
- Documented the performance improvement.
| File | Summary |
|---|---|
library/src/methods/required/requiredAsync.ts |
Structural async matching |
library/src/methods/required/required.ts |
Structural sync matching |
library/src/methods/pick/pick.ts |
Structural matching for pick branches |
library/src/methods/partial/partialAsync.ts |
Structural async matching |
library/src/methods/partial/partial.ts |
Structural sync matching |
library/src/methods/omit/omit.ts |
Structural matching for omit branches |
library/CHANGELOG.md |
Documents the performance improvement |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| : TSchema extends { | ||
| readonly type: 'object' | 'strict_object'; | ||
| readonly async: true; | ||
| readonly entries: infer TEntries extends ObjectEntriesAsync; | ||
| } |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="library/src/methods/omit/omit.ts">
<violation number="1" location="library/src/methods/omit/omit.ts:134">
P2: Add type assertions for async, loose, and strict schemas in `pick.test-d.ts`, and cover the corresponding omit cases so misclassified structural branches fail the type tests.</violation>
<violation number="2" location="library/src/methods/omit/omit.ts:324">
P3: The async `object_with_rest` branch keeps the sync-only `BaseSchema` rest requirement, so `omit(objectWithRestAsync(entries, anyAsyncSchema), keys)` — where `objectWithRestAsync` accepts `BaseSchema | BaseSchemaAsync` rests (see `objectWithRestAsync.ts` and the local `Schema` union) — matches no branch and resolves to the `: never` fallback, even though the runtime `omit` implementation never touches `rest` and works fine. `partialAsync`/`requiredAsync` in this same batch accept `BaseSchema | BaseSchemaAsync` for the identical branch, so this is the only async object schema that omitting/picking rejects. This was also broken before the change (the old `ObjectWithRestSchemaAsync<…, BaseSchema<…>, …>` pattern had the same gap), so it's not a regression, but while re-writing these infer sites it's a one-line alignment with the async variants: allow `BaseSchema | BaseSchemaAsync` in the rest requirement. Consider a type test like `expectNever`/`expectTypeOf` for `omit(objectWithRestAsync(entries, stringAsync()), ['key'])` since none currently covers the async-rest case.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| > | ||
| : TSchema extends { | ||
| readonly type: 'object' | 'strict_object'; | ||
| readonly async: true; |
There was a problem hiding this comment.
P2: Add type assertions for async, loose, and strict schemas in pick.test-d.ts, and cover the corresponding omit cases so misclassified structural branches fail the type tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At library/src/methods/omit/omit.ts, line 134:
<comment>Add type assertions for async, loose, and strict schemas in `pick.test-d.ts`, and cover the corresponding omit cases so misclassified structural branches fail the type tests.</comment>
<file context>
@@ -125,15 +129,11 @@ export type SchemaWithOmit<
- >
+ : TSchema extends {
+ readonly type: 'object' | 'strict_object';
+ readonly async: true;
+ readonly entries: infer TEntries extends ObjectEntriesAsync;
+ }
</file context>
| readonly type: 'object_with_rest'; | ||
| readonly async: false; | ||
| readonly entries: infer TEntries extends ObjectEntries; | ||
| readonly rest: BaseSchema<unknown, unknown, BaseIssue<unknown>>; |
There was a problem hiding this comment.
P3: The async object_with_rest branch keeps the sync-only BaseSchema rest requirement, so omit(objectWithRestAsync(entries, anyAsyncSchema), keys) — where objectWithRestAsync accepts BaseSchema | BaseSchemaAsync rests (see objectWithRestAsync.ts and the local Schema union) — matches no branch and resolves to the : never fallback, even though the runtime omit implementation never touches rest and works fine. partialAsync/requiredAsync in this same batch accept BaseSchema | BaseSchemaAsync for the identical branch, so this is the only async object schema that omitting/picking rejects. This was also broken before the change (the old ObjectWithRestSchemaAsync<…, BaseSchema<…>, …> pattern had the same gap), so it's not a regression, but while re-writing these infer sites it's a one-line alignment with the async variants: allow BaseSchema | BaseSchemaAsync in the rest requirement. Consider a type test like expectNever/expectTypeOf for omit(objectWithRestAsync(entries, stringAsync()), ['key']) since none currently covers the async-rest case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At library/src/methods/omit/omit.ts, line 324:
<comment>The async `object_with_rest` branch keeps the sync-only `BaseSchema` rest requirement, so `omit(objectWithRestAsync(entries, anyAsyncSchema), keys)` — where `objectWithRestAsync` accepts `BaseSchema | BaseSchemaAsync` rests (see `objectWithRestAsync.ts` and the local `Schema` union) — matches no branch and resolves to the `: never` fallback, even though the runtime `omit` implementation never touches `rest` and works fine. `partialAsync`/`requiredAsync` in this same batch accept `BaseSchema | BaseSchemaAsync` for the identical branch, so this is the only async object schema that omitting/picking rejects. This was also broken before the change (the old `ObjectWithRestSchemaAsync<…, BaseSchema<…>, …>` pattern had the same gap), so it's not a regression, but while re-writing these infer sites it's a one-line alignment with the async variants: allow `BaseSchema | BaseSchemaAsync` in the rest requirement. Consider a type test like `expectNever`/`expectTypeOf` for `omit(objectWithRestAsync(entries, stringAsync()), ['key'])` since none currently covers the async-rest case.</comment>
<file context>
@@ -315,11 +317,12 @@ export type SchemaWithOmit<
+ readonly type: 'object_with_rest';
+ readonly async: false;
+ readonly entries: infer TEntries extends ObjectEntries;
+ readonly rest: BaseSchema<unknown, unknown, BaseIssue<unknown>>;
+ }
? Omit<TSchema, 'entries' | '~standard' | '~run' | '~types'> & {
</file context>
| readonly rest: BaseSchema<unknown, unknown, BaseIssue<unknown>>; | |
| readonly rest: | |
| | BaseSchema<unknown, unknown, BaseIssue<unknown>> | |
| | BaseSchemaAsync<unknown, unknown, BaseIssue<unknown>>; |
Part of #1663 (finding 11, part 1). Independent of the other PRs from that investigation.
Problem
The result types of
partial,partialAsync,required,requiredAsync,pickandomitpick their branch withTSchema extends ObjectSchema<infer TEntries, ErrorMessage<ObjectIssue> | undefined> | StrictObjectSchema<infer TEntries, …>(and likewise for loose objects and objects with rest, sync and async). Inferring from and relating to another instantiation of the same interface forces TypeScript to measure the variance of each object schema interface.ObjectSchema's measured variance is "invariant | unreliable", so TS still falls back to a structural comparison afterwards.Change
All 20
infersites now match structurally on the discriminants plus the inferred properties:TSchemais already constrained to Valibot object schemas without a pipe, sotype+asyncselect exactly the same branch as before. Thepick/omitobject-with-rest branches keep their existingrest: BaseSchema<…>requirement unchanged, including in the async branch.Measurements
TypeScript 6.0.3, schema-benchmarks probe (schema declaration):
partial,pick,omit,required(partial(…))on a sync objectobjectAsync, plusobjectWithRest,strictObject,looseObjectBehavior
The displayed
InferInput/InferOutputtypes of both cases are byte-identical, and all runtime and type tests pass.Follow-up
The
Schemaconstraints of these methods (andObjectKeys) still referenceObjectSchema<ObjectEntries, …>, which still triggers the variance measurement. Making them structural as well cuts the two cases above further, to 13,470 and 21,877. It loosens the public constraints slightly, so that will be a separate PR stacked on this one.🤖 Generated with Claude Code