Skip to content

Conversation

@Andarist
Copy link
Contributor

fixes #55129

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 24, 2023
const objectType = (type as IndexedAccessType).objectType;
const indexType = (type as IndexedAccessType).indexType;
if (isTypeAssignableTo(indexType, getIndexType(objectType, IndexFlags.None))) {
if (isTypeAssignableTo(indexType, getIndexType(objectType, IndexFlags.NoRemappingMappedTypeDeferral))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda expected that I would have to pass this at more locations. Given the fact that this is the only call site using this... maybe I just should remove this IndexFlags.NoRemappingMappedTypeDeferral and perform the targeted simplification of keyof RemappingMappedType here to pass it to the isTypeAssignableTo.

Copy link
Member

Choose a reason for hiding this comment

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

That would probably be better - IndexFlags originally only existed to support the keyofStringsOnly flag, and its' other uses have kind of become a convenience. With keyofStringsOnly likely to go away soonish, it'd be nice if we could stop having all these quiet variant index types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed out this change

@sandersn sandersn requested a review from ahejlsberg August 11, 2023 23:06
@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 11, 2023
@sandersn sandersn requested a review from weswigham August 11, 2023 23:06
const objectType = (type as IndexedAccessType).objectType;
const indexType = (type as IndexedAccessType).indexType;
if (isTypeAssignableTo(indexType, getIndexType(objectType, IndexFlags.None))) {
if (isTypeAssignableTo(indexType, getIndexType(objectType, IndexFlags.NoRemappingMappedTypeDeferral))) {
Copy link
Member

Choose a reason for hiding this comment

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

That would probably be better - IndexFlags originally only existed to support the keyofStringsOnly flag, and its' other uses have kind of become a convenience. With keyofStringsOnly likely to go away soonish, it'd be nice if we could stop having all these quiet variant index types.

const enum MappedTypeNameTypeKind {
None = 0,
Filtering = 1 << 0,
Remapping = 1 << 1,
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a flags enum? I can't see anywhere we actually combine multiple flags - in fact, it seems to me like a mapped type can only be one, given the implementation of getMappedTypeNameTypeKind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had some composite values here before but removed them at the end since I didn't actually use them anywhere. Refactored this now to be a non-flag enum

@Andarist Andarist requested a review from weswigham August 14, 2023 21:08
@weswigham
Copy link
Member

@typescript-bot test this
@typescript-bot run dt
@typescript-bot test top100
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2023

Heya @weswigham, I've started to run the regular perf test suite on this PR at 9f95520. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 300,275k (± 0.01%) 300,270k (± 0.01%) ~ 300,247k 300,307k p=0.873 n=6
Parse Time 3.01s (± 0.28%) 3.01s (± 0.14%) ~ 3.00s 3.01s p=0.285 n=6
Bind Time 0.93s (± 0.00%) 0.93s (± 0.44%) ~ 0.93s 0.94s p=0.405 n=6
Check Time 9.34s (± 0.34%) 9.31s (± 0.30%) ~ 9.28s 9.35s p=0.075 n=6
Emit Time 7.64s (± 0.27%) 7.63s (± 0.24%) ~ 7.61s 7.66s p=0.626 n=6
Total Time 20.92s (± 0.20%) 20.88s (± 0.15%) ~ 20.84s 20.92s p=0.106 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 193,951k (± 0.02%) 193,959k (± 0.02%) ~ 193,931k 194,001k p=0.810 n=6
Parse Time 1.58s (± 0.00%) 1.58s (± 0.00%) ~ 1.58s 1.58s p=1.000 n=6
Bind Time 0.80s (± 0.65%) 0.80s (± 0.65%) ~ 0.79s 0.80s p=1.000 n=6
Check Time 9.93s (± 0.31%) 9.94s (± 0.28%) ~ 9.91s 9.98s p=0.809 n=6
Emit Time 2.74s (± 0.27%) 2.74s (± 0.15%) ~ 2.73s 2.74s p=0.389 n=6
Total Time 15.05s (± 0.23%) 15.06s (± 0.20%) ~ 15.03s 15.10s p=0.809 n=6
Monaco - node (v16.17.1, x64)
Memory used 347,197k (± 0.01%) 347,190k (± 0.01%) ~ 347,156k 347,219k p=0.470 n=6
Parse Time 2.69s (± 0.19%) 2.69s (± 0.20%) ~ 2.68s 2.69s p=0.640 n=6
Bind Time 0.99s (± 0.00%) 0.99s (± 0.41%) ~ 0.98s 0.99s p=0.405 n=6
Check Time 7.93s (± 0.10%) 7.94s (± 0.26%) ~ 7.92s 7.97s p=0.557 n=6
Emit Time 4.27s (± 0.32%) 4.27s (± 0.24%) ~ 4.25s 4.28s p=0.867 n=6
Total Time 15.88s (± 0.11%) 15.88s (± 0.16%) ~ 15.85s 15.92s p=0.871 n=6
TFS - node (v16.17.1, x64)
Memory used 301,179k (± 0.00%) 301,190k (± 0.00%) ~ 301,176k 301,205k p=0.199 n=6
Parse Time 2.17s (± 0.56%) 2.17s (± 0.61%) ~ 2.15s 2.18s p=0.652 n=6
Bind Time 1.11s (± 0.00%) 1.11s (± 0.37%) ~ 1.11s 1.12s p=0.405 n=6
Check Time 7.23s (± 0.21%) 7.23s (± 0.17%) ~ 7.22s 7.25s p=0.327 n=6
Emit Time 3.99s (± 0.43%) 3.98s (± 0.32%) ~ 3.96s 3.99s p=0.359 n=6
Total Time 14.50s (± 0.13%) 14.50s (± 0.17%) ~ 14.46s 14.52s p=1.000 n=6
material-ui - node (v16.17.1, x64)
Memory used 479,542k (± 0.00%) 479,534k (± 0.00%) ~ 479,522k 479,541k p=0.521 n=6
Parse Time 3.15s (± 0.16%) 3.16s (± 0.20%) +0.01s (+ 0.42%) 3.15s 3.17s p=0.009 n=6
Bind Time 0.91s (± 0.00%) 0.91s (± 0.00%) ~ 0.91s 0.91s p=1.000 n=6
Check Time 17.86s (± 0.27%) 17.93s (± 0.40%) ~ 17.82s 18.02s p=0.106 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 21.92s (± 0.23%) 21.99s (± 0.34%) ~ 21.89s 22.10s p=0.090 n=6
xstate - node (v16.17.1, x64)
Memory used 542,941k (± 0.01%) 542,872k (± 0.01%) ~ 542,806k 542,930k p=0.128 n=6
Parse Time 3.69s (± 0.33%) 3.69s (± 0.20%) ~ 3.68s 3.70s p=0.867 n=6
Bind Time 1.42s (± 4.50%) 1.44s (± 3.69%) ~ 1.33s 1.46s p=0.440 n=6
Check Time 3.23s (± 2.96%) 3.19s (± 2.23%) ~ 3.15s 3.33s p=0.060 n=6
Emit Time 0.08s (± 4.99%) 0.09s (± 6.44%) ~ 0.08s 0.09s p=0.282 n=6
Total Time 8.43s (± 0.45%) 8.40s (± 0.31%) ~ 8.37s 8.45s p=0.347 n=6
System info unknown
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 pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,491ms (± 0.07%) 2,491ms (± 0.11%) ~ 2,488ms 2,494ms p=0.934 n=6
Req 2 - geterr 5,942ms (± 0.36%) 5,942ms (± 0.26%) ~ 5,914ms 5,959ms p=1.000 n=6
Req 3 - references 342ms (± 0.22%) 343ms (± 0.40%) ~ 342ms 346ms p=0.081 n=6
Req 4 - navto 275ms (± 0.37%) 276ms (± 0.20%) ~ 275ms 276ms p=0.663 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 78ms (± 3.97%) 78ms (± 3.32%) ~ 76ms 81ms p=0.774 n=6
CompilerTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,615ms (± 0.48%) 2,618ms (± 0.46%) ~ 2,606ms 2,638ms p=0.572 n=6
Req 2 - geterr 4,781ms (± 0.18%) 4,780ms (± 0.15%) ~ 4,772ms 4,788ms p=0.936 n=6
Req 3 - references 350ms (± 0.28%) 351ms (± 0.23%) ~ 350ms 352ms p=0.282 n=6
Req 4 - navto 269ms (± 0.20%) 271ms (± 1.51%) ~ 266ms 276ms p=0.371 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 79ms (± 0.80%) 79ms (± 0.65%) ~ 79ms 80ms p=0.386 n=6
xstateTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,705ms (± 0.28%) 2,710ms (± 0.17%) ~ 2,704ms 2,716ms p=0.199 n=6
Req 2 - geterr 1,964ms (± 2.56%) 1,951ms (± 2.02%) ~ 1,900ms 1,981ms p=0.296 n=6
Req 3 - references 133ms (± 8.56%) 137ms (± 1.94%) ~ 134ms 140ms p=0.466 n=6
Req 4 - navto 359ms (± 0.99%) 358ms (± 0.76%) ~ 355ms 362ms p=0.517 n=6
Req 5 - completionInfo count 2,071 (± 0.00%) 2,071 (± 0.00%) ~ 2,071 2,071 p=1.000 n=6
Req 5 - completionInfo 328ms (± 1.05%) 321ms (± 1.66%) ~ 317ms 331ms p=0.091 n=6
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • CompilerTSServer - node (v16.17.1, x64)
  • Compiler-UnionsTSServer - node (v16.17.1, x64)
  • xstateTSServer - node (v16.17.1, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v16.17.1, x64)
Execution time 156.22ms (± 0.16%) 156.53ms (± 0.18%) +0.31ms (+ 0.20%) 154.58ms 159.01ms p=0.000 n=600
tsserver-startup - node (v16.17.1, x64)
Execution time 231.16ms (± 0.15%) 231.86ms (± 0.13%) +0.71ms (+ 0.31%) 230.35ms 234.83ms p=0.000 n=600
tsserverlibrary-startup - node (v16.17.1, x64)
Execution time 235.57ms (± 0.12%) 236.08ms (± 0.12%) +0.50ms (+ 0.21%) 234.77ms 240.01ms p=0.000 n=600
typescript-startup - node (v16.17.1, x64)
Execution time 235.64ms (± 0.12%) 235.61ms (± 0.11%) ~ 234.33ms 239.65ms p=0.393 n=600
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • tsc-startup - node (v16.17.1, x64)
  • tsserver-startup - node (v16.17.1, x64)
  • tsserverlibrary-startup - node (v16.17.1, x64)
  • typescript-startup - node (v16.17.1, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@typescript-bot
Copy link
Collaborator

Hey @weswigham, it looks like the DT test run failed. Please check the log for more details.
You can check the log here.

@sandersn
Copy link
Member

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 30, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

@sandersn sandersn merged commit 2c4cbd9 into microsoft:main Nov 30, 2023
@Andarist Andarist deleted the fix/defer-index-types-on-remapping-mapped-types branch November 11, 2024 19:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Backlog Bug PRs that fix a backlog bug

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

keyof collapses the expression when used in a mapped type

5 participants