Skip to content

Conversation

@ahejlsberg
Copy link
Member

We consider any intersection A & B assignable to a type T if either A or B is assignable to T. This can be problematic when T is an object type with optional properties. For example:

declareletx: {a?: number,b: string};declarelety: {a: null,b: string};declareletz: {a: null}&{b: string};x=y;// Errorx=z;// No error!

The issue here is that because {b: string } is assignable to {a?: number, b: string } we also consider any intersection that includes {b: string } to be assignable--even if the intersection includes another type that conflicts with the optional a property.

With this PR we now require intersections of object types as a whole to be assignable to the target type. In other words, we treat {a: null } &{b: string} identically to {a: null, b: string } in relationships.

Fixes#36604.

@ahejlsberg
Copy link
MemberAuthor

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 3, 2020

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 3, 2020

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 3, 2020

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 3, 2020

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

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..37195

Metricmaster37195DeltaBestWorst
Angular - node (v10.16.3, x64)
Memory used334,334k (± 0.05%)333,809k (± 0.03%)-525k (- 0.16%)333,521k334,026k
Parse Time1.62s (± 0.61%)1.63s (± 0.80%)+0.00s (+ 0.25%)1.60s1.65s
Bind Time0.89s (± 0.82%)0.90s (± 0.56%)+0.01s (+ 0.79%)0.89s0.91s
Check Time4.70s (± 0.53%)4.74s (± 0.47%)+0.04s (+ 0.85%)4.71s4.80s
Emit Time5.30s (± 0.48%)5.30s (± 0.62%)+0.00s (+ 0.08%)5.23s5.36s
Total Time12.51s (± 0.28%)12.57s (± 0.39%)+0.06s (+ 0.48%)12.48s12.67s
Monaco - node (v10.16.3, x64)
Memory used335,259k (± 0.01%)335,245k (± 0.02%)-14k (- 0.00%)335,082k335,358k
Parse Time1.25s (± 0.82%)1.26s (± 0.54%)+0.01s (+ 0.40%)1.24s1.27s
Bind Time0.78s (± 0.94%)0.78s (± 0.44%)-0.00s (- 0.13%)0.77s0.78s
Check Time4.72s (± 0.64%)4.71s (± 0.79%)-0.01s (- 0.13%)4.63s4.80s
Emit Time2.93s (± 0.71%)2.95s (± 1.02%)+0.03s (+ 0.85%)2.87s3.02s
Total Time9.67s (± 0.33%)9.70s (± 0.65%)+0.03s (+ 0.29%)9.57s9.81s
TFS - node (v10.16.3, x64)
Memory used299,484k (± 0.02%)299,485k (± 0.02%)+0k (+ 0.00%)299,340k299,611k
Parse Time0.94s (± 0.59%)0.94s (± 0.50%)-0.00s (- 0.21%)0.93s0.95s
Bind Time0.74s (± 0.49%)0.75s (± 0.69%)+0.01s (+ 0.94%)0.74s0.77s
Check Time4.27s (± 0.49%)4.28s (± 0.43%)+0.01s (+ 0.12%)4.25s4.32s
Emit Time3.03s (± 0.75%)3.06s (± 0.62%)+0.03s (+ 0.86%)3.02s3.10s
Total Time8.99s (± 0.37%)9.02s (± 0.36%)+0.04s (+ 0.40%)8.94s9.09s
material-ui - node (v10.16.3, x64)
Memory used488,881k (± 0.01%)489,335k (± 0.01%)+455k (+ 0.09%)489,193k489,441k
Parse Time1.77s (± 0.34%)1.77s (± 0.25%)-0.00s (- 0.17%)1.76s1.78s
Bind Time0.68s (± 0.76%)0.69s (± 0.69%)+0.01s (+ 0.88%)0.68s0.70s
Check Time13.62s (± 0.93%)13.64s (± 0.84%)+0.02s (+ 0.15%)13.47s13.95s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time16.08s (± 0.81%)16.10s (± 0.73%)+0.02s (+ 0.16%)15.92s16.42s
Angular - node (v12.1.0, x64)
Memory used309,968k (± 0.14%)309,625k (± 0.02%)-343k (- 0.11%)309,460k309,721k
Parse Time1.58s (± 0.53%)1.58s (± 0.48%)-0.00s (- 0.25%)1.56s1.59s
Bind Time0.87s (± 0.87%)0.87s (± 1.17%)+0.00s (+ 0.35%)0.85s0.89s
Check Time4.62s (± 0.69%)4.62s (± 0.51%)0.00s ( 0.00%)4.58s4.68s
Emit Time5.48s (± 1.25%)5.47s (± 0.82%)-0.01s (- 0.20%)5.38s5.58s
Total Time12.54s (± 0.75%)12.53s (± 0.45%)-0.01s (- 0.06%)12.40s12.64s
Monaco - node (v12.1.0, x64)
Memory used315,184k (± 0.01%)315,196k (± 0.02%)+12k (+ 0.00%)315,045k315,323k
Parse Time1.20s (± 0.40%)1.20s (± 0.79%)+0.00s (+ 0.08%)1.19s1.23s
Bind Time0.74s (± 1.10%)0.75s (± 1.27%)+0.00s (+ 0.40%)0.73s0.78s
Check Time4.55s (± 0.26%)4.56s (± 0.43%)+0.01s (+ 0.31%)4.53s4.61s
Emit Time2.96s (± 0.72%)2.95s (± 0.79%)-0.01s (- 0.27%)2.90s3.01s
Total Time9.46s (± 0.28%)9.47s (± 0.47%)+0.01s (+ 0.11%)9.39s9.60s
TFS - node (v12.1.0, x64)
Memory used281,780k (± 0.02%)281,777k (± 0.03%)-3k (- 0.00%)281,601k281,955k
Parse Time0.92s (± 0.79%)0.93s (± 0.82%)+0.00s (+ 0.33%)0.91s0.94s
Bind Time0.71s (± 0.97%)0.71s (± 0.81%)+0.00s (+ 0.71%)0.70s0.72s
Check Time4.18s (± 0.72%)4.20s (± 0.40%)+0.02s (+ 0.55%)4.17s4.25s
Emit Time3.08s (± 1.35%)3.09s (± 0.59%)+0.01s (+ 0.19%)3.05s3.13s
Total Time8.89s (± 0.57%)8.93s (± 0.30%)+0.04s (+ 0.45%)8.86s8.97s
material-ui - node (v12.1.0, x64)
Memory used466,312k (± 0.02%)466,828k (± 0.01%)+516k (+ 0.11%)466,730k466,905k
Parse Time1.74s (± 0.33%)1.76s (± 0.45%)+0.02s (+ 0.86%)1.74s1.78s
Bind Time0.62s (± 0.95%)0.63s (± 0.64%)+0.01s (+ 0.80%)0.62s0.64s
Check Time12.05s (± 0.54%)12.20s (± 0.80%)+0.15s (+ 1.26%)11.99s12.38s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time14.41s (± 0.46%)14.59s (± 0.69%)+0.17s (+ 1.20%)14.37s14.77s
Angular - node (v8.9.0, x64)
Memory used329,366k (± 0.01%)328,925k (± 0.02%)-441k (- 0.13%)328,754k329,071k
Parse Time2.11s (± 0.32%)2.10s (± 0.29%)-0.01s (- 0.28%)2.09s2.12s
Bind Time0.92s (± 0.87%)0.92s (± 0.43%)-0.00s (- 0.11%)0.91s0.93s
Check Time5.51s (± 0.44%)5.52s (± 0.82%)+0.00s (+ 0.04%)5.43s5.62s
Emit Time6.23s (± 0.90%)6.26s (± 1.03%)+0.03s (+ 0.42%)6.04s6.36s
Total Time14.78s (± 0.43%)14.80s (± 0.61%)+0.02s (+ 0.14%)14.61s15.01s
Monaco - node (v8.9.0, x64)
Memory used333,576k (± 0.02%)333,533k (± 0.01%)-43k (- 0.01%)333,456k333,625k
Parse Time1.54s (± 0.66%)1.54s (± 0.46%)+0.00s (+ 0.20%)1.53s1.56s
Bind Time0.90s (± 0.66%)0.90s (± 1.33%)+0.01s (+ 0.78%)0.87s0.93s
Check Time5.40s (± 0.60%)5.42s (± 0.61%)+0.02s (+ 0.31%)5.32s5.47s
Emit Time3.55s (± 0.56%)3.55s (± 0.37%)+0.01s (+ 0.17%)3.53s3.59s
Total Time11.38s (± 0.36%)11.41s (± 0.43%)+0.03s (+ 0.25%)11.31s11.54s
TFS - node (v8.9.0, x64)
Memory used298,881k (± 0.02%)298,895k (± 0.02%)+13k (+ 0.00%)298,777k299,060k
Parse Time1.25s (± 0.53%)1.26s (± 0.47%)+0.00s (+ 0.24%)1.24s1.27s
Bind Time0.75s (± 0.69%)0.75s (± 0.64%)-0.00s (- 0.53%)0.74s0.76s
Check Time4.83s (± 1.44%)4.82s (± 0.79%)-0.00s (- 0.08%)4.75s4.96s
Emit Time3.34s (± 1.92%)3.37s (± 1.36%)+0.03s (+ 0.84%)3.22s3.45s
Total Time10.17s (± 0.32%)10.20s (± 0.43%)+0.03s (+ 0.28%)10.08s10.29s
material-ui - node (v8.9.0, x64)
Memory used494,722k (± 0.01%)495,296k (± 0.01%)+574k (+ 0.12%)495,176k495,416k
Parse Time2.11s (± 0.53%)2.10s (± 0.81%)-0.01s (- 0.38%)2.07s2.13s
Bind Time0.82s (± 1.35%)0.82s (± 1.96%)+0.00s (+ 0.24%)0.78s0.86s
Check Time19.54s (± 0.47%)19.62s (± 0.54%)+0.08s (+ 0.41%)19.38s19.87s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time22.46s (± 0.47%)22.54s (± 0.50%)+0.08s (+ 0.36%)22.29s22.78s
Angular - node (v8.9.0, x86)
Memory used188,954k (± 0.03%)188,772k (± 0.02%)-182k (- 0.10%)188,696k188,862k
Parse Time2.05s (± 0.77%)2.06s (± 0.48%)+0.01s (+ 0.29%)2.04s2.08s
Bind Time1.07s (± 1.10%)1.07s (± 0.69%)+0.00s (+ 0.09%)1.05s1.08s
Check Time5.07s (± 0.95%)5.07s (± 0.40%)-0.00s (- 0.04%)5.03s5.12s
Emit Time6.19s (± 1.27%)6.21s (± 1.36%)+0.02s (+ 0.26%)6.09s6.50s
Total Time14.38s (± 0.92%)14.40s (± 0.55%)+0.02s (+ 0.17%)14.26s14.65s
Monaco - node (v8.9.0, x86)
Memory used189,214k (± 0.02%)189,226k (± 0.02%)+12k (+ 0.01%)189,146k189,290k
Parse Time1.58s (± 0.99%)1.59s (± 0.94%)+0.01s (+ 0.69%)1.57s1.63s
Bind Time0.77s (± 0.87%)0.77s (± 0.58%)+0.00s (+ 0.13%)0.76s0.78s
Check Time5.35s (± 1.51%)5.35s (± 1.78%)-0.00s (- 0.07%)5.11s5.49s
Emit Time3.01s (± 3.38%)3.02s (± 3.99%)+0.01s (+ 0.43%)2.83s3.29s
Total Time10.71s (± 0.33%)10.73s (± 0.54%)+0.02s (+ 0.18%)10.57s10.85s
TFS - node (v8.9.0, x86)
Memory used170,430k (± 0.02%)170,430k (± 0.03%)-0k (- 0.00%)170,316k170,510k
Parse Time1.28s (± 0.73%)1.28s (± 0.90%)+0.00s (+ 0.23%)1.27s1.31s
Bind Time0.72s (± 0.77%)0.72s (± 0.83%)-0.00s (- 0.28%)0.71s0.73s
Check Time4.62s (± 1.13%)4.64s (± 0.73%)+0.02s (+ 0.48%)4.57s4.73s
Emit Time2.99s (± 1.87%)2.97s (± 1.00%)-0.02s (- 0.60%)2.90s3.03s
Total Time9.61s (± 0.58%)9.60s (± 0.63%)-0.00s (- 0.01%)9.47s9.70s
material-ui - node (v8.9.0, x86)
Memory used277,168k (± 0.02%)277,417k (± 0.02%)+249k (+ 0.09%)277,320k277,496k
Parse Time2.17s (± 0.38%)2.18s (± 0.63%)+0.00s (+ 0.14%)2.14s2.21s
Bind Time0.69s (± 1.16%)0.68s (± 1.95%)-0.00s (- 0.58%)0.67s0.73s
Check Time17.72s (± 0.80%)17.70s (± 0.53%)-0.03s (- 0.16%)17.53s17.90s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time20.58s (± 0.69%)20.55s (± 0.48%)-0.03s (- 0.16%)20.37s20.75s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-166-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v8.9.0, x64)
  • material-ui - node (v8.9.0, x86)
