Skip to content

remove recursive type checks from Kysely and subtypes - #1963

Open
koskimas wants to merge 8 commits into
speed-up-transaction-type-checksfrom
remove-recursive-type-checks-from-kysely-class
Open

remove recursive type checks from Kysely and subtypes#1963
koskimas wants to merge 8 commits into
speed-up-transaction-type-checksfrom
remove-recursive-type-checks-from-kysely-class

Conversation

@koskimas

@koskimas koskimas commented Jul 31, 2026

Copy link
Copy Markdown
Member

Remove Kysely assignment type check recursion. This one comes with a rather big caveat, that we might not want:

let k1: Kysely<{ a: A, b: B }>
let k2: Kysely<{ a: A }>
k2 = k1 // This no longer works!

Opus says it only worked so far because typescript gave up with the infinite recursion. If it was able to follow it through, the result would've been an error:

On the assignability change you approved: Kysely<{a,b}> ↔ Kysely<{a}> no longer compiles in either direction. Worth knowing why it used to: the compiler didn't trust the variance it measured and fell back to a structural comparison, where methods match bivariantly. It was never a variance the library could have declared — I confirmed TypeScript rejects out DB with TS2636 because selectFrom takes keyof DB. Users who relied on it need $pickTables/$omitTables.

So do we want to keep that feature, which only works because typescript gives up over infinite recursion (assuming Opus is right)?

This is on top of #1962

@koskimas
koskimas requested a review from igalklebanov July 31, 2026 09:20
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
kysely Skipped Skipped Aug 2, 2026 6:06am

Request Review

@pkg-pr-new

pkg-pr-new Bot commented Jul 31, 2026

Copy link
Copy Markdown

commit: 594062b

// intersection, the second of these cost 1.3M instantiations on TypeScript 7.
bench('ReadonlyKysely assignable to ReadonlyKysely', () => {
return acceptsReadonlyKysely(readonlyKysely)
}).types([26, 'instantiations'])

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This used to cost 1.3M instantations 😬

@github-actions

Copy link
Copy Markdown

⏱️ TypeScript Benchmark Results

✅ ⏱️  No benchmark changes detected.

Comment thread src/kysely.ts
// wider builder stand in for a narrower one, which is bivariance, and bivariance
// can't be spelled as an annotation. See `test/typings/test-d/generic.test-d.ts`
// for what keeps those in check instead.
export class Kysely<in out DB>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Rather excessive AI babbling... Do we want this here at all?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably not.

@igalklebanov igalklebanov Aug 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Its better if this stuff is kept in PR descriptions - agents love getting context from gh api - when necessary.

@koskimas
koskimas marked this pull request as ready for review July 31, 2026 12:36
Comment thread src/readonly/readonly-query-creator.ts Outdated
Comment thread src/readonly/readonly-kysely.ts Outdated
bench('Kysely assignable to Kysely', () => {
return acceptsKysely(plainKysely)
}).types([11784, 'instantiations'])
}).types([2, 'instantiations'])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

amazing!

Comment thread test/ts-benchmarks/generic.bench.ts Outdated
Comment thread src/readonly/readonly-kysely.ts Outdated
Comment thread src/kysely.ts Outdated
Comment thread src/kysely.ts Outdated
Comment thread test/ts-benchmarks/generic.bench.ts Outdated
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.

2 participants