No recursive intersection property checks

[ahejlsberg]

Fixes the first half of #37600. I have manually verified that the instantiation depth error is gone.

[weswigham]

This looks like a pretty straightforward change to me - do we know if it causes any measurable perf improvement in react-page? (Roughly?)

Aside: So I looked into the node 8 build failures (I saw this earlier today, but only just looked into it - I didn't for awhile because for about a week jobs were randomly failing on install across all versions with a hang in npm, which disappeared on rerun) - looks like they're because a dependency of playwright's install script requires node 10.15 or greater, and was picked up as a bugfix in playwright. Maybe this is an impetus to drop node 8 testing and pick up a newer node version.

[ahejlsberg]

Regarding perf, react-page check time drops from 3s to just under 1s and memory consumption drops in half, so dramatically better. That said, the cause really is the recursive types, and the best fix would be to simplify those.

[orta]

#37869 fixed the node 8 issues 👍

The discussion on dropping support for 8 is still reasonable to have

[ahejlsberg]

Some more data on the impact of this fix.

Compiling react-page with master:

Files:              191
Lines:            87660
Nodes:           295586
Identifiers:     105146
Symbols:         245477
Types:            51522
Instantiations:  274119
Memory used:    219966K
I/O read:         0.03s
I/O write:        0.00s
Parse time:       0.81s
Bind time:        0.33s
Check time:       3.10s
Emit time:        0.60s
Total time:       4.85s

Compiling fixed react-page with master:

Files:              191
Lines:            87660
Nodes:           295590
Identifiers:     105148
Symbols:         102597
Types:            16084
Instantiations:   67060
Memory used:    129625K
I/O read:         0.02s
I/O write:        0.00s
Parse time:       0.79s
Bind time:        0.33s
Check time:       1.18s
Emit time:        0.59s
Total time:       2.89s

Compiling react-page with this PR:

Files:              191
Lines:            87660
Nodes:           295586
Identifiers:     105146
Symbols:          86450
Types:            12290
Instantiations:   44560
Memory used:    125939K
I/O read:         0.02s
I/O write:        0.00s
Parse time:       0.82s
Bind time:        0.33s
Check time:       0.98s
Emit time:        0.67s
Total time:       2.79s

The fixed react-page has two minor edits of adding a missing type parameter (the fix suggested in #37600 (comment) needs to be applied to both ContentPluginProps and LayoutPluginProps).

With the fixed react-page we're still a bit slower on master than this PR, but also more correct (because we check recursively up to the depth limiter). I haven't seen other code bases that are appreciably slowed down by the recursive check, so I'm not really sure I want this PR vs. just fixing react-page.

[weswigham]

So... Adding the type parameter just ensures the type of the Component property is sufficiently similar that we quickly find a place to bail on the comparison (because we actually recur on the same type, rather than a variation of it); the fundamental type structure in play here is not uncommon - it's just a react higher order component, and a non-generic one, no less. The pattern is relatively common in react libraries. Knowing that the 3.9 release has the ability to double check times and introduce non-meaningful, unactionable errors into this common pattern, if not for very carefully consistent code where it previously didn't really matter... Might not be great.

[ahejlsberg]

Ok, we'll go with this PR then.

[amcasey]

Are there any drawbacks? This looks like a pure win.

[ahejlsberg]

Are there any drawbacks? This looks like a pure win.

The checking is slightly looser. For example:

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

x = z;  // No longer an error

We currently report an error on the example above. With this PR there is no error (as was the case before 3.9).