Writable: Fix support for polymorphic this and generics - #1516
deshrajvermay9517-png wants to merge 3 commits into
Conversation
Writable: Fix support for polymorphic this and generics
There was a problem hiding this comment.
September 7 update: This review describes the old head. My candidate sentinel check was too broad for key unions with undefined; the maintainer identified that missing case. The latest validated follow-up confirms the author fixed it using exact sentinel equality. The old candidate patch is superseded and should not be used.
The default-argument fix works, but please preserve an explicitly empty key selection before merging. I reproduced the inline regression against 5715dfe23dc5853a148afa37df3c0a698ca9cbb9 and this exact head, 08b29d21cf4391350dd73f0c94bc1f0b4927c582.
On each of TypeScript 5.9.3, 6.0.3 and 7.0.2, 175 independent checks cover every subset of four string/numeric/symbol keys, optional and mixed-readonly properties, index signatures, derived empty selections, collections and generic class/function use. Head repairs three of the six baseline failures, but introduces 12 readonly-assignment regressions when Keys resolves to never. The other three failures are unchanged explicit keyof this / keyof Item generic cases; the new concrete Writable<Foo, keyof Foo> test does not establish that generic variant.
I also tested a candidate that reserves undefined for the omitted argument (Keys extends keyof BaseType | undefined = undefined), branches on undefined extends Keys, and uses Extract<Keys, keyof BaseType> for Except. That preserves the three working fixes and removes all 12 new regressions across those compiler versions. Seven added negative assertions fail on this PR head and pass with the candidate. This is a possible implementation, with the explicit-keyof generic limitation still present; it is not a claim to have resolved that separate limitation.
Repository-wide test:tsc and test:tsd pass on both head and the candidate. The Node test runner passes 38 tests, and focused lint on the candidate's source/test files passes. Full npm test remains red on four XO errors in unchanged abstract-class.ts / readonly-deep.ts; all four reproduce on the exact base with the same installed dependencies and match the upstream failed job. I did not change those files.
Candidate source/test patch and the per-compiler results: apply-ready evidence.
AI-assisted review with independently executed compiler checks.
| // Make the specified keys writable. | ||
| & {-readonly [KeyType in keyof BaseType as KeyType extends Keys ? KeyType : never]: BaseType[KeyType]} | ||
| >; | ||
| : IsNever<Keys> extends true |
There was a problem hiding this comment.
[P2] Preserve readonly properties when the selected key set is empty
never is already a valid explicit key set meaning “make no properties writable”. Using it as the omitted-argument sentinel changes that to “make every property writable”. This also affects ordinary computed selections, not just callers spelling never directly:
type RecordData = {readonly id: string};
type EditableKeys = Extract<keyof RecordData, `editable${string}`>;
declare const data: Writable<RecordData, EditableKeys>;
data.id = 'changed'; // TS2540 on base; accepted on this headThe same regression removes readonly index-signature restrictions. Please distinguish the omitted argument from an empty selection and add negative assignment tests for both explicit never and a computed empty key set. Non-empty subsets and the new default Writable<this> behavior should remain covered.
There was a problem hiding this comment.
Good catch, thank you! I've updated the implementation to reserve undefined for the omitted argument (Keys extends keyof BaseType | undefined = undefined), and used Extract<Keys, keyof BaseType> when passing keys down.
Explicit never and computed key selections resolving to never now preserve readonly properties and index signatures. Added regression tests covering both explicit and computed never cases to test-d/writable.ts.
|
The overall direction makes sense. Using a sentinel for the omitted second argument lets that path use a direct homomorphic mapped type, which fixes I think the implementation should be smaller, thouh. Check the sentinel with The later One test also iss not exercising an empty computed selection. A string index signature has |
|
Updated as suggested:
All tests pass cleanly ( |
nrps9909
left a comment
There was a problem hiding this comment.
Rechecked a7ec8550c5989649bb1b3fd8d328663e18bb330d. My reported empty-selection regression is fixed: explicit never and computed empty selections preserve readonly properties/index signatures, while default Writable<this> / Writable<Item> still work.
Thanks for catching the broader sentinel test. My earlier candidate also used undefined extends Keys, so it had the same omission. I expanded the independent matrix to include every subset of four string/numeric/symbol keys unioned with undefined, using both all-readonly and mixed-readonly objects. On each of TypeScript 5.9.3, 6.0.3 and 7.0.2, the previous candidate has 42 additional readonly-assignment failures; the current exact-sentinel implementation fixes all 42. The full expanded set gives 300/303 expected results. The only three failures are the inherited explicit-keyof generic forms, now correctly excluded from this PR's claims. I am marking my old candidate artifact as superseded, with this missing coverage disclosed.
Repository-wide test:tsc and test:tsd, the 38 Node tests, and focused source/test lint pass. Full npm test still reports the same four XO errors in unchanged abstract-class.ts / readonly-deep.ts, matching the exact-base run and the current CI log; I am not claiming full CI is green.
No remaining objection from my reported regression. This follow-up is scoped to the recorded head and leaves the broader generic limitation separate. AI-assisted validation with independently executed compiler checks.
Fixes #1515.
What was wrong
In #1470 (v5.9.0),
Writablewas updated to preserve index signatures by using anasclause to filter keys:When
BaseTypeis an uninstantiated type variable (such as polymorphicthisinside a class method ortypeof this, or genericT), TypeScript cannot eagerly evaluate the key-filtering conditional in theasclause. As a result, properties onWritable<this>and uninstantiatedWritable<T>were dropped, causing errors such as:What changed
Keysargument usesundefinedas the sentinel (Keys extends keyof BaseType | undefined = undefined), checked withIsEqual<Keys, undefined> extends true.IsEqual<Keys, undefined>ensures unions containingundefined(such asWritable<Model, 'selected' | undefined>) only make the selected keys writable rather than treating the argument as omitted.Keysis omitted (resolving toundefined),Writableevaluates directly to{-readonly [KeyType in keyof BaseType]: BaseType[KeyType]}. This direct homomorphic mapping:this,typeof this, and genericWritable<T>without dropping properties.Writable: Fix behavior with index signatures #1470).neveror computed key selections that resolve toneverremain valid empty key selections and preserve readonly properties and index signatures.Extract<Keys, keyof BaseType>is used when forwarding selected keys toExcept.Keysis specified (e.g.Writable<T, 'a'>orWritable<Foo, keyof Foo>), it continues through theExcept+ filtered mapped type path as expected.How it was tested
test-d/writable.tscovering:(this as Writable<this>).field = 4and(this as Writable<typeof this>).field = 4inside class methods.Writable<T>(testGeneric<T>(item: Writable<T>)).undefinedlikeWritable<Foo, 'a' | undefined>, verifying non-selected properties remain readonly.Writable<T, never>with negative assignment tests.never(Writable<RecordData, EditableKeys>) with negative assignment tests.neveror computed empty selection (Extract<keyof IndexRecord, symbol>) is passed.keyof BaseTypeasKeysargument (Writable<Foo, keyof Foo>).any,never,unknown).test-d/writable.tspass cleanly withtsd.tsc), tests (node --test), and linter (xoon changed files).