Skip to content

Conversation

@ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Oct 11, 2022

Fixes #51043.

This PR also moves some intersection property check logic from isRelatedTo to structuredTypeRelatedTo (i.e. from the un-cached to the cached side of relationship checking). This allows us to get rid of a separate cache lookup and two associated IntersectionState flags.

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

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

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

Metric main 51140 Delta Best Worst
Angular - node (v16.17.1, x64)
Memory used 352,327k (± 0.02%) 352,403k (± 0.02%) +75k (+ 0.02%) 352,120k 352,509k
Parse Time 1.90s (± 0.77%) 1.91s (± 0.60%) +0.02s (+ 0.90%) 1.90s 1.95s
Bind Time 0.74s (± 0.40%) 0.75s (± 0.66%) +0.00s (+ 0.54%) 0.74s 0.76s
Check Time 5.67s (± 0.55%) 5.69s (± 0.63%) +0.02s (+ 0.39%) 5.63s 5.77s
Emit Time 6.06s (± 0.88%) 6.10s (± 0.59%) +0.04s (+ 0.71%) 6.03s 6.19s
Total Time 14.37s (± 0.60%) 14.46s (± 0.47%) +0.09s (+ 0.59%) 14.33s 14.65s
Compiler-Unions - node (v16.17.1, x64)
Memory used 198,639k (± 0.60%) 198,144k (± 0.49%) -495k (- 0.25%) 197,401k 200,827k
Parse Time 0.78s (± 0.79%) 0.79s (± 0.87%) +0.00s (+ 0.64%) 0.77s 0.80s
Bind Time 0.45s (± 0.89%) 0.46s (± 1.04%) +0.01s (+ 1.33%) 0.45s 0.47s
Check Time 6.49s (± 0.66%) 6.52s (± 0.51%) +0.03s (+ 0.51%) 6.47s 6.62s
Emit Time 2.31s (± 1.02%) 2.29s (± 0.68%) -0.02s (- 0.74%) 2.25s 2.33s
Total Time 10.03s (± 0.56%) 10.06s (± 0.36%) +0.03s (+ 0.27%) 10.00s 10.16s
Monaco - node (v16.17.1, x64)
Memory used 331,117k (± 0.01%) 331,138k (± 0.01%) +20k (+ 0.01%) 331,059k 331,215k
Parse Time 1.42s (± 0.71%) 1.43s (± 0.45%) +0.00s (+ 0.21%) 1.41s 1.44s
Bind Time 0.69s (± 1.09%) 0.70s (± 0.98%) +0.00s (+ 0.29%) 0.68s 0.71s
Check Time 5.47s (± 0.50%) 5.48s (± 0.46%) +0.01s (+ 0.16%) 5.43s 5.54s
Emit Time 3.25s (± 0.58%) 3.26s (± 0.52%) +0.01s (+ 0.40%) 3.24s 3.31s
Total Time 10.84s (± 0.39%) 10.87s (± 0.37%) +0.03s (+ 0.28%) 10.80s 10.96s
TFS - node (v16.17.1, x64)
Memory used 294,083k (± 0.01%) 294,042k (± 0.02%) -41k (- 0.01%) 293,880k 294,132k
Parse Time 1.21s (± 1.34%) 1.24s (± 1.23%) +0.02s (+ 1.98%) 1.21s 1.29s
Bind Time 0.64s (± 0.97%) 0.64s (± 0.62%) +0.00s (+ 0.31%) 0.63s 0.65s
Check Time 5.09s (± 0.39%) 5.13s (± 0.41%) +0.04s (+ 0.77%) 5.09s 5.19s
Emit Time 3.48s (± 0.57%) 3.48s (± 0.60%) +0.01s (+ 0.14%) 3.45s 3.54s
Total Time 10.42s (± 0.32%) 10.49s (± 0.46%) +0.07s (+ 0.66%) 10.39s 10.61s
material-ui - node (v16.17.1, x64)
Memory used 439,362k (± 0.02%) 439,371k (± 0.01%) +8k (+ 0.00%) 439,315k 439,567k
Parse Time 1.72s (± 1.08%) 1.74s (± 0.98%) +0.01s (+ 0.87%) 1.70s 1.78s
Bind Time 0.54s (± 0.74%) 0.54s (± 0.55%) +0.00s (+ 0.56%) 0.54s 0.55s
Check Time 12.59s (± 0.78%) 12.63s (± 1.05%) +0.04s (+ 0.31%) 12.49s 13.13s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.86s (± 0.62%) 14.91s (± 0.88%) +0.06s (+ 0.38%) 14.73s 15.39s
xstate - node (v16.17.1, x64)
Memory used 554,246k (± 0.01%) 554,428k (± 0.01%) +182k (+ 0.03%) 554,292k 554,615k
Parse Time 2.30s (± 0.24%) 2.30s (± 0.44%) +0.01s (+ 0.30%) 2.29s 2.33s
Bind Time 0.89s (± 2.09%) 0.89s (± 1.38%) +0.01s (+ 0.68%) 0.88s 0.94s
Check Time 1.43s (± 0.48%) 1.44s (± 0.93%) +0.02s (+ 1.33%) 1.40s 1.47s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 4.68s (± 0.32%) 4.71s (± 0.33%) +0.03s (+ 0.64%) 4.68s 4.74s
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-126-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 51140 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

