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

Improve contextual typing of ending tuple elements #53036

Merged
merged 7 commits into from
Mar 19, 2023
Merged

Improve contextual typing of ending tuple elements #53036

merged 7 commits into from
Mar 19, 2023

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Mar 1, 2023

This PR improves the precision of contextual types originating in ending elements of contextual tuple types. For example:

declare function test(...args: [...((arg: number) => void)[], (arg: string) => void]): void;

test(
    a => a,  // number
    b => b,  // number
    c => c   // string
);

const funcs: [...((arg: number) => void)[], (arg: string) => void] = [
    a => a,  // number
    b => b,  // number
    c => c   // string
];

Previously, the arrow function parameters would just be given an implicit any type.

Fixes #43122.
Fixes #52846.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 1, 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 1, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2023

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

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@typescript-bot
Copy link
Collaborator

Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here.

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@ahejlsberg
Copy link
Member Author

@typescript-bot perf test faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2023

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at b2f0543. 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..53036

Metric main 53036 Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 359,068k (± 0.01%) 359,288k (± 0.01%) +220k (+ 0.06%) 359,263k 359,311k p=0.005 n=6
Parse Time 3.71s (± 0.59%) 3.72s (± 0.56%) ~ 3.68s 3.74s p=0.571 n=6
Bind Time 1.19s (± 0.53%) 1.20s (± 0.43%) ~ 1.19s 1.20s p=0.091 n=6
Check Time 9.45s (± 0.58%) 9.46s (± 0.38%) ~ 9.40s 9.50s p=0.568 n=6
Emit Time 7.89s (± 0.32%) 7.90s (± 0.67%) ~ 7.87s 8.01s p=0.870 n=6
Total Time 22.24s (± 0.27%) 22.28s (± 0.33%) ~ 22.21s 22.42s p=0.809 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 191,785k (± 0.68%) 191,266k (± 0.03%) ~ 191,201k 191,348k p=0.936 n=6
Parse Time 1.56s (± 0.94%) 1.57s (± 1.10%) ~ 1.55s 1.60s p=0.742 n=6
Bind Time 0.82s (± 0.92%) 0.82s (± 0.00%) ~ 0.82s 0.82s p=0.598 n=6
Check Time 10.09s (± 0.57%) 10.09s (± 0.40%) ~ 10.04s 10.14s p=0.747 n=6
Emit Time 2.99s (± 0.63%) 3.01s (± 0.58%) ~ 3.00s 3.04s p=0.088 n=6
Total Time 15.46s (± 0.40%) 15.49s (± 0.36%) ~ 15.42s 15.57s p=0.520 n=6
Monaco - node (v16.17.1, x64)
Memory used 343,116k (± 0.01%) 343,193k (± 0.00%) +77k (+ 0.02%) 343,171k 343,219k p=0.005 n=6
Parse Time 2.80s (± 0.53%) 2.81s (± 0.35%) ~ 2.80s 2.82s p=0.216 n=6
Bind Time 1.08s (± 0.48%) 1.09s (± 1.07%) ~ 1.08s 1.11s p=0.523 n=6
Check Time 7.68s (± 0.45%) 7.68s (± 0.54%) ~ 7.64s 7.76s p=1.000 n=6
Emit Time 4.43s (± 0.82%) 4.42s (± 0.90%) ~ 4.37s 4.48s p=0.748 n=6
Total Time 15.99s (± 0.41%) 16.01s (± 0.42%) ~ 15.90s 16.07s p=0.629 n=6
TFS - node (v16.17.1, x64)
Memory used 299,241k (± 0.01%) 299,288k (± 0.00%) +47k (+ 0.02%) 299,278k 299,314k p=0.030 n=6
Parse Time 2.15s (± 0.48%) 2.16s (± 0.62%) ~ 2.14s 2.18s p=0.652 n=6
Bind Time 1.24s (± 1.19%) 1.25s (± 0.71%) ~ 1.24s 1.26s p=0.208 n=6
Check Time 7.18s (± 0.33%) 7.16s (± 0.33%) ~ 7.14s 7.20s p=0.257 n=6
Emit Time 4.34s (± 0.62%) 4.32s (± 0.48%) ~ 4.30s 4.35s p=0.327 n=6
Total Time 14.91s (± 0.30%) 14.90s (± 0.40%) ~ 14.84s 14.97s p=0.748 n=6
material-ui - node (v16.17.1, x64)
Memory used 475,646k (± 0.01%) 475,711k (± 0.01%) +65k (+ 0.01%) 475,647k 475,765k p=0.045 n=6
Parse Time 3.29s (± 0.61%) 3.30s (± 0.25%) ~ 3.29s 3.31s p=0.565 n=6
Bind Time 0.96s (± 0.93%) 0.96s (± 0.66%) ~ 0.95s 0.97s p=1.000 n=6
Check Time 18.03s (± 0.21%) 18.08s (± 0.45%) ~ 17.97s 18.18s p=0.334 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 22.28s (± 0.22%) 22.33s (± 0.34%) ~ 22.23s 22.43s p=0.295 n=6
xstate - node (v16.17.1, x64)
Memory used 545,709k (± 0.02%) 545,736k (± 0.01%) ~ 545,666k 545,771k p=0.810 n=6
Parse Time 4.28s (± 0.50%) 4.30s (± 0.65%) ~ 4.26s 4.33s p=0.195 n=6
Bind Time 1.76s (± 0.46%) 1.76s (± 0.29%) ~ 1.76s 1.77s p=0.140 n=6
Check Time 2.98s (± 0.42%) 2.99s (± 0.49%) ~ 2.97s 3.01s p=0.255 n=6
Emit Time 0.09s (± 0.00%) 0.09s (± 0.00%) ~ 0.09s 0.09s p=1.000 n=6
Total Time 9.10s (± 0.27%) 9.14s (± 0.25%) +0.04s (+ 0.48%) 9.11s 9.18s p=0.016 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 53036 6
Baseline main 6

