Skip to content
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

Fix multiple issues with indexed access types applied to mapped types #47109

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Dec 10, 2021

This PR fixes multiple issues related to indexed access types applied to mapped types. The fixes enable most of the problems outlined in #30581 to be solved using an approach that involves "distributive object types" created using indexed access types applied to mapped types.

First, a summary of the issue we're trying to solve:

type UnionRecord = 
    | { kind: "n", v: number, f: (v: number) => void }
    | { kind: "s", v: string, f: (v: string) => void }
    | { kind: "b", v: boolean, f: (v: boolean) => void };

function processRecord(rec: UnionRecord) {
    rec.f(rec.v);  // Error, 'string | number | boolean' not assignable to 'never'
}

The UnionRecord type is a union of records with two correlated properties, v and f, where the type of v is always the same as the type of f's parameter. In the processRecord function we're attempting to take advantage of the correlation by calling rec.f(rec.v) for some record. By casual inspection this seems perfectly safe, but the type checker doesn't see it that way. From its viewpoint, the type of rec.v is string | number | boolean and the type of rec.f is (v: never) => void, the never originating from the intersection string & number & boolean. The type is basically trying to check if a v from some record can be passed as an argument to an f from some record--but not necessarily the same record.

In the following I will outline an approach to writing types for this kind of pattern. Key to the approach is that the union types in question contain a discriminant property with a type that can itself serve as a property name (e.g. a string literal type or a unique symbol type). Note that the examples below only compile successfully with the fixes in this PR.

The following formulation of the UnionRecord type encodes the correspondence between kinds and types and the correlated properties:

type RecordMap = { n: number, s: string, b: boolean };
type RecordType<K extends keyof RecordMap> = { kind: K, v: RecordMap[K], f: (v: RecordMap[K]) => void };
type UnionRecord = RecordType<'n'> | RecordType<'s'> | RecordType<'b'>;

The manual step of creating a union of RecordType<K> for each key in RecordMap can instead be automated:

type UnionRecord = { [P in keyof RecordMap]: RecordType<P> }[keyof RecordMap];

The pattern of applying an indexed access type to a mapped type is effectively a device for distributing a type (in this case RecordType<P>) over a union (in this case keyof RecordMap). We can even go one step further and allow a UnionRecord to be created for any arbitrary subset of keys:

type UnionRecord<K extends keyof RecordMap = keyof RecordMap> = { [P in K]: RecordType<P> }[K];

We can then merge RecordType<K> into UnionRecord<K>, which removes the possibility of creating non-distributed records. This leaves us with the final formulation:

type RecordMap = { n: number, s: string, b: boolean };
type UnionRecord<K extends keyof RecordMap = keyof RecordMap> = { [P in K]: {
    kind: P,
    v: RecordMap[P],
    f: (v: RecordMap[P]) => void
}}[K];

And, using this formulation, we can now write types for the processRecord function that reflect the correlation and work for singletons as well as unions:

function processRecord<K extends keyof RecordMap>(rec: UnionRecord<K>) {
    rec.f(rec.v);  // Ok
}

declare const r1: UnionRecord<'n'>;  // { kind: 'n', v: number, f: (v: number) => void }
declare const r2: UnionRecord;  // { kind: 'n', ... } | { kind: 's', ... } | { kind: 'b', ... }

processRecord(r1);
processRecord(r2);
processRecord({ kind: 'n', v: 42, f: v => v.toExponential() });

And, because everything is expressed in terms of the mapping in RecordMap, new kinds and data types can be added in a single place, nicely conforming to the DRY principle.

Fixes #30581.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Dec 10, 2021
@ahejlsberg
Copy link
Member Author

@typescript-bot perf test faster
@typescript-bot test this
@typescript-bot user test inline
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 10, 2021

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at d42e4c2. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 10, 2021

Heya @ahejlsberg, I've started to run the extended test suite on this PR at d42e4c2. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 10, 2021

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at d42e4c2. You can monitor the build here.

@ahejlsberg
Copy link
Member Author

@typescript-bot user test this inline

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 10, 2021

Heya @ahejlsberg, I've started to run the inline community code test suite on this PR at d42e4c2. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..47109