BenchmarkNameIterations
Current3719510
Baselinemaster10

@ahejlsberg
Copy link
MemberAuthor

@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 4, 2020

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

@ahejlsberg
Copy link
MemberAuthor

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 4, 2020

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 1a5911a. You can monitor the build here.

@weswigham
Copy link
Member

So, notably, this does not fix #36637 because that has a generic T involved, right?

@ahejlsberg
Copy link
MemberAuthor

So, notably, this does not fix #36637 because that has a generic T involved, right?

That's right.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@ahejlsberg
Copy link
MemberAuthor

@weswigham Can you take a look at the last user test suite run and tell me which of those are preexisting conditions, or how I might find out? The office-ui-fabric-react is for sure a new issue, but I don't know about the rest.

@weswigham
Copy link
Member

Can you take a look at the last user test suite run and tell me which of those are preexisting conditions, or how I might find out? The office-ui-fabric-react is for sure a new issue, but I don't know about the rest.

Looking over #37219 and #37203 , looks like just the office-ui-fabric-react ones are new.

@ahejlsberg
Copy link
MemberAuthor

Summarizing test runs: No change in RWC and DT and no appreciable perf impact. New errors in the office-ui-fabric-react user test project are indeed legitimate errors that we didn't catch before.

So, this PR is technically a breaking change because we find new issues.

