Increase selectivity of subtype relationship for signatures #35659

ahejlsberg · 2019-12-13T00:08:58Z

This PR introduces a new strict subtype relationship and uses this relationship in union subtype reduction. The new strict subtype relationship increases selectivity for signatures which in turn increases the stability of subtype reduction in union types. Specifically

the signature () => void is now always considered a strict subtype of (x?: string) => void (it previously was only for strictly checked function types, see Strict function types #18654), and
the signature (x: string | undefined) => void is now considered a strict subtype of (x?: string) => void.

Beyond these differences, the strict subtype relationship is the same as our existing subtype relationship. Ideally we'd use the new strict subtype relationship everywhere, but to reduce breaking changes we're only using it for union subtype reduction.

An example:

function f1(x: { f(): void }, y: { f(x?: string): void }) {
    let z = !!true ? x : y;
    z.f();
    z.f("hello");  // Previously error, now ok
}

function f2(x: { f(x: string | undefined): void }, y: { f(x?: string): void }) {
    let z = !!true ? x : y;
    z.f();         // Previously error, now ok
    z.f("hello");
}

Previously errors were reported in both functions above.

Fixes #35414.

weswigham · 2019-12-13T00:14:46Z

Do we likewise now consider (x?: string | undefined) => void to be a strict subtype of (x: string | undefined) => void?

ahejlsberg · 2019-12-13T00:30:53Z

@weswigham No, but it probably wouldn't be too hard to add.

ahejlsberg · 2019-12-13T00:34:45Z

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

typescript-bot · 2019-12-13T00:34:56Z

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 6d67054. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-12-13T00:34:56Z

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 6d67054. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-12-13T00:34:56Z

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 6d67054. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2019-12-13T00:56:29Z

Hm, I think I misspoke earlier, though - since (x?: string | undefined) => void is logically equivalent to (() => void) | ((x: string | undefined) => void), (x: string | undefined) => void is clearly a subtype of (x?: string | undefined) => void, not the other way around. In any case, that's only a small reformulation of the original report, so it's probably a good thing to make explicit.

typescript-bot · 2019-12-13T01:05:39Z

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

…) => void

ahejlsberg · 2019-12-14T22:58:05Z

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

typescript-bot · 2019-12-14T22:58:08Z

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at e60d7ff. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-12-14T22:58:08Z

Heya @ahejlsberg, I've started to run the extended test suite on this PR at e60d7ff. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-12-14T22:58:08Z

Heya @ahejlsberg, I've started to run the perf test suite on this PR at e60d7ff. You can monitor the build here. It should now contribute to this PR's status checks.

Update: The results are in!

typescript-bot · 2019-12-14T22:58:08Z

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at e60d7ff. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-12-14T23:21:59Z

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

Here they are:

Comparison Report - master..35659