Metric main 47109 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 331,241k (± 0.01%) 331,232k (± 0.01%) -9k (- 0.00%) 331,147k 331,307k
Parse Time 1.95s (± 0.46%) 1.94s (± 1.45%) -0.01s (- 0.67%) 1.92s 2.05s
Bind Time 0.86s (± 0.64%) 0.86s (± 0.52%) -0.00s (- 0.46%) 0.85s 0.87s
Check Time 5.41s (± 0.36%) 5.40s (± 0.40%) -0.01s (- 0.15%) 5.37s 5.47s
Emit Time 6.21s (± 0.65%) 6.17s (± 0.65%) -0.04s (- 0.69%) 6.11s 6.28s
Total Time 14.44s (± 0.34%) 14.38s (± 0.30%) -0.06s (- 0.42%) 14.28s 14.46s
Compiler-Unions - node (v14.15.1, x64)
Memory used 191,776k (± 0.60%) 191,902k (± 0.62%) +126k (+ 0.07%) 190,235k 193,521k
Parse Time 0.81s (± 1.00%) 0.81s (± 0.94%) -0.01s (- 0.86%) 0.79s 0.83s
Bind Time 0.56s (± 0.59%) 0.56s (± 0.65%) -0.00s (- 0.71%) 0.55s 0.56s
Check Time 7.44s (± 0.76%) 7.48s (± 0.77%) +0.04s (+ 0.52%) 7.36s 7.66s
Emit Time 2.48s (± 0.75%) 2.48s (± 0.84%) -0.00s (- 0.08%) 2.44s 2.54s
Total Time 11.29s (± 0.51%) 11.32s (± 0.60%) +0.03s (+ 0.25%) 11.19s 11.50s
Monaco - node (v14.15.1, x64)
Memory used 324,384k (± 0.00%) 324,393k (± 0.01%) +9k (+ 0.00%) 324,357k 324,431k
Parse Time 1.51s (± 0.62%) 1.52s (± 1.26%) +0.00s (+ 0.20%) 1.49s 1.59s
Bind Time 0.76s (± 0.90%) 0.76s (± 0.48%) 0.00s ( 0.00%) 0.75s 0.76s
Check Time 5.33s (± 0.61%) 5.31s (± 0.52%) -0.02s (- 0.32%) 5.27s 5.40s
Emit Time 3.26s (± 0.46%) 3.25s (± 0.66%) -0.01s (- 0.37%) 3.21s 3.32s
Total Time 10.86s (± 0.44%) 10.83s (± 0.44%) -0.02s (- 0.21%) 10.73s 10.95s
TFS - node (v14.15.1, x64)
Memory used 289,204k (± 0.01%) 289,192k (± 0.01%) -12k (- 0.00%) 289,155k 289,236k
Parse Time 1.24s (± 0.72%) 1.24s (± 0.89%) +0.00s (+ 0.32%) 1.22s 1.28s
Bind Time 0.73s (± 0.41%) 0.73s (± 0.76%) -0.00s (- 0.00%) 0.72s 0.75s
Check Time 4.95s (± 0.45%) 4.96s (± 0.57%) +0.00s (+ 0.10%) 4.90s 5.03s
Emit Time 3.50s (± 1.22%) 3.49s (± 0.63%) -0.02s (- 0.49%) 3.44s 3.55s
Total Time 10.43s (± 0.48%) 10.41s (± 0.43%) -0.01s (- 0.13%) 10.30s 10.49s
material-ui - node (v14.15.1, x64)
Memory used 448,288k (± 0.06%) 448,440k (± 0.00%) +152k (+ 0.03%) 448,386k 448,488k
Parse Time 1.84s (± 0.36%) 1.85s (± 2.15%) +0.00s (+ 0.27%) 1.80s 2.00s
Bind Time 0.68s (± 0.65%) 0.68s (± 0.76%) -0.00s (- 0.15%) 0.67s 0.70s
Check Time 12.96s (± 0.97%) 12.87s (± 0.63%) -0.09s (- 0.70%) 12.76s 13.11s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.49s (± 0.81%) 15.40s (± 0.60%) -0.09s (- 0.59%) 15.27s 15.62s
xstate - node (v14.15.1, x64)
Memory used 532,835k (± 0.00%) 532,865k (± 0.00%) +30k (+ 0.01%) 532,843k 532,913k
Parse Time 2.55s (± 0.23%) 2.56s (± 0.56%) +0.01s (+ 0.51%) 2.53s 2.59s
Bind Time 1.15s (± 0.65%) 1.16s (± 0.91%) +0.00s (+ 0.35%) 1.13s 1.18s
Check Time 1.48s (± 0.54%) 1.48s (± 0.87%) -0.00s (- 0.13%) 1.46s 1.52s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.26s (± 0.24%) 5.27s (± 0.42%) +0.01s (+ 0.19%) 5.22s 5.32s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory8 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 47109 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

@ahejlsberg
Great news! no new errors were found between main..refs/pull/47109/merge

@jcalz
Copy link
Contributor

jcalz commented Dec 11, 2021

