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 assignment of intersections to objects with optional properties #37195

Merged
merged 8 commits into from
Mar 14, 2020

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:

declare let x: { a?: number, b: string };
declare let y: { a: null, b: string };
declare let z: { a: null } & { b: string };

x = y;  // Error
x = 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
Member Author

@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

Metric master 37195 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 334,334k (± 0.05%) 333,809k (± 0.03%) -525k (- 0.16%) 333,521k 334,026k
Parse Time 1.62s (± 0.61%) 1.63s (± 0.80%) +0.00s (+ 0.25%) 1.60s 1.65s
Bind Time 0.89s (± 0.82%) 0.90s (± 0.56%) +0.01s (+ 0.79%) 0.89s 0.91s
Check Time 4.70s (± 0.53%) 4.74s (± 0.47%) +0.04s (+ 0.85%) 4.71s 4.80s
Emit Time 5.30s (± 0.48%) 5.30s (± 0.62%) +0.00s (+ 0.08%) 5.23s 5.36s
Total Time 12.51s (± 0.28%) 12.57s (± 0.39%) +0.06s (+ 0.48%) 12.48s 12.67s
Monaco - node (v10.16.3, x64)
Memory used 335,259k (± 0.01%) 335,245k (± 0.02%) -14k (- 0.00%) 335,082k 335,358k
Parse Time 1.25s (± 0.82%) 1.26s (± 0.54%) +0.01s (+ 0.40%) 1.24s 1.27s
Bind Time 0.78s (± 0.94%) 0.78s (± 0.44%) -0.00s (- 0.13%) 0.77s 0.78s
Check Time 4.72s (± 0.64%) 4.71s (± 0.79%) -0.01s (- 0.13%) 4.63s 4.80s
Emit Time 2.93s (± 0.71%) 2.95s (± 1.02%) +0.03s (+ 0.85%) 2.87s 3.02s
Total Time 9.67s (± 0.33%) 9.70s (± 0.65%) +0.03s (+ 0.29%) 9.57s 9.81s
TFS - node (v10.16.3, x64)
Memory used 299,484k (± 0.02%) 299,485k (± 0.02%) +0k (+ 0.00%) 299,340k 299,611k
Parse Time 0.94s (± 0.59%) 0.94s (± 0.50%) -0.00s (- 0.21%) 0.93s 0.95s
Bind Time 0.74s (± 0.49%) 0.75s (± 0.69%) +0.01s (+ 0.94%) 0.74s 0.77s
Check Time 4.27s (± 0.49%) 4.28s (± 0.43%) +0.01s (+ 0.12%) 4.25s 4.32s
Emit Time 3.03s (± 0.75%) 3.06s (± 0.62%) +0.03s (+ 0.86%) 3.02s 3.10s
Total Time 8.99s (± 0.37%) 9.02s (± 0.36%) +0.04s (+ 0.40%) 8.94s 9.09s
material-ui - node (v10.16.3, x64)
Memory used 488,881k (± 0.01%) 489,335k (± 0.01%) +455k (+ 0.09%) 489,193k 489,441k
Parse Time 1.77s (± 0.34%) 1.77s (± 0.25%) -0.00s (- 0.17%) 1.76s 1.78s
Bind Time 0.68s (± 0.76%) 0.69s (± 0.69%) +0.01s (+ 0.88%) 0.68s 0.70s
Check Time 13.62s (± 0.93%) 13.64s (± 0.84%) +0.02s (+ 0.15%) 13.47s 13.95s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.08s (± 0.81%) 16.10s (± 0.73%) +0.02s (+ 0.16%) 15.92s 16.42s
Angular - node (v12.1.0, x64)
Memory used 309,968k (± 0.14%) 309,625k (± 0.02%) -343k (- 0.11%) 309,460k 309,721k
Parse Time 1.58s (± 0.53%) 1.58s (± 0.48%) -0.00s (- 0.25%) 1.56s 1.59s
Bind Time 0.87s (± 0.87%) 0.87s (± 1.17%) +0.00s (+ 0.35%) 0.85s 0.89s
Check Time 4.62s (± 0.69%) 4.62s (± 0.51%) 0.00s ( 0.00%) 4.58s 4.68s
Emit Time 5.48s (± 1.25%) 5.47s (± 0.82%) -0.01s (- 0.20%) 5.38s 5.58s
Total Time 12.54s (± 0.75%) 12.53s (± 0.45%) -0.01s (- 0.06%) 12.40s 12.64s
Monaco - node (v12.1.0, x64)
Memory used 315,184k (± 0.01%) 315,196k (± 0.02%) +12k (+ 0.00%) 315,045k 315,323k
Parse Time 1.20s (± 0.40%) 1.20s (± 0.79%) +0.00s (+ 0.08%) 1.19s 1.23s
Bind Time 0.74s (± 1.10%) 0.75s (± 1.27%) +0.00s (+ 0.40%) 0.73s 0.78s
Check Time 4.55s (± 0.26%) 4.56s (± 0.43%) +0.01s (+ 0.31%) 4.53s 4.61s
Emit Time 2.96s (± 0.72%) 2.95s (± 0.79%) -0.01s (- 0.27%) 2.90s 3.01s
Total Time 9.46s (± 0.28%) 9.47s (± 0.47%) +0.01s (+ 0.11%) 9.39s 9.60s
TFS - node (v12.1.0, x64)
Memory used 281,780k (± 0.02%) 281,777k (± 0.03%) -3k (- 0.00%) 281,601k 281,955k
Parse Time 0.92s (± 0.79%) 0.93s (± 0.82%) +0.00s (+ 0.33%) 0.91s 0.94s
Bind Time 0.71s (± 0.97%) 0.71s (± 0.81%) +0.00s (+ 0.71%) 0.70s 0.72s
Check Time 4.18s (± 0.72%) 4.20s (± 0.40%) +0.02s (+ 0.55%) 4.17s 4.25s
Emit Time 3.08s (± 1.35%) 3.09s (± 0.59%) +0.01s (+ 0.19%) 3.05s 3.13s
Total Time 8.89s (± 0.57%) 8.93s (± 0.30%) +0.04s (+ 0.45%) 8.86s 8.97s
material-ui - node (v12.1.0, x64)
Memory used 466,312k (± 0.02%) 466,828k (± 0.01%) +516k (+ 0.11%) 466,730k 466,905k
Parse Time 1.74s (± 0.33%) 1.76s (± 0.45%) +0.02s (+ 0.86%) 1.74s 1.78s
Bind Time 0.62s (± 0.95%) 0.63s (± 0.64%) +0.01s (+ 0.80%) 0.62s 0.64s
Check Time 12.05s (± 0.54%) 12.20s (± 0.80%) +0.15s (+ 1.26%) 11.99s 12.38s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.41s (± 0.46%) 14.59s (± 0.69%) +0.17s (+ 1.20%) 14.37s 14.77s
Angular - node (v8.9.0, x64)
Memory used 329,366k (± 0.01%) 328,925k (± 0.02%) -441k (- 0.13%) 328,754k 329,071k
Parse Time 2.11s (± 0.32%) 2.10s (± 0.29%) -0.01s (- 0.28%) 2.09s 2.12s
Bind Time 0.92s (± 0.87%) 0.92s (± 0.43%) -0.00s (- 0.11%) 0.91s 0.93s
Check Time 5.51s (± 0.44%) 5.52s (± 0.82%) +0.00s (+ 0.04%) 5.43s 5.62s
Emit Time 6.23s (± 0.90%) 6.26s (± 1.03%) +0.03s (+ 0.42%) 6.04s 6.36s
Total Time 14.78s (± 0.43%) 14.80s (± 0.61%) +0.02s (+ 0.14%) 14.61s 15.01s
Monaco - node (v8.9.0, x64)
Memory used 333,576k (± 0.02%) 333,533k (± 0.01%) -43k (- 0.01%) 333,456k 333,625k
Parse Time 1.54s (± 0.66%) 1.54s (± 0.46%) +0.00s (+ 0.20%) 1.53s 1.56s
Bind Time 0.90s (± 0.66%) 0.90s (± 1.33%) +0.01s (+ 0.78%) 0.87s 0.93s
Check Time 5.40s (± 0.60%) 5.42s (± 0.61%) +0.02s (+ 0.31%) 5.32s 5.47s
Emit Time 3.55s (± 0.56%) 3.55s (± 0.37%) +0.01s (+ 0.17%) 3.53s 3.59s
Total Time 11.38s (± 0.36%) 11.41s (± 0.43%) +0.03s (+ 0.25%) 11.31s 11.54s
TFS - node (v8.9.0, x64)
Memory used 298,881k (± 0.02%) 298,895k (± 0.02%) +13k (+ 0.00%) 298,777k 299,060k
Parse Time 1.25s (± 0.53%) 1.26s (± 0.47%) +0.00s (+ 0.24%) 1.24s 1.27s
Bind Time 0.75s (± 0.69%) 0.75s (± 0.64%) -0.00s (- 0.53%) 0.74s 0.76s
Check Time 4.83s (± 1.44%) 4.82s (± 0.79%) -0.00s (- 0.08%) 4.75s 4.96s
Emit Time 3.34s (± 1.92%) 3.37s (± 1.36%) +0.03s (+ 0.84%) 3.22s 3.45s
Total Time 10.17s (± 0.32%) 10.20s (± 0.43%) +0.03s (+ 0.28%) 10.08s 10.29s
material-ui - node (v8.9.0, x64)
Memory used 494,722k (± 0.01%) 495,296k (± 0.01%) +574k (+ 0.12%) 495,176k 495,416k
Parse Time 2.11s (± 0.53%) 2.10s (± 0.81%) -0.01s (- 0.38%) 2.07s 2.13s
Bind Time 0.82s (± 1.35%) 0.82s (± 1.96%) +0.00s (+ 0.24%) 0.78s 0.86s
Check Time 19.54s (± 0.47%) 19.62s (± 0.54%) +0.08s (+ 0.41%) 19.38s 19.87s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 22.46s (± 0.47%) 22.54s (± 0.50%) +0.08s (+ 0.36%) 22.29s 22.78s
Angular - node (v8.9.0, x86)
Memory used 188,954k (± 0.03%) 188,772k (± 0.02%) -182k (- 0.10%) 188,696k 188,862k
Parse Time 2.05s (± 0.77%) 2.06s (± 0.48%) +0.01s (+ 0.29%) 2.04s 2.08s
Bind Time 1.07s (± 1.10%) 1.07s (± 0.69%) +0.00s (+ 0.09%) 1.05s 1.08s
Check Time 5.07s (± 0.95%) 5.07s (± 0.40%) -0.00s (- 0.04%) 5.03s 5.12s
Emit Time 6.19s (± 1.27%) 6.21s (± 1.36%) +0.02s (+ 0.26%) 6.09s 6.50s
Total Time 14.38s (± 0.92%) 14.40s (± 0.55%) +0.02s (+ 0.17%) 14.26s 14.65s
Monaco - node (v8.9.0, x86)
Memory used 189,214k (± 0.02%) 189,226k (± 0.02%) +12k (+ 0.01%) 189,146k 189,290k
Parse Time 1.58s (± 0.99%) 1.59s (± 0.94%) +0.01s (+ 0.69%) 1.57s 1.63s
Bind Time 0.77s (± 0.87%) 0.77s (± 0.58%) +0.00s (+ 0.13%) 0.76s 0.78s
Check Time 5.35s (± 1.51%) 5.35s (± 1.78%) -0.00s (- 0.07%) 5.11s 5.49s
Emit Time 3.01s (± 3.38%) 3.02s (± 3.99%) +0.01s (+ 0.43%) 2.83s 3.29s
Total Time 10.71s (± 0.33%) 10.73s (± 0.54%) +0.02s (+ 0.18%) 10.57s 10.85s
TFS - node (v8.9.0, x86)
Memory used 170,430k (± 0.02%) 170,430k (± 0.03%) -0k (- 0.00%) 170,316k 170,510k
Parse Time 1.28s (± 0.73%) 1.28s (± 0.90%) +0.00s (+ 0.23%) 1.27s 1.31s
Bind Time 0.72s (± 0.77%) 0.72s (± 0.83%) -0.00s (- 0.28%) 0.71s 0.73s
Check Time 4.62s (± 1.13%) 4.64s (± 0.73%) +0.02s (+ 0.48%) 4.57s 4.73s
Emit Time 2.99s (± 1.87%) 2.97s (± 1.00%) -0.02s (- 0.60%) 2.90s 3.03s
Total Time 9.61s (± 0.58%) 9.60s (± 0.63%) -0.00s (- 0.01%) 9.47s 9.70s
material-ui - node (v8.9.0, x86)
Memory used 277,168k (± 0.02%) 277,417k (± 0.02%) +249k (+ 0.09%) 277,320k 277,496k
Parse Time 2.17s (± 0.38%) 2.18s (± 0.63%) +0.00s (+ 0.14%) 2.14s 2.21s
Bind Time 0.69s (± 1.16%) 0.68s (± 1.95%) -0.00s (- 0.58%) 0.67s 0.73s
Check Time 17.72s (± 0.80%) 17.70s (± 0.53%) -0.03s (- 0.16%) 17.53s 17.90s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 20.58s (± 0.69%) 20.55s (± 0.48%) -0.03s (- 0.16%) 20.37s 20.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)
Benchmark Name Iterations
Current 37195 10
Baseline master 10

@ahejlsberg
Copy link
Member Author

@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
Member Author

@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
Member Author

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
Member Author

@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
Member Author

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.

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

@ahejlsberg ahejlsberg merged commit b8baf48 into master Mar 14, 2020
@ahejlsberg ahejlsberg deleted the fix36604 branch March 14, 2020 16:46
@kitsonk kitsonk mentioned this pull request May 13, 2020
@x87 x87 mentioned this pull request May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would 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
4 participants