Metric	master	35659	Delta	Best	Worst
Angular - node (v10.16.3, x64)
Memory used	354,570k (± 0.02%)	356,487k (± 0.02%)	+1,918k (+ 0.54%)	356,271k	356,649k
Parse Time	1.60s (± 0.51%)	1.64s (± 0.54%)	+0.03s (+ 2.06%)	1.62s	1.66s
Bind Time	0.86s (± 0.65%)	0.87s (± 1.02%)	+0.01s (+ 1.05%)	0.85s	0.88s
Check Time	4.50s (± 0.42%)	4.52s (± 0.66%)	+0.03s (+ 0.56%)	4.48s	4.59s
Emit Time	5.24s (± 1.10%)	5.22s (± 0.56%)	-0.02s (- 0.29%)	5.16s	5.28s
Total Time	12.20s (± 0.57%)	12.25s (± 0.46%)	+0.05s (+ 0.39%)	12.16s	12.35s
Monaco - node (v10.16.3, x64)
Memory used	366,164k (± 0.02%)	366,359k (± 0.02%)	+195k (+ 0.05%)	366,114k	366,457k
Parse Time	1.26s (± 0.74%)	1.26s (± 0.56%)	-0.00s (- 0.24%)	1.24s	1.27s
Bind Time	0.75s (± 0.69%)	0.75s (± 0.59%)	+0.00s (+ 0.13%)	0.74s	0.76s
Check Time	4.65s (± 0.61%)	4.68s (± 0.51%)	+0.03s (+ 0.56%)	4.64s	4.72s
Emit Time	2.93s (± 1.10%)	2.94s (± 0.65%)	+0.01s (+ 0.38%)	2.91s	2.99s
Total Time	9.59s (± 0.49%)	9.63s (± 0.37%)	+0.04s (+ 0.41%)	9.57s	9.72s
TFS - node (v10.16.3, x64)
Memory used	322,069k (± 0.02%)	322,086k (± 0.02%)	+17k (+ 0.01%)	321,861k	322,199k
Parse Time	0.95s (± 0.52%)	0.95s (± 0.76%)	-0.00s (- 0.31%)	0.94s	0.97s
Bind Time	0.72s (± 1.44%)	0.73s (± 1.61%)	+0.01s (+ 0.97%)	0.70s	0.76s
Check Time	4.15s (± 0.47%)	4.15s (± 0.71%)	+0.01s (+ 0.24%)	4.07s	4.20s
Emit Time	3.04s (± 0.43%)	3.06s (± 0.69%)	+0.02s (+ 0.66%)	3.01s	3.12s
Total Time	8.86s (± 0.37%)	8.90s (± 0.61%)	+0.04s (+ 0.39%)	8.73s	9.01s
Angular - node (v12.1.0, x64)
Memory used	330,177k (± 0.06%)	331,928k (± 0.07%)	+1,751k (+ 0.53%)	331,156k	332,212k
Parse Time	1.56s (± 0.47%)	1.58s (± 0.69%)	+0.02s (+ 1.28%)	1.55s	1.60s
Bind Time	0.84s (± 0.66%)	0.84s (± 0.35%)	+0.00s (+ 0.48%)	0.84s	0.85s
Check Time	4.44s (± 0.69%)	4.44s (± 0.41%)	+0.00s (+ 0.07%)	4.40s	4.49s
Emit Time	5.47s (± 1.07%)	5.45s (± 0.68%)	-0.02s (- 0.35%)	5.39s	5.57s
Total Time	12.31s (± 0.62%)	12.32s (± 0.42%)	+0.01s (+ 0.06%)	12.20s	12.43s
Monaco - node (v12.1.0, x64)
Memory used	345,894k (± 0.02%)	346,109k (± 0.01%)	+215k (+ 0.06%)	346,030k	346,164k
Parse Time	1.23s (± 0.67%)	1.22s (± 0.74%)	-0.01s (- 0.57%)	1.20s	1.25s
Bind Time	0.72s (± 0.51%)	0.72s (± 0.66%)	-0.01s (- 1.10%)	0.71s	0.73s
Check Time	4.50s (± 0.25%)	4.49s (± 0.42%)	-0.00s (- 0.11%)	4.45s	4.53s
Emit Time	2.99s (± 0.71%)	2.98s (± 0.48%)	-0.01s (- 0.23%)	2.95s	3.02s
Total Time	9.43s (± 0.23%)	9.41s (± 0.19%)	-0.02s (- 0.23%)	9.36s	9.43s
TFS - node (v12.1.0, x64)
Memory used	304,447k (± 0.02%)	304,450k (± 0.02%)	+3k (+ 0.00%)	304,314k	304,592k
Parse Time	0.95s (± 0.98%)	0.94s (± 0.72%)	-0.00s (- 0.42%)	0.93s	0.96s
Bind Time	0.68s (± 0.54%)	0.68s (± 0.66%)	+0.00s (+ 0.30%)	0.67s	0.69s
Check Time	4.06s (± 0.47%)	4.05s (± 0.61%)	-0.00s (- 0.02%)	3.98s	4.09s
Emit Time	3.04s (± 0.85%)	3.05s (± 0.55%)	+0.00s (+ 0.07%)	3.01s	3.07s
Total Time	8.72s (± 0.44%)	8.72s (± 0.35%)	+0.00s (+ 0.03%)	8.64s	8.80s
Angular - node (v8.9.0, x64)
Memory used	349,475k (± 0.02%)	351,262k (± 0.02%)	+1,788k (+ 0.51%)	351,164k	351,418k
Parse Time	2.10s (± 0.50%)	2.11s (± 0.56%)	+0.01s (+ 0.67%)	2.09s	2.14s
Bind Time	0.92s (± 0.54%)	0.91s (± 0.74%)	-0.01s (- 0.98%)	0.89s	0.92s
Check Time	5.29s (± 0.54%)	5.32s (± 0.50%)	+0.03s (+ 0.47%)	5.24s	5.37s
Emit Time	6.17s (± 1.04%)	6.22s (± 0.92%)	+0.05s (+ 0.88%)	6.03s	6.32s
Total Time	14.47s (± 0.58%)	14.56s (± 0.48%)	+0.08s (+ 0.59%)	14.35s	14.69s
Monaco - node (v8.9.0, x64)
Memory used	364,070k (± 0.01%)	364,245k (± 0.02%)	+175k (+ 0.05%)	364,126k	364,332k
Parse Time	1.56s (± 0.36%)	1.56s (± 0.40%)	-0.00s (- 0.19%)	1.55s	1.58s
Bind Time	0.93s (± 1.20%)	0.93s (± 0.70%)	+0.00s (+ 0.32%)	0.92s	0.94s
Check Time	5.50s (± 1.49%)	5.53s (± 1.25%)	+0.04s (+ 0.73%)	5.30s	5.63s
Emit Time	3.17s (± 4.83%)	3.06s (± 3.11%)	-0.10s (- 3.22%)	2.99s	3.44s
Total Time	11.15s (± 0.95%)	11.08s (± 0.55%)	-0.06s (- 0.56%)	10.96s	11.25s
TFS - node (v8.9.0, x64)
Memory used	321,129k (± 0.02%)	321,163k (± 0.01%)	+34k (+ 0.01%)	321,056k	321,238k
Parse Time	1.27s (± 0.52%)	1.26s (± 0.53%)	-0.01s (- 0.55%)	1.25s	1.28s
Bind Time	0.74s (± 0.65%)	0.74s (± 0.78%)	+0.00s (+ 0.41%)	0.73s	0.76s
Check Time	4.72s (± 0.58%)	4.72s (± 0.72%)	+0.00s (+ 0.06%)	4.67s	4.81s
Emit Time	3.19s (± 0.71%)	3.21s (± 0.76%)	+0.02s (+ 0.72%)	3.16s	3.26s
Total Time	9.91s (± 0.44%)	9.93s (± 0.56%)	+0.02s (+ 0.23%)	9.85s	10.08s
Angular - node (v8.9.0, x86)
Memory used	198,405k (± 0.01%)	199,314k (± 0.02%)	+909k (+ 0.46%)	199,245k	199,376k
Parse Time	2.03s (± 1.11%)	2.04s (± 0.56%)	+0.01s (+ 0.64%)	2.01s	2.06s
Bind Time	1.02s (± 0.89%)	1.02s (± 1.18%)	-0.00s (- 0.20%)	1.00s	1.06s
Check Time	4.83s (± 0.34%)	4.79s (± 0.74%)	-0.04s (- 0.89%)	4.70s	4.88s
Emit Time	6.06s (± 1.68%)	6.05s (± 1.21%)	-0.01s (- 0.23%)	5.92s	6.27s
Total Time	13.94s (± 0.90%)	13.90s (± 0.72%)	-0.05s (- 0.33%)	13.71s	14.22s
Monaco - node (v8.9.0, x86)
Memory used	203,987k (± 0.02%)	204,048k (± 0.01%)	+61k (+ 0.03%)	203,975k	204,127k
Parse Time	1.60s (± 0.55%)	1.60s (± 0.56%)	+0.00s (+ 0.12%)	1.59s	1.63s
Bind Time	0.75s (± 0.94%)	0.75s (± 0.66%)	-0.00s (- 0.40%)	0.74s	0.76s
Check Time	5.41s (± 0.83%)	5.44s (± 0.56%)	+0.03s (+ 0.59%)	5.36s	5.50s
Emit Time	2.89s (± 1.83%)	2.87s (± 1.36%)	-0.02s (- 0.62%)	2.82s	2.98s
Total Time	10.65s (± 0.62%)	10.66s (± 0.44%)	+0.01s (+ 0.09%)	10.60s	10.83s
TFS - node (v8.9.0, x86)
Memory used	181,022k (± 0.02%)	181,037k (± 0.02%)	+15k (+ 0.01%)	180,961k	181,130k
Parse Time	1.31s (± 0.67%)	1.31s (± 0.78%)	+0.00s (+ 0.08%)	1.30s	1.34s
Bind Time	0.70s (± 0.47%)	0.70s (± 1.27%)	-0.00s (- 0.43%)	0.69s	0.73s
Check Time	4.50s (± 0.52%)	4.49s (± 0.80%)	-0.01s (- 0.24%)	4.43s	4.58s
Emit Time	2.97s (± 1.13%)	2.95s (± 1.51%)	-0.02s (- 0.61%)	2.86s	3.07s
Total Time	9.48s (± 0.44%)	9.45s (± 0.68%)	-0.03s (- 0.35%)	9.30s	9.57s