@ahejlsbergahejlsberg added the Breaking Change Would introduce errors in existing code label Mar 5, 2020
@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2020

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2020

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/67180/artifacts?artifactName=tgz&fileId=8A4B17A3BD0AA9909034921D0F3C3FBDFFB9B06D33543D6718C4F38A9CAA8A6602&fileName=/typescript-3.9.0-insiders.20200305.tgz" } } 

and then running npm install.


There is also a playground for this build.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 5, 2020

CC @dzearing@JasonGore, it looks like this change will cause issues for Fabric. It looks like tightening the declarations of

https://github.com/OfficeDev/office-ui-fabric-react/blob/e5c9f2bea83a4b9b0a82d303682c2ea093036c83/packages/fluentui/react-proptypes/src/index.ts#L436-L463

to use PropTypes.oneOf([...]) with the appropriate string literals for design will fix it. To try it out, I'd recommend using the above build.

@ahejlsbergahejlsberg merged commit b8baf48 into masterMar 14, 2020
@ahejlsbergahejlsberg deleted the fix36604 branch March 14, 2020 16:46
@kitsonkkitsonk mentioned this pull request May 13, 2020
@x87x87 mentioned this pull request May 14, 2020
@microsoftmicrosoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Breaking ChangeWould introduce errors in existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can assign types even though interface field overriden using Omit

5 participants

@ahejlsberg@typescript-bot@weswigham@DanielRosenwasser