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

No this type arguments in base constraints #54536

Merged
merged 4 commits into from Jun 13, 2023
Merged

No this type arguments in base constraints #54536

merged 4 commits into from Jun 13, 2023

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Jun 5, 2023

This PR implements the following:

  • When a base constraint resolves to a type reference, we no longer add a this type argument to the reference. Instead, this type arguments are only added when obtaining apparent types.
  • When the target side of a type relation check is a conditional type, we ignore instantiations of the same conditional type on the source side (as we may otherwise generate a lot of useless work during variance computation).
  • When obtaining constraints for tuple types, we only perform substitutions for variadic elements only when the element references a type parameter (as opposed to an arbitrary generic type). This reduces instances of deeply recursive constraints.

With these changes, the repros in #54491 now type check faster than in 5.0.

Fixes #54491.
Fixes #54495.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 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 Jun 5, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 5, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 5, 2023

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 5, 2023

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 5, 2023

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 9d570a0. 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..54536

Metric main 54536 Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 365,512k (± 0.01%) 365,489k (± 0.01%) ~ 365,464k 365,548k p=0.296 n=6
Parse Time 3.55s (± 0.48%) 3.57s (± 0.65%) ~ 3.54s 3.60s p=0.123 n=6
Bind Time 1.18s (± 0.35%) 1.18s (± 0.35%) ~ 1.18s 1.19s p=0.218 n=6
Check Time 9.57s (± 0.65%) 9.65s (± 0.39%) +0.08s (+ 0.82%) 9.61s 9.70s p=0.037 n=6
Emit Time 7.88s (± 0.67%) 7.97s (± 1.11%) ~ 7.86s 8.08s p=0.109 n=6
Total Time 22.18s (± 0.42%) 22.37s (± 0.56%) ~ 22.25s 22.53s p=0.054 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 194,025k (± 0.89%) 192,584k (± 0.02%) -1,441k (- 0.74%) 192,522k 192,649k p=0.005 n=6
Parse Time 1.60s (± 0.32%) 1.61s (± 0.75%) ~ 1.59s 1.62s p=0.666 n=6
Bind Time 0.82s (± 0.77%) 0.83s (± 0.66%) ~ 0.82s 0.83s p=0.201 n=6
Check Time 10.15s (± 0.79%) 10.19s (± 0.57%) ~ 10.14s 10.30s p=0.229 n=6
Emit Time 3.01s (± 0.34%) 3.00s (± 0.60%) ~ 2.97s 3.02s p=0.211 n=6
Total Time 15.59s (± 0.50%) 15.62s (± 0.34%) ~ 15.58s 15.71s p=0.422 n=6
Monaco - node (v16.17.1, x64)
Memory used 345,888k (± 0.00%) 345,869k (± 0.00%) ~ 345,853k 345,893k p=0.065 n=6
Parse Time 2.73s (± 0.38%) 2.74s (± 0.44%) ~ 2.72s 2.75s p=0.675 n=6
Bind Time 1.09s (± 0.82%) 1.08s (± 0.75%) ~ 1.07s 1.09s p=0.270 n=6
Check Time 7.85s (± 0.78%) 7.90s (± 0.44%) ~ 7.86s 7.95s p=0.109 n=6
Emit Time 4.47s (± 0.90%) 4.48s (± 0.89%) ~ 4.43s 4.53s p=1.000 n=6
Total Time 16.14s (± 0.51%) 16.20s (± 0.33%) ~ 16.11s 16.27s p=0.297 n=6
TFS - node (v16.17.1, x64)
Memory used 299,962k (± 0.00%) 299,962k (± 0.01%) ~ 299,938k 300,008k p=0.423 n=6
Parse Time 2.16s (± 0.96%) 2.16s (± 0.70%) ~ 2.15s 2.19s p=0.933 n=6
Bind Time 1.23s (± 0.66%) 1.24s (± 1.18%) ~ 1.23s 1.26s p=0.498 n=6
Check Time 7.27s (± 0.68%) 7.31s (± 0.29%) ~ 7.28s 7.34s p=0.077 n=6
Emit Time 4.34s (± 0.60%) 4.37s (± 0.63%) ~ 4.33s 4.41s p=0.106 n=6
Total Time 15.00s (± 0.41%) 15.08s (± 0.32%) ~ 15.03s 15.17s p=0.054 n=6
material-ui - node (v16.17.1, x64)
Memory used 480,995k (± 0.01%) 480,338k (± 0.01%) -658k (- 0.14%) 480,297k 480,366k p=0.005 n=6
Parse Time 3.26s (± 0.17%) 3.26s (± 0.37%) ~ 3.24s 3.27s p=0.865 n=6
Bind Time 0.94s (± 0.67%) 0.94s (± 0.95%) ~ 0.93s 0.95s p=1.000 n=6
Check Time 17.89s (± 0.63%) 17.95s (± 0.91%) ~ 17.78s 18.19s p=0.748 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 22.08s (± 0.50%) 22.14s (± 0.75%) ~ 21.98s 22.38s p=0.810 n=6
xstate - node (v16.17.1, x64)
Memory used 560,684k (± 0.02%) 560,586k (± 0.01%) ~ 560,480k 560,675k p=0.173 n=6
Parse Time 3.99s (± 0.29%) 4.00s (± 0.19%) ~ 3.99s 4.01s p=0.157 n=6
Bind Time 1.76s (± 0.51%) 1.77s (± 0.80%) ~ 1.75s 1.79s p=0.215 n=6
Check Time 3.06s (± 0.82%) 3.06s (± 0.48%) ~ 3.04s 3.08s p=0.515 n=6
Emit Time 0.09s (± 0.00%) 0.09s (± 0.00%) ~ 0.09s 0.09s p=1.000 n=6
Total Time 8.90s (± 0.35%) 8.92s (± 0.21%) ~ 8.90s 8.94s p=0.334 n=6
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-148-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 54536 6
Baseline main 6

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

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