System

Machine Name	ts-ci-ubuntu
Platform	linux 4.4.0-166-generic
Architecture	x64
Available Memory	16 GB
Available Memory	6 GB
CPUs	4 × 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)

Benchmark	Name	Iterations
Current	35659	10
Baseline	master	10

typescript-bot · 2019-12-14T23:26:34Z

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

ahejlsberg · 2019-12-16T18:42:58Z

@typescript-bot user test this

typescript-bot · 2019-12-16T18:43:04Z

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 7df76b6. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-12-16T19:12:56Z

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

ahejlsberg · 2019-12-17T01:32:23Z

Summary of impact on test suites:

RWC suites have breaks in Azure_Framework and Azure_Website, all related to union subtype reduction (or lack thereof) between a local Promise<T> and Q.Promise<T>. It's actually a bit surprising that we previously reduced to the local Promise<T> and we certainly wouldn't have in --strict mode.
Community test suites have breaks in lodash and uglify-js. Both are JavaScript code bases with JSDoc comments that already generate lots of errors. We now generate fewer errors overall, but a there are a couple of new ones. In general this PR improves our inferences.
DT test suites have a single break that appears unrelated to this PR.
Performance appears unaffected.

Overall I think the breaks are acceptable.

weswigham · 2019-12-17T21:30:35Z

