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

Defer resolution of indexed access types with reducible object types #53098

Merged
merged 4 commits into from
Mar 14, 2023

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Mar 5, 2023

This reuses some of the machinery introduced in #46812 to also defer resolution of indexed access types with reducible object types. The additional issue I mention here is actually caused by #46812 always deferring keyof for reducible types, making it impossible to explore their constraints. That's fixed in this PR by a new flag on getIndexType that disables deferral.

EDIT: Turns out the additional issue is a duplicate of #51161. This also means that this PR supercedes #51176.

Fixes #51161.
Fixes #53030.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 5, 2023
@ahejlsberg
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2023

Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 8334e30. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2023

Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 8334e30. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@ahejlsberg Here are the results of running the user test suite comparing main and refs/pull/53098/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..53098

Metric main 53098 Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 359,078k (± 0.00%) 359,091k (± 0.00%) ~ 359,071k 359,099k p=0.172 n=6
Parse Time 3.72s (± 0.22%) 3.72s (± 0.50%) ~ 3.69s 3.74s p=0.187 n=6
Bind Time 1.19s (± 0.46%) 1.19s (± 0.68%) ~ 1.18s 1.20s p=0.859 n=6
Check Time 9.42s (± 0.62%) 9.41s (± 0.31%) ~ 9.37s 9.45s p=0.520 n=6
Emit Time 7.90s (± 0.84%) 7.92s (± 0.57%) ~ 7.86s 7.97s p=0.572 n=6
Total Time 22.23s (± 0.25%) 22.25s (± 0.24%) ~ 22.19s 22.32s p=0.520 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 192,383k (± 0.03%) 192,232k (± 0.03%) -151k (- 0.08%) 192,152k 192,314k p=0.008 n=6
Parse Time 1.56s (± 0.52%) 1.57s (± 0.48%) ~ 1.56s 1.58s p=0.120 n=6
Bind Time 0.82s (± 0.00%) 0.82s (± 0.50%) ~ 0.81s 0.82s p=0.405 n=6
Check Time 10.12s (± 0.60%) 10.05s (± 0.63%) ~ 9.98s 10.15s p=0.121 n=6
Emit Time 2.98s (± 0.86%) 3.01s (± 0.83%) ~ 2.96s 3.03s p=0.123 n=6
Total Time 15.48s (± 0.45%) 15.45s (± 0.59%) ~ 15.31s 15.57s p=0.688 n=6
Monaco - node (v16.17.1, x64)
Memory used 343,113k (± 0.00%) 343,103k (± 0.01%) ~ 343,078k 343,126k p=0.298 n=6
Parse Time 2.79s (± 0.27%) 2.79s (± 1.03%) ~ 2.76s 2.83s p=0.935 n=6
Bind Time 1.08s (± 0.76%) 1.08s (± 0.38%) ~ 1.08s 1.09s p=0.218 n=6
Check Time 7.67s (± 0.37%) 7.67s (± 0.32%) ~ 7.64s 7.71s p=1.000 n=6
Emit Time 4.42s (± 0.44%) 4.42s (± 0.48%) ~ 4.39s 4.45s p=1.000 n=6
Total Time 15.97s (± 0.18%) 15.97s (± 0.31%) ~ 15.90s 16.02s p=1.000 n=6
TFS - node (v16.17.1, x64)
Memory used 299,227k (± 0.01%) 299,233k (± 0.01%) ~ 299,200k 299,260k p=0.689 n=6
Parse Time 2.17s (± 0.48%) 2.16s (± 0.46%) ~ 2.14s 2.17s p=0.176 n=6
Bind Time 1.24s (± 0.41%) 1.24s (± 0.94%) ~ 1.22s 1.25s p=0.542 n=6
Check Time 7.17s (± 0.39%) 7.17s (± 0.44%) ~ 7.13s 7.20s p=0.809 n=6
Emit Time 4.33s (± 0.65%) 4.34s (± 0.81%) ~ 4.28s 4.38s p=0.566 n=6
Total Time 14.91s (± 0.15%) 14.90s (± 0.46%) ~ 14.83s 14.99s p=1.000 n=6
material-ui - node (v16.17.1, x64)
Memory used 475,669k (± 0.01%) 475,667k (± 0.01%) ~ 475,633k 475,741k p=0.575 n=6
Parse Time 3.28s (± 0.42%) 3.29s (± 0.25%) ~ 3.28s 3.30s p=0.865 n=6
Bind Time 0.96s (± 0.57%) 0.96s (± 0.43%) ~ 0.95s 0.96s p=0.282 n=6
Check Time 17.99s (± 0.24%) 18.04s (± 0.48%) ~ 17.91s 18.15s p=0.261 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 22.23s (± 0.16%) 22.28s (± 0.42%) ~ 22.16s 22.42s p=0.257 n=6
xstate - node (v16.17.1, x64)
Memory used 546,456k (± 0.02%) 546,492k (± 0.02%) ~ 546,342k 546,629k p=0.689 n=6
Parse Time 4.28s (± 0.54%) 4.27s (± 0.34%) ~ 4.25s 4.29s p=0.418 n=6
Bind Time 1.77s (± 0.46%) 1.76s (± 0.23%) ~ 1.75s 1.76s p=0.056 n=6
Check Time 2.98s (± 0.76%) 2.99s (± 0.89%) ~ 2.96s 3.03s p=0.370 n=6
Emit Time 0.09s (± 5.53%) 0.09s (± 4.45%) ~ 0.09s 0.10s p=0.595 n=6
Total Time 9.11s (± 0.36%) 9.11s (± 0.39%) ~ 9.06s 9.15s p=1.000 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-135-generic
Architecturex64
Available Memory16 GB
Available Memory15 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current 53098 6
Baseline main 6

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