@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 Oct 11, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at f0f853e. 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/51140/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..51140

Metric main 51140 Delta Best Worst
Angular - node (v16.17.1, x64)
Memory used 352,356k (± 0.02%) 352,340k (± 0.02%) -16k (- 0.00%) 352,159k 352,504k
Parse Time 1.90s (± 0.76%) 1.88s (± 0.62%) -0.01s (- 0.69%) 1.86s 1.91s
Bind Time 0.74s (± 0.50%) 0.75s (± 0.64%) +0.00s (+ 0.27%) 0.74s 0.76s
Check Time 5.69s (± 0.58%) 5.69s (± 0.46%) +0.00s (+ 0.02%) 5.65s 5.76s
Emit Time 6.08s (± 0.52%) 6.09s (± 0.58%) +0.01s (+ 0.21%) 6.02s 6.15s
Total Time 14.41s (± 0.37%) 14.41s (± 0.36%) +0.00s (+ 0.00%) 14.30s 14.51s
Compiler-Unions - node (v16.17.1, x64)
Memory used 198,243k (± 0.55%) 198,072k (± 0.48%) -171k (- 0.09%) 197,376k 200,648k
Parse Time 0.78s (± 0.71%) 0.79s (± 1.15%) +0.01s (+ 0.90%) 0.77s 0.81s
Bind Time 0.45s (± 0.75%) 0.46s (± 1.27%) +0.00s (+ 0.88%) 0.45s 0.47s
Check Time 6.50s (± 0.43%) 6.47s (± 1.01%) -0.03s (- 0.42%) 6.35s 6.60s
Emit Time 2.31s (± 0.72%) 2.31s (± 0.73%) +0.00s (+ 0.09%) 2.27s 2.36s
Total Time 10.04s (± 0.34%) 10.02s (± 0.72%) -0.02s (- 0.18%) 9.91s 10.18s
Monaco - node (v16.17.1, x64)
Memory used 331,137k (± 0.01%) 331,159k (± 0.01%) +22k (+ 0.01%) 331,071k 331,236k
Parse Time 1.43s (± 0.69%) 1.43s (± 0.58%) +0.00s (+ 0.21%) 1.42s 1.45s
Bind Time 0.69s (± 0.81%) 0.70s (± 0.98%) +0.01s (+ 1.16%) 0.68s 0.71s
Check Time 5.46s (± 0.56%) 5.48s (± 0.48%) +0.02s (+ 0.40%) 5.44s 5.55s
Emit Time 3.26s (± 0.53%) 3.27s (± 0.99%) +0.01s (+ 0.28%) 3.20s 3.35s
Total Time 10.84s (± 0.33%) 10.88s (± 0.52%) +0.04s (+ 0.36%) 10.77s 11.03s
TFS - node (v16.17.1, x64)
Memory used 294,097k (± 0.01%) 294,057k (± 0.02%) -40k (- 0.01%) 293,874k 294,113k
Parse Time 1.22s (± 1.33%) 1.22s (± 1.56%) -0.00s (- 0.08%) 1.18s 1.27s
Bind Time 0.64s (± 0.74%) 0.64s (± 0.78%) -0.01s (- 1.24%) 0.63s 0.65s
Check Time 5.12s (± 0.49%) 5.11s (± 0.38%) -0.01s (- 0.20%) 5.07s 5.15s
Emit Time 3.49s (± 0.35%) 3.47s (± 0.41%) -0.02s (- 0.52%) 3.45s 3.50s
Total Time 10.47s (± 0.46%) 10.44s (± 0.37%) -0.03s (- 0.31%) 10.34s 10.49s
material-ui - node (v16.17.1, x64)
Memory used 439,383k (± 0.02%) 439,072k (± 0.01%) -311k (- 0.07%) 438,961k 439,116k
Parse Time 1.75s (± 1.06%) 1.75s (± 0.78%) +0.01s (+ 0.34%) 1.71s 1.77s
Bind Time 0.54s (± 0.83%) 0.54s (± 0.74%) +0.00s (+ 0.19%) 0.53s 0.55s
Check Time 12.50s (± 0.29%) 12.55s (± 0.60%) +0.05s (+ 0.37%) 12.44s 12.81s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.79s (± 0.24%) 14.84s (± 0.54%) +0.05s (+ 0.36%) 14.72s 15.13s
xstate - node (v16.17.1, x64)
Memory used 554,341k (± 0.02%) 554,500k (± 0.02%) +159k (+ 0.03%) 554,343k 554,733k
Parse Time 2.30s (± 0.35%) 2.31s (± 0.36%) +0.01s (+ 0.39%) 2.30s 2.34s
Bind Time 0.89s (± 1.68%) 0.89s (± 1.65%) -0.00s (- 0.23%) 0.87s 0.94s
Check Time 1.43s (± 0.42%) 1.43s (± 0.82%) +0.00s (+ 0.35%) 1.40s 1.45s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 4.69s (± 0.47%) 4.70s (± 0.24%) +0.02s (+ 0.36%) 4.69s 4.74s
System
Machine Namets-ci-ubuntu
Platformlinux 5.4.0-126-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 51140 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

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

