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

Fix isTypeDerivedFrom to properly handle {} and intersections #51631

Merged
merged 2 commits into from Nov 29, 2022
Merged

Conversation

ahejlsberg
Copy link
Member

Fixes #50844.
Fixes #51007.

@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 Nov 23, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 23, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 23, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 23, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 23, 2022

Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 8ccd6d5. 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/51631/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..51631

Metric main 51631 Delta Best Worst
Angular - node (v16.17.1, x64)
Memory used 340,640k (± 0.01%) 340,645k (± 0.02%) +5k (+ 0.00%) 340,444k 340,734k
Parse Time 1.90s (± 0.46%) 1.89s (± 0.64%) -0.02s (- 0.84%) 1.86s 1.91s
Bind Time 0.65s (± 0.56%) 0.65s (± 1.19%) -0.00s (- 0.46%) 0.64s 0.67s
Check Time 5.19s (± 0.44%) 5.16s (± 0.36%) -0.03s (- 0.58%) 5.13s 5.20s
Emit Time 5.12s (± 0.79%) 5.08s (± 0.72%) -0.04s (- 0.74%) 5.00s 5.17s
Total Time 12.87s (± 0.27%) 12.78s (± 0.35%) -0.09s (- 0.70%) 12.71s 12.88s
Compiler-Unions - node (v16.17.1, x64)
Memory used 187,005k (± 0.39%) 187,371k (± 0.52%) +366k (+ 0.20%) 186,629k 190,034k
Parse Time 0.80s (± 0.37%) 0.79s (± 0.84%) -0.01s (- 1.00%) 0.78s 0.81s
Bind Time 0.42s (± 0.70%) 0.42s (± 0.79%) -0.00s (- 0.47%) 0.41s 0.43s
Check Time 6.12s (± 0.36%) 6.09s (± 0.59%) -0.03s (- 0.46%) 6.04s 6.22s
Emit Time 1.93s (± 0.94%) 1.94s (± 0.80%) +0.01s (+ 0.52%) 1.92s 1.99s
Total Time 9.27s (± 0.34%) 9.24s (± 0.49%) -0.03s (- 0.35%) 9.18s 9.39s
Monaco - node (v16.17.1, x64)
Memory used 319,882k (± 0.05%) 319,818k (± 0.01%) -64k (- 0.02%) 319,761k 319,881k
Parse Time 1.44s (± 0.73%) 1.42s (± 0.64%) -0.02s (- 1.52%) 1.40s 1.44s
Bind Time 0.60s (± 0.87%) 0.59s (± 0.56%) -0.01s (- 1.50%) 0.58s 0.60s
Check Time 4.88s (± 0.39%) 4.87s (± 0.38%) -0.01s (- 0.29%) 4.81s 4.90s
Emit Time 2.75s (± 0.50%) 2.72s (± 0.81%) -0.03s (- 1.23%) 2.68s 2.78s
Total Time 9.68s (± 0.32%) 9.60s (± 0.35%) -0.08s (- 0.79%) 9.54s 9.67s
TFS - node (v16.17.1, x64)
Memory used 282,275k (± 0.01%) 282,302k (± 0.01%) +27k (+ 0.01%) 282,259k 282,360k
Parse Time 1.18s (± 1.09%) 1.16s (± 0.77%) -0.02s (- 1.36%) 1.15s 1.19s
Bind Time 0.65s (± 3.54%) 0.66s (± 3.27%) +0.02s (+ 2.48%) 0.60s 0.70s
Check Time 4.77s (± 0.27%) 4.76s (± 0.43%) -0.01s (- 0.29%) 4.71s 4.80s
Emit Time 2.75s (± 2.05%) 2.72s (± 2.07%) -0.03s (- 0.98%) 2.65s 2.86s
Total Time 9.35s (± 0.80%) 9.31s (± 0.74%) -0.04s (- 0.46%) 9.20s 9.51s
material-ui - node (v16.17.1, x64)
Memory used 435,253k (± 0.00%) 435,273k (± 0.00%) +21k (+ 0.00%) 435,252k 435,323k
Parse Time 1.65s (± 0.37%) 1.64s (± 0.58%) -0.02s (- 0.91%) 1.62s 1.67s
Bind Time 0.51s (± 0.72%) 0.50s (± 0.68%) -0.00s (- 0.59%) 0.50s 0.51s
Check Time 11.98s (± 0.41%) 11.83s (± 0.72%) -0.14s (- 1.20%) 11.64s 11.96s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.14s (± 0.35%) 13.97s (± 0.67%) -0.17s (- 1.18%) 13.76s 14.10s
xstate - node (v16.17.1, x64)
Memory used 515,991k (± 0.01%) 516,073k (± 0.01%) +82k (+ 0.02%) 515,975k 516,215k
Parse Time 2.32s (± 0.40%) 2.31s (± 0.62%) -0.01s (- 0.34%) 2.27s 2.34s
Bind Time 0.84s (± 1.38%) 0.84s (± 1.91%) -0.00s (- 0.12%) 0.81s 0.89s
Check Time 1.37s (± 0.41%) 1.35s (± 0.65%) -0.02s (- 1.10%) 1.33s 1.37s
Emit Time 0.06s (± 0.00%) 0.06s (± 0.00%) 0.00s ( 0.00%) 0.06s 0.06s
Total Time 4.59s (± 0.24%) 4.57s (± 0.72%) -0.02s (- 0.48%) 4.51s 4.66s
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-131-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 51631 10
Baseline main 10

Developer Information:

Download Benchmark

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

// S is a type variable with a base constraint that is derived from T,
// S is an intersection type and some constituent of S is derived from T, or
// S is a type variable with a base constraint that is derived from T, or
// T is {} and S is an object-like type (ensuring {} is less derived than Object), or
Copy link
Contributor

Choose a reason for hiding this comment

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

q: what does "less derived" mean? how something can be more or less derived?

Copy link
Member Author

Choose a reason for hiding this comment

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

It means that any object-like type, including Object, appears to be derived from {}. The effect of this addition is that x instanceof Object will narrow {} to Object, which in turn means that x qualifies as the right hand operand of an in check.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably reword this like this then:

Suggested change
// T is {} and S is an object-like type (ensuring {} is less derived than Object), or
// T is {} and S is an object-like type (ensuring that object-likes, including Object, are derived from {}), or

Maybe I'm just missing the appropriate background and lingo but this explanation is more readable to me.

Copy link

Choose a reason for hiding this comment

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

In the context of type theory, "less derived" is definitely a thing. e.g. Animal is less derived than Dog which is less derived than PitBull, while Cat and Dog are equally derived, being sister types. These relationships are obvious under nominal typing, but probably get pretty hairy under structural typing.

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

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