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.

Sign up for GitHub

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

Jump to bottom

Resolve apparent mapped types recursively #57091

Closed
wants to merge 2 commits into from
Closed

Resolve apparent mapped types recursively #57091

wants to merge 2 commits into from

Conversation

Andarist
Copy link
Contributor

this expands on #56727
I'm opening this as a draft but I'd love it if somebody could run an extended test suite on this PR in the meantime (cc @gabritto )

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jan 18, 2024
Andarist
Andarist commented Jan 18, 2024
View reviewed changes
src/compiler/checker.ts
constraint = getApparentType(modifiersConstraint);
const modifiersType = getModifiersTypeFromMappedType(type);
if (modifiersType) {
constraint = getApparentType(getConstraintOfType(modifiersType) || modifiersType);
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'm not particularly fond of this - maybe there is a better way of doing this. This mainly tries to get the apparent type of the modifiers type when the modifiers type is a mapped type itself (like in Mapped<Mapped2<Foo>>). Mapped types don't return anything from getConstraintOfType.

@Andarist Andarist mentioned this pull request Jan 18, 2024
Merged
@Andarist
Copy link
Contributor Author

The test failure doesn't surprise me. It would pass if another problem would get fixed - the one that is being tackled by #50034

The recursive resolution here just uncovers the fact that this other thing doesn't exactly work properly - as far as I was able to investigate so far and I spent 2 nights on this.

@gabritto
Copy link
Member

@typescript-bot run DT
@typescript-bot user test this
@typescript-bot test top100
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2024 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

@typescript-bot
Copy link
Collaborator

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

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,619k (± 0.00%) 295,612k (± 0.01%) ~ 295,579k 295,630k p=0.574 n=6
Parse Time 2.65s (± 0.19%) 2.65s (± 0.28%) ~ 2.64s 2.66s p=0.784 n=6
Bind Time 0.83s (± 0.49%) 0.83s (± 1.52%) ~ 0.82s 0.85s p=0.930 n=6
Check Time 8.17s (± 0.38%) 8.15s (± 0.53%) ~ 8.08s 8.19s p=0.747 n=6
Emit Time 7.08s (± 0.35%) 7.08s (± 0.33%) ~ 7.05s 7.11s p=1.000 n=6
Total Time 18.73s (± 0.18%) 18.72s (± 0.24%) ~ 18.66s 18.78s p=0.686 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,342k (± 1.20%) 194,462k (± 1.66%) ~ 191,501k 197,498k p=0.378 n=6
Parse Time 1.36s (± 0.93%) 1.37s (± 1.26%) ~ 1.35s 1.39s p=0.402 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.00%) ~ 0.72s 0.72s p=1.000 n=6
Check Time 9.36s (± 0.21%) 9.38s (± 0.30%) ~ 9.33s 9.41s p=0.332 n=6
Emit Time 2.62s (± 0.75%) 2.61s (± 0.40%) ~ 2.60s 2.63s p=0.289 n=6
Total Time 14.07s (± 0.17%) 14.08s (± 0.26%) ~ 14.01s 14.10s p=0.212 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,395k (± 0.00%) 347,387k (± 0.01%) ~ 347,365k 347,409k p=0.575 n=6
Parse Time 2.47s (± 0.42%) 2.47s (± 0.60%) ~ 2.45s 2.48s p=0.676 n=6
Bind Time 0.93s (± 0.56%) 0.93s (± 0.56%) ~ 0.92s 0.93s p=1.000 n=6
Check Time 6.89s (± 0.48%) 6.88s (± 0.37%) ~ 6.86s 6.92s p=0.683 n=6
Emit Time 4.05s (± 0.19%) 4.05s (± 0.16%) ~ 4.04s 4.06s p=0.718 n=6
Total Time 14.33s (± 0.19%) 14.33s (± 0.22%) ~ 14.30s 14.37s p=0.934 n=6
TFS - node (v18.15.0, x64)
Memory used 302,803k (± 0.01%) 302,779k (± 0.00%) ~ 302,762k 302,794k p=0.092 n=6
Parse Time 2.01s (± 0.68%) 2.01s (± 1.16%) ~ 1.98s 2.04s p=0.935 n=6
Bind Time 1.00s (± 0.41%) 1.00s (± 1.09%) ~ 0.99s 1.02s p=0.445 n=6
Check Time 6.30s (± 0.35%) 6.30s (± 0.34%) ~ 6.27s 6.33s p=0.871 n=6
Emit Time 3.58s (± 0.25%) 3.59s (± 0.33%) ~ 3.58s 3.61s p=0.114 n=6
Total Time 12.90s (± 0.16%) 12.91s (± 0.32%) ~ 12.86s 12.96s p=0.569 n=6
material-ui - node (v18.15.0, x64)
Memory used 511,156k (± 0.00%) 511,167k (± 0.00%) ~ 511,129k 511,187k p=0.298 n=6
Parse Time 2.64s (± 0.28%) 2.64s (± 0.56%) ~ 2.61s 2.65s p=1.000 n=6
Bind Time 1.00s (± 0.98%) 0.99s (± 0.82%) ~ 0.99s 1.01s p=0.235 n=6
Check Time 17.15s (± 0.43%) 17.14s (± 0.52%) ~ 17.02s 17.26s p=0.936 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.79s (± 0.39%) 20.78s (± 0.45%) ~ 20.64s 20.89s p=0.809 n=6
xstate - node (v18.15.0, x64)
Memory used 513,382k (± 0.01%) 513,420k (± 0.01%) ~ 513,313k 513,486k p=0.378 n=6
Parse Time 3.29s (± 0.25%) 3.29s (± 0.19%) ~ 3.28s 3.30s p=0.599 n=6
Bind Time 1.54s (± 0.33%) 1.54s (± 0.26%) ~ 1.54s 1.55s p=0.595 n=6
Check Time 2.84s (± 0.61%) 2.84s (± 0.66%) ~ 2.82s 2.87s p=0.746 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 0.00%) ~ 0.08s 0.08s p=1.000 n=6
Total Time 7.75s (± 0.21%) 7.75s (± 0.22%) ~ 7.72s 7.77s p=0.453 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, 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 (v18.15.0, x64)
Req 1 - updateOpen 2,348ms (± 0.69%) 2,352ms (± 0.55%) ~ 2,337ms 2,369ms p=0.810 n=6
Req 2 - geterr 5,505ms (± 1.19%) 5,541ms (± 1.72%) ~ 5,433ms 5,646ms p=0.630 n=6
Req 3 - references 325ms (± 1.11%) 326ms (± 1.84%) ~ 320ms 336ms p=0.808 n=6
Req 4 - navto 276ms (± 0.90%) 276ms (± 0.48%) ~ 274ms 278ms p=0.867 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 85ms (± 8.11%) 91ms (± 6.01%) ~ 84ms 95ms p=0.058 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,473ms (± 1.06%) 2,470ms (± 1.16%) ~ 2,435ms 2,522ms p=0.689 n=6
Req 2 - geterr 4,142ms (± 1.47%) 4,183ms (± 1.85%) ~ 4,106ms 4,262ms p=0.689 n=6
Req 3 - references 334ms (± 1.72%) 334ms (± 1.12%) ~ 331ms 341ms p=0.934 n=6
Req 4 - navto 285ms (± 0.42%) 285ms (± 1.01%) ~ 283ms 291ms p=1.000 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 80ms (±10.60%) 79ms (± 7.58%) ~ 74ms 90ms p=0.744 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,608ms (± 0.42%) 2,606ms (± 0.69%) ~ 2,579ms 2,627ms p=0.747 n=6
Req 2 - geterr 1,720ms (± 2.26%) 1,692ms (± 2.43%) ~ 1,651ms 1,767ms p=0.378 n=6
Req 3 - references 113ms (± 9.53%) 123ms (± 6.60%) ~ 106ms 126ms p=0.317 n=6
Req 4 - navto 364ms (± 0.70%) 365ms (± 0.38%) ~ 363ms 367ms p=1.000 n=6
Req 5 - completionInfo count 2,078 (± 0.00%) 2,078 (± 0.00%) ~ 2,078 2,078 p=1.000 n=6
Req 5 - completionInfo 310ms (± 0.78%) 307ms (± 0.95%) ~ 304ms 312ms p=0.062 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, 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 (v18.15.0, x64)
Execution time 153.95ms (± 0.20%) 153.83ms (± 0.20%) -0.12ms (- 0.08%) 152.65ms 157.49ms p=0.000 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 230.47ms (± 0.18%) 230.32ms (± 0.16%) -0.15ms (- 0.06%) 228.94ms 233.20ms p=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 231.26ms (± 0.18%) 231.16ms (± 0.22%) -0.10ms (- 0.04%) 229.60ms 243.34ms p=0.003 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 230.97ms (± 0.19%) 230.90ms (± 0.20%) -0.08ms (- 0.03%) 229.40ms 236.22ms p=0.039 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

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

@typescript-bot
Copy link
Collaborator

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

Something interesting changed - please have a look.

Details

chakra-ui/chakra-ui

4 of 28 projects failed to build with the old tsc and were ignored

packages/components/tsconfig.build.json

  • error TS5056: Cannot write file '/mnt/ts_downloads/chakra-ui/packages/components/dist/types/menu/menu.stories.d.ts' because it would be overwritten by multiple input files.
    • Project Scope

@Andarist Andarist marked this pull request as ready for review January 18, 2024 23:11
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@Andarist
Copy link
Contributor Author

I pushed out a test case that shows how this is an improvement. It fixes the VS Code's build failure that @jakebailey reported post-merge here.

It's obviously not perfect right now because ramdaToolsNoInfinite test case fails. As mentioned above, that would get fixed with #50034 (or an alternative fix to the underlying issue).

@Andarist Andarist marked this pull request as draft January 20, 2024 08:01
@Andarist
Copy link
Contributor Author

superseded by #57122

@Andarist Andarist closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Andarist @gabritto @typescript-bot