Developer Information:

Download Benchmark

}
// If element index is known and a contextual property with that name exists, return it. Otherwise return the
// iterated or element type of the contextual type.
return (!firstSpreadIndex || index < firstSpreadIndex) && getTypeOfPropertyOfContextualType(type, "" + index as __String) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should be treated as a separate issue... but I feel that perhaps this feature here might directly complicate things for that issue so it's at least worth mentioning now.

Take a look at this TS playground. The problem there is that getTypeOfPropertyOfContextualType gets the getElementTypeOfSliceOfTupleType, so we end up with a union of all element types instead of types of individual tuple members.

I prepared a quick fix for this issue in this PR: #53042 but it doesn't handle the case that is being fixed here (ending tuple elements).

The case outlined there is currently a little bit silly (T[K & keyof T] within [K in keyof T2]: { ... }) but I partially encountered this need when working on inference for intersected mapped types (PR here). This case would be better if rewritten like this:

diff --git a/tests/cases/compiler/contextuallyTypedElementsOfGenericZippingTuples.ts b/tests/cases/compiler/contextuallyTypedElementsOfGenericZippingTuples.ts
index a011321a26..c27c91930f 100644
--- a/tests/cases/compiler/contextuallyTypedElementsOfGenericZippingTuples.ts
+++ b/tests/cases/compiler/contextuallyTypedElementsOfGenericZippingTuples.ts
@@ -11,8 +11,8 @@ declare function test<T extends unknown[], T2 extends unknown[]>(
   ],
   b: [
     ...{
-      [K in keyof T2]: {
-        consume: (arg: T[K & keyof T]) => T2[K];
+      [K in keyof T2 as K & keyof T]: {
+        consume: (arg: T[K]) => T2[K];
       };
     }
   ]

There are 2 problems with this code today though:

  1. such a reverse mapped type isn't inferred at all (a PR fixing this problem can be found here)
  2. this isn't recognized as a tuple type so TS reports A rest element type must be an array type.ts(2574). That probably could be improved as well.

Copy link
Member

Choose a reason for hiding this comment

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

@ahejlsberg Any concerns given the above?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that's a separate issue.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I just meant specifically the "this feature here might directly complicate things for that issue so it's at least worth mentioning now" bit.

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
Archived in project
4 participants