There were infrastructure failures potentially unrelated to your change:

  • 1 instance of "Unknown failure"
  • 1 instance of "Package install failed"

Otherwise...

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@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.

@Andarist
Copy link
Contributor

Andarist commented Jun 5, 2023

This PR also fixes #54495 , cc @alecgibson

@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 5, 2023
@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 5, 2023

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 9d570a0. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 5, 2023

Hey @DanielRosenwasser, 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/155350/artifacts?artifactName=tgz&fileId=25390837027CDB2F59440AFD227BEAC42D7474DF7E5DDBB45B82274982C27B3002&fileName=/typescript-5.2.0-insiders.20230605.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.2.0-pr-54536-13".;

@DanielRosenwasser
Copy link
Member

@typescript-bot user test tsserver
@typescript-bot test tsserver top100

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 5, 2023

Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at 9d570a0. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 5, 2023

Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at 9d570a0. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@jakebailey
Copy link
Member

Apparently this is blocking ts-eslint from upgrading to 5.1; this is intended to be cherry-picked, right? @DanielRosenwasser

@jakebailey
Copy link
Member

Scratch that, it's actually #54542 (comment)

!!! error TS2430: 'this' could be instantiated with an arbitrary type which could be unrelated to 'Set<T>'.
!!! error TS2430: 'Set<T>' is assignable to the constraint of type 'this', but 'this' could be instantiated with a different subtype of constraint 'Set<T>'.
Copy link
Member

Choose a reason for hiding this comment

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

Well this is definitely an improvement. 😄

# Conflicts:
#	tests/baselines/reference/complexRecursiveCollections.errors.txt
@ahejlsberg ahejlsberg merged commit 89cbea8 into main Jun 13, 2023
15 of 16 checks passed
@ahejlsberg ahejlsberg deleted the fix54491 branch June 13, 2023 02:07
@DanielRosenwasser
Copy link
Member

@typescript-bot cherry-pick this to release-5.1

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 13, 2023

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-5.1 on this PR at f0b25b3. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-5.1 manually.

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