Hey @ahejlsberg, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot
Copy link
Collaborator

@ahejlsberg Here are the results of running the top-repos suite comparing main and refs/pull/53098/merge:

Everything looks good!


// Additional repro from 53030

type GetPayload<P extends Payload, K extends keyof P> = P extends { dataType: K } ? P["data"] : never;
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps it would still be worthwhile to include a test from #51161? that one was even more suspicious as the index access in the true branch was using the same property name as the one in the extendsType. This additional repro is using a different one

Suggested change
type GetPayload<P extends Payload, K extends keyof P> = P extends { dataType: K } ? P["data"] : never;
type GetPayload<P extends Payload, K extends keyof P> = P extends { dataType: K } ? P["data"] : never;
// repro from 51161
export type AnyOneof = { oneofKind: string; [k: string]: unknown } | { oneofKind: undefined };
export type AnyOneofKind<T extends AnyOneof> = T extends { oneofKind: keyof T }
? T['oneofKind']
: never;

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll add the extra test.

@seansfkelley
Copy link

seansfkelley commented Mar 10, 2023

@typescript-bot pack this

edit: I thought anyone could request a build... I think this fixes an issue I'm having so I'd like to test it in the playground.

@ahejlsberg
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 10, 2023

Heya @ahejlsberg, I've started to run the tarball bundle task on this PR at 20827a5. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 10, 2023

Hey @ahejlsberg, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/149032/artifacts?artifactName=tgz&fileId=14D9E5F2E6B54A9FD94927AE981553AEE2D6DF9F25DC315BEBA913DBA210C42F02&fileName=/typescript-5.1.0-insiders.20230310.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.1.0-pr-53098-13".;

@seansfkelley
Copy link

Thanks! Looks like it doesn't resolve my issue, unfortunately. It looks like the same basic pattern to me -- is this expected after the fix?

const SomeRpcEndpointId = 'someRpcEndpoint' as const ;
type SomeRpcEndpointId = typeof SomeRpcEndpointId;

interface SomeRpcEndpointActions {
  doFoo: {
    in: string;
    out: number;
  };
  doBar: {
    in: boolean;
    out: boolean;
  };
}

type AllRpcEndpoints = {
  [SomeRpcEndpointId]: SomeRpcEndpointActions;
};

type InvokeRpc = <H extends keyof AllRpcEndpoints, A extends keyof AllRpcEndpoints[H]>(
  handler: H,
  action: A,
  payload: AllRpcEndpoints[H][A]['in'],
) => Promise<AllRpcEndpoints[H][A]['out']>;

declare const invokeRpc: InvokeRpc;
// Despite the compiler errors above, this does type-check correctly.
const result: boolean = await invokeRpc('someRpcEndpoint', 'doBar', true);

// Workaround inspired by https://github.com/microsoft/TypeScript/issues/53030#issuecomment-1448934975.
type GetIn<T> = T extends { in: any } ? T['in'] : never;
type GetOut<T> = T extends { out: any } ? T['out'] : never;

type InvokeRpcWorkaround = <H extends keyof AllRpcEndpoints, A extends keyof AllRpcEndpoints[H]>(
  handler: H,
  action: A,
  payload: GetIn<AllRpcEndpoints[H][A]>,
) => Promise<GetOut<AllRpcEndpoints[H][A]>>;

declare const invokeRpcWorkaround: InvokeRpcWorkaround;
// Type-checks as expected.
const resultWorkaround: boolean = await invokeRpcWorkaround('someRpcEndpoint', 'doBar', true);

export {}; // appease the compiler

playground link for this PR

@Andarist
Copy link
Contributor

@seansfkelley you are looking for a fix for this issue.

@ahejlsberg ahejlsberg merged commit 9769421 into main Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
5 participants