src/compiler/checker.ts

@@ -15490,7 +15495,6 @@ namespace ts {
                    // to X. Failing both of those we want to check if the aggregation of A and B's members structurally
                    // relates to X. Thus, we include intersection types on the source side here.
                    if (source.flags & (TypeFlags.Object | TypeFlags.Intersection) && target.flags & TypeFlags.Object) {
-                        // Report structural errors only if we haven't reported any errors yet


Did you mean to remove this comment?

No, I'll put it back.

weswigham · 2019-12-17T21:34:15Z

src/compiler/checker.ts

+                    compareSignaturesRelated(targetSig!, sourceSig!, (checkMode & SignatureCheckMode.StrictArity) | (strictVariance ? SignatureCheckMode.StrictCallback : SignatureCheckMode.BivariantCallback), reportErrors, errorReporter, incompatibleErrorReporter, compareTypes, reportUnreliableMarkers) :
+                    !(checkMode & SignatureCheckMode.Callback) && !strictVariance && compareTypes(sourceType, targetType, /*reportErrors*/ false) || compareTypes(targetType, sourceType, reportErrors);
+                // With strict arity, (x: number | undefined) => void is a subtype of (x?: number | undefined) => void
+                if (related && checkMode & SignatureCheckMode.StrictArity && i >= getMinArgumentCount(source) && i < getMinArgumentCount(target) && isTypeIdenticalTo(sourceType, targetType)) {


What's the isTypeIdenticalTo check here for?

Without it a signature like (x?: string | undefined) => void wouldn't be a subtype of (x: string) => void. We want it to be such that we reduce a union to just (x: string) => void. It's basically a tiebreaker for positions with identical types but one with and one without the ? modifier.

...Mmm, but isTypeIdenticalTo doesn't seem quite right...? Shouldn't it be compareTypes(sourceType, targetType, /*reportErrors*/ false)? The difference, I think, should really only be visible, but from what I gather is important when callbacks with optional parameters are the parameter type of callbacks with optional parameters, eg (x?: (x?: string | undefined) => void) => void and (x: (x: string | undefined) => void) => void The first should be a subtype of the second because the second's parameter is a subtype of the first's.

Yeah, probably better to use compareTypes.

weswigham

We should probably add this cache to getRelationCacheSizes for diagnostic purposes, too.

ahejlsberg added 3 commits December 12, 2019 15:58

Increase selectivity of subtype relationship for signatures

b457de4

Add regression test

5524a61

Accept new baselines

6d67054

ahejlsberg mentioned this pull request Dec 13, 2019

Stable union subtype reduction for expression operators #35625

Closed

typescript-bot mentioned this pull request Dec 13, 2019

🤖 User test baselines have changed for fix35414-2 #35663

Closed

ahejlsberg added 3 commits December 14, 2019 08:36

Use strictSubtypeRelation for union subtype reduction

47f26e6

(x: number | undefined) -> void is subtype of (x?: number | undefined…

42c0451

…) => void

Accept new baselines

e60d7ff

typescript-bot mentioned this pull request Dec 14, 2019

🤖 User test baselines have changed for fix35414-2 #35684

Closed

Merge branch 'master' into fix35414-2

7df76b6

typescript-bot mentioned this pull request Dec 16, 2019

🤖 User test baselines have changed for fix35414-2 #35706

Closed

ahejlsberg added 2 commits December 17, 2019 13:09

Add tests

baac083

Accept new baselines

e0e45ec

weswigham reviewed Dec 17, 2019

View reviewed changes

weswigham requested changes Dec 17, 2019

View reviewed changes

Address CR feedback

8228888

DanielRosenwasser mentioned this pull request Dec 20, 2019

Design Meeting Notes, 12/20/2019 #35807

Closed

ahejlsberg added 2 commits December 20, 2019 14:30

Fix parameter list length check

273bb3e

Accept API baseline changes

4154dcb

ahejlsberg merged commit 2f0d07c into master Dec 20, 2019

sandersn mentioned this pull request Jan 9, 2020

🤖 User test baselines have changed for refs/heads/master #36046

Merged

Increase selectivity of subtype relationship for signatures #35659

Increase selectivity of subtype relationship for signatures #35659

Uh oh!

Conversation

ahejlsberg commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Dec 13, 2019

Uh oh!

ahejlsberg commented Dec 13, 2019

Uh oh!

ahejlsberg commented Dec 13, 2019

Uh oh!

typescript-bot commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Dec 13, 2019

Uh oh!

ahejlsberg commented Dec 14, 2019

Uh oh!

typescript-bot commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Dec 14, 2019

Uh oh!

typescript-bot commented Dec 14, 2019

Uh oh!

ahejlsberg commented Dec 16, 2019

Uh oh!

typescript-bot commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Dec 16, 2019

Uh oh!

ahejlsberg commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

ahejlsberg Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

weswigham Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

ahejlsberg Dec 19, 2019

Choose a reason for hiding this comment

Uh oh!

weswigham Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahejlsberg Dec 20, 2019

Choose a reason for hiding this comment

Uh oh!

weswigham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahejlsberg commented Dec 13, 2019 •

edited

Loading

typescript-bot commented Dec 13, 2019 •

edited

Loading

typescript-bot commented Dec 13, 2019 •

edited

Loading

typescript-bot commented Dec 13, 2019 •

edited

Loading

weswigham commented Dec 13, 2019 •

edited

Loading

typescript-bot commented Dec 14, 2019 •

edited

Loading

typescript-bot commented Dec 14, 2019 •

edited

Loading

typescript-bot commented Dec 14, 2019 •

edited

Loading

typescript-bot commented Dec 14, 2019 •

edited

Loading

typescript-bot commented Dec 16, 2019 •

edited

Loading

ahejlsberg commented Dec 17, 2019 •

edited

Loading

weswigham Dec 19, 2019 •

edited

Loading