I'm kind of surprised at how much of this already works in TS4.5 without these fixes. Both processRecord(r1); and processRecord(r2); are fine. Sure, the K parameter isn't inferred, but it falls back to keyof RecordMap which is fine for a distributive object type. Only

processRecord({ kind: 'n', v: 42, f: v => v.toExponential() }); // error

fails outright, and that's just because of unfortunate contextual typing of v as RecordMap[keyof RecordMap] . In current TS I'd be inclined to fix that with either of these:

processRecord<'n'>({ kind: 'n', v: 42, f: v => v.toExponential() }); // okay
processRecord({ kind: 'n', v: 42, f: (v: number) => v.toExponential() }); // okay

Am I right in thinking the current fix is mostly helping inference for K succeed here so that these are not necessary? I'm definitely interested in playing around with this. Thanks for working on it!!

Playground link

@ahejlsberg
Copy link
Member Author

ahejlsberg commented Dec 11, 2021

I'm kind of surprised at how much of this already works in TS4.5 without these fixes.

Yes, the nice thing is that this approach isn't really a new feature. The core capabilities were all there, they just didn't work quite right in a few cases. This in contrast to a whole new feature like existential types, which would be a very complex undertaking.

Am I right in thinking the current fix is mostly helping inference for K succeed here so that these are not necessary?

The PR has three fixes in it. Two of them relate to inference, specifically that we would fail to infer from object literals that contain contextually typed arrow functions.

The third fix relates to indexing non-generic mapped types with generic index types. For example:

type RecordFuncMap = { [P in keyof RecordMap]: (x: RecordMap[P]) => void };

const recordFuncs: RecordFuncMap = {
    n: n => n.toFixed(2),
    s: s => s.length,
    b: b => b ? 1 : 0,
}

function callRecordFunc<K extends keyof RecordMap>(rec: UnionRecord<K>) {
    const fn = recordFuncs[rec.kind];  // RecordFuncMap[K]
    fn(rec.v);  // Would fail, but works with this PR
}

The call in callRecordFunc failed to compile because we transformed type RecordFuncMap[K] into RecordFuncMap[typeof RecordMap] by constraint substitution when obtaining what we call the apparent type of fn. With the fix in the PR, we instead transform RecordFuncMap[K] into (x: RecordMap[K]) => void.

There are more examples of this pattern in the tests I just added to the PR. See here.

@ahejlsberg
Copy link
Member Author

Performance is unaffected by this PR and all tests look clean. I think this is ready to merge.

@ahejlsberg ahejlsberg merged commit 3d3825e into main Dec 14, 2021
@ahejlsberg ahejlsberg deleted the fix30581 branch December 14, 2021 19:51
zerobias added a commit to effector/effector that referenced this pull request Dec 14, 2021
Anders Hejlsberg fixed issues of version 4.5.4 in microsoft/TypeScript#47109 so I revert typescript version until it will be released

Hejlsberg saved my day ❤️
@quixot1c
Copy link

quixot1c commented Jan 1, 2022

I have run into an oddity that might be related to this PR. Consider the following code:

enum MyEvent {
    One,
    Two
}

type Callback<T> = (arg: T) => void;

interface CallbackTypes {
    [MyEvent.One]: number
    [MyEvent.Two]: string
}

type Callbacks = { [E in MyEvent]: Callback<CallbackTypes[E]>[] };

class CallbackContainer {
    callbacks: Callbacks = {
        [MyEvent.One]: [],
        [MyEvent.Two]: []
    };

    constructor() { }

    setListener<E extends MyEvent>(event: E, callback: Callback<CallbackTypes[E]>) {
        const callbacks = this.callbacks[event];
        callbacks.push(callback); // okay
    }

    getListeners<E extends MyEvent>(event: E) {
        return this.callbacks[event];
    }
}

Thanks to this PR above code now works! But when I change const callbacks = this.callbacks[event] to const callbacks: Array<Callback<CallbackTypes[E]>> = this.callbacks[event] the compiler throws Type 'Callbacks[E]' is not assignable to type 'Callback<CallbackTypes[E]>[]'. But when (in both versions) examining the type of callbacks.push, the Array is typed as Array<Callback<CallbackTypes[E]>>.

Why then, is Callbacks[E] not assignable to Callback<CallbackTypes[E]>[] while in the push call the Callbacks[E] is automagically converted to that very same type Array<Callback<CallbackTypes[E]>>?

@jcalz jcalz mentioned this pull request Jul 5, 2024
6 tasks
@Andarist
Copy link
Contributor

This PR is linked from TypeScript 4.6 release notes but the implementation changed soon after it here: #47370

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wishlist: support for correlated union types
6 participants