Everything looks good!

}

const isPerformingCommonPropertyChecks = (relation !== comparableRelation || !(source.flags & TypeFlags.Union) && isLiteralType(source)) &&
const isPerformingCommonPropertyChecks = (relation !== comparableRelation || isUnitType(source)) &&
Copy link
Member

Choose a reason for hiding this comment

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

I believe the problem with isUnitType is that it doesn't consider enum literal types.

https://github.com/microsoft/TypeScript/pull/50423/files#r954112029

So we won't catch assignability from an enum literal to a weak type with no overlap.

enum E { A = "A" }

// these will differ
let x: { nope?: any } = E.A;
let y: { nope?: any } = "A";

Copy link
Member Author

Choose a reason for hiding this comment

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

Enum literal types are definitely considered unit types. It's a bit subtle, isUnitType checks for TypeFlags.Unit, which includes TypeFlags.Literal, which includes TypeFlags.StringLiteral and TypeFlags.NumberLiteral. For enum literal types, the TypeFlags.EnumLiteral flag is always combined with one of the latter two, so isUnitType will always be true.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be sure, I verified both of your examples generate errors.

Copy link
Member

Choose a reason for hiding this comment

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

Great! Can you add that as a test case?

@ahejlsberg ahejlsberg merged commit 37317a2 into main Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: Team For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Combination of intersections/partials causes invalid nested object type to not report error

4 participants