Skip to content

Treat trailing 'void' as optional for assignability - #40231

Merged
Ron Buckton (rbuckton) merged 1 commit into
masterfrom
optionalVoidAssignability
Aug 28, 2020
Merged

Treat trailing 'void' as optional for assignability#40231
Ron Buckton (rbuckton) merged 1 commit into
masterfrom
optionalVoidAssignability

Conversation

@rbuckton

@rbucktonRon Buckton (rbuckton) commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

This adds checking for void types in getMinArgumentCount so that we not only treat void params as optional for function calls, but also for assignability.

This does not yet support the recent addition of treating unknown, any, or undefined as optional in JS files as there is no Node available that can be used as the context for the assignment to know whether we are currently in a JS file.

Related #36749
Fixes#40227

This does not address #29131, as that issue will require a separate change in overload resolution.

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

Heya Ron Buckton (@rbuckton), I've started to run the perf test suite on this PR at 3bbbe28. You can monitor the build here.

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

Heya Ron Buckton (@rbuckton), I've started to run the extended test suite on this PR at 3bbbe28. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

Heya Ron Buckton (@rbuckton), I've started to run the parallelized Definitely Typed test suite on this PR at 3bbbe28. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

Heya Ron Buckton (@rbuckton), I've started to run the parallelized community code test suite on this PR at 3bbbe28. You can monitor the build here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Ron Buckton (@rbuckton)
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..40231

Metricmaster40231DeltaBestWorst
Angular - node (v10.16.3, x64)
Memory used344,453k (± 0.02%)344,572k (± 0.02%)+118k (+ 0.03%)344,464k344,700k
Parse Time2.02s (± 0.58%)1.99s (± 0.37%)-0.03s (- 1.29%)1.98s2.01s
Bind Time0.83s (± 1.89%)0.82s (± 1.13%)-0.00s (- 0.48%)0.80s0.84s
Check Time4.80s (± 0.62%)4.77s (± 0.45%)-0.02s (- 0.48%)4.73s4.84s
Emit Time5.18s (± 0.68%)5.14s (± 0.50%)-0.04s (- 0.71%)5.10s5.22s
Total Time12.82s (± 0.54%)12.73s (± 0.25%)-0.09s (- 0.73%)12.68s12.81s
Monaco - node (v10.16.3, x64)
Memory used339,280k (± 0.04%)339,336k (± 0.04%)+55k (+ 0.02%)339,100k339,576k
Parse Time1.56s (± 0.89%)1.55s (± 0.29%)-0.01s (- 0.51%)1.54s1.56s
Bind Time0.71s (± 0.31%)0.71s (± 0.52%)+0.00s (+ 0.56%)0.71s0.72s
Check Time4.95s (± 0.71%)4.96s (± 0.72%)+0.01s (+ 0.14%)4.88s5.04s
Emit Time2.74s (± 1.08%)2.72s (± 0.56%)-0.02s (- 0.69%)2.69s2.76s
Total Time9.96s (± 0.67%)9.95s (± 0.52%)-0.02s (- 0.15%)9.84s10.04s
TFS - node (v10.16.3, x64)
Memory used302,273k (± 0.03%)302,350k (± 0.01%)+77k (+ 0.03%)302,248k302,472k
Parse Time1.21s (± 0.56%)1.21s (± 0.82%)+0.01s (+ 0.58%)1.19s1.24s
Bind Time0.67s (± 1.04%)0.67s (± 0.83%)-0.00s (- 0.45%)0.65s0.68s
Check Time4.44s (± 0.83%)4.47s (± 0.56%)+0.03s (+ 0.61%)4.41s4.54s
Emit Time2.92s (± 0.72%)2.89s (± 1.10%)-0.03s (- 1.03%)2.80s2.94s
Total Time9.23s (± 0.65%)9.24s (± 0.46%)+0.01s (+ 0.06%)9.15s9.30s
material-ui - node (v10.16.3, x64)
Memory used460,841k (± 0.01%)461,299k (± 0.01%)+458k (+ 0.10%)461,184k461,387k
Parse Time1.95s (± 0.30%)1.95s (± 0.45%)+0.00s (+ 0.00%)1.93s1.97s
Bind Time0.66s (± 1.10%)0.66s (± 0.98%)-0.00s (- 0.60%)0.64s0.67s
Check Time13.51s (± 0.84%)13.47s (± 0.86%)-0.05s (- 0.33%)13.24s13.74s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time16.13s (± 0.72%)16.08s (± 0.71%)-0.05s (- 0.29%)15.85s16.34s
Angular - node (v12.1.0, x64)
Memory used321,740k (± 0.03%)321,601k (± 0.09%)-139k (- 0.04%)320,497k321,908k
Parse Time1.99s (± 0.56%)2.00s (± 0.63%)+0.00s (+ 0.10%)1.96s2.02s
Bind Time0.81s (± 0.61%)0.80s (± 0.45%)-0.00s (- 0.25%)0.80s0.81s
Check Time4.68s (± 0.56%)4.70s (± 0.37%)+0.02s (+ 0.51%)4.67s4.75s
Emit Time5.36s (± 0.61%)5.35s (± 0.70%)-0.01s (- 0.11%)5.29s5.48s
Total Time12.83s (± 0.45%)12.85s (± 0.44%)+0.01s (+ 0.12%)12.76s13.05s
Monaco - node (v12.1.0, x64)
Memory used321,551k (± 0.01%)321,613k (± 0.01%)+62k (+ 0.02%)321,460k321,684k
Parse Time1.53s (± 0.74%)1.53s (± 0.99%)+0.00s (+ 0.07%)1.50s1.56s
Bind Time0.69s (± 0.81%)0.69s (± 0.75%)+0.00s (+ 0.15%)0.68s0.70s
Check Time4.74s (± 0.30%)4.77s (± 0.64%)+0.03s (+ 0.68%)4.72s4.86s
Emit Time2.79s (± 0.66%)2.81s (± 0.87%)+0.02s (+ 0.86%)2.74s2.86s
Total Time9.75s (± 0.42%)9.80s (± 0.49%)+0.06s (+ 0.57%)9.66s9.89s
TFS - node (v12.1.0, x64)
Memory used286,603k (± 0.03%)286,658k (± 0.03%)+55k (+ 0.02%)286,467k286,762k
Parse Time1.23s (± 1.05%)1.23s (± 0.78%)-0.00s (- 0.08%)1.21s1.25s
Bind Time0.64s (± 0.87%)0.64s (± 0.74%)+0.01s (+ 0.78%)0.63s0.65s
Check Time4.36s (± 0.48%)4.36s (± 0.31%)+0.01s (+ 0.16%)4.33s4.39s
Emit Time2.93s (± 0.62%)2.92s (± 0.58%)-0.01s (- 0.38%)2.89s2.96s
Total Time9.15s (± 0.36%)9.15s (± 0.24%)-0.00s (- 0.01%)9.09s9.18s
material-ui - node (v12.1.0, x64)
Memory used439,051k (± 0.07%)439,251k (± 0.09%)+199k (+ 0.05%)438,196k439,782k
Parse Time1.99s (± 0.48%)1.97s (± 0.48%)-0.01s (- 0.65%)1.95s1.99s
Bind Time0.64s (± 0.70%)0.63s (± 0.76%)-0.01s (- 1.72%)0.62s0.64s
Check Time12.02s (± 0.80%)12.08s (± 0.92%)+0.06s (+ 0.52%)11.88s12.30s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time14.64s (± 0.69%)14.68s (± 0.76%)+0.04s (+ 0.27%)14.46s14.88s
Angular - node (v8.9.0, x64)
Memory used341,072k (± 0.01%)341,071k (± 0.03%)-1k (- 0.00%)340,933k341,297k
Parse Time2.55s (± 0.51%)2.54s (± 0.42%)-0.00s (- 0.20%)2.52s2.56s
Bind Time0.86s (± 1.34%)0.85s (± 0.89%)-0.01s (- 0.70%)0.84s0.87s
Check Time5.40s (± 0.89%)5.41s (± 0.68%)+0.01s (+ 0.24%)5.34s5.51s
Emit Time6.06s (± 1.48%)5.98s (± 2.04%)-0.08s (- 1.35%)5.68s6.20s
Total Time14.86s (± 0.72%)14.79s (± 1.00%)-0.08s (- 0.51%)14.48s15.09s
Monaco - node (v8.9.0, x64)
Memory used340,580k (± 0.01%)340,664k (± 0.01%)+83k (+ 0.02%)340,549k340,753k
Parse Time1.87s (± 0.56%)1.87s (± 0.57%)+0.00s (+ 0.05%)1.86s1.90s
Bind Time0.88s (± 0.53%)0.88s (± 0.54%)+0.00s (+ 0.34%)0.87s0.89s
Check Time5.48s (± 0.60%)5.51s (± 0.65%)+0.03s (+ 0.53%)5.43s5.63s
Emit Time3.19s (± 0.50%)3.22s (± 0.61%)+0.03s (+ 1.00%)3.18s3.27s
Total Time11.42s (± 0.32%)11.49s (± 0.35%)+0.06s (+ 0.56%)11.35s11.55s
TFS - node (v8.9.0, x64)
Memory used303,884k (± 0.02%)303,969k (± 0.02%)+85k (+ 0.03%)303,816k304,100k
Parse Time1.54s (± 0.45%)1.55s (± 0.48%)+0.01s (+ 0.39%)1.53s1.56s
Bind Time0.67s (± 0.86%)0.67s (± 0.73%)+0.00s (+ 0.15%)0.66s0.68s
Check Time5.20s (± 0.56%)5.21s (± 0.50%)+0.01s (+ 0.19%)5.16s5.27s
Emit Time2.93s (± 0.63%)2.93s (± 0.92%)+0.01s (+ 0.21%)2.85s2.99s
Total Time10.33s (± 0.37%)10.36s (± 0.50%)+0.02s (+ 0.21%)10.23s10.49s
material-ui - node (v8.9.0, x64)
Memory used465,199k (± 0.01%)465,640k (± 0.02%)+441k (+ 0.09%)465,482k465,876k
Parse Time2.40s (± 0.46%)2.38s (± 0.43%)-0.02s (- 0.67%)2.36s2.40s
Bind Time0.77s (± 1.07%)0.78s (± 1.16%)+0.01s (+ 0.65%)0.76s0.80s
Check Time18.05s (± 0.77%)18.07s (± 1.13%)+0.02s (+ 0.13%)17.72s18.49s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time21.22s (± 0.68%)21.23s (± 1.01%)+0.01s (+ 0.05%)20.86s21.69s
Angular - node (v8.9.0, x86)
Memory used195,727k (± 0.01%)195,749k (± 0.02%)+22k (+ 0.01%)195,678k195,819k
Parse Time2.46s (± 0.56%)2.46s (± 0.55%)-0.00s (- 0.20%)2.42s2.48s
Bind Time1.00s (± 0.80%)1.00s (± 1.14%)-0.00s (- 0.40%)0.98s1.03s
Check Time4.89s (± 0.52%)4.92s (± 0.60%)+0.03s (+ 0.55%)4.85s4.99s
Emit Time5.95s (± 0.90%)5.91s (± 1.02%)-0.03s (- 0.57%)5.84s6.13s
Total Time14.30s (± 0.47%)14.29s (± 0.54%)-0.02s (- 0.12%)14.15s14.50s
Monaco - node (v8.9.0, x86)
Memory used193,625k (± 0.02%)193,680k (± 0.03%)+55k (+ 0.03%)193,538k193,775k
Parse Time1.92s (± 0.78%)1.90s (± 0.90%)-0.01s (- 0.78%)1.87s1.95s
Bind Time0.70s (± 0.95%)0.70s (± 0.92%)+0.00s (+ 0.29%)0.69s0.72s
Check Time5.57s (± 0.59%)5.59s (± 0.64%)+0.02s (+ 0.31%)5.49s5.66s
Emit Time2.67s (± 0.92%)2.67s (± 0.70%)-0.00s (- 0.04%)2.63s2.72s
Total Time10.86s (± 0.51%)10.87s (± 0.52%)+0.00s (+ 0.04%)10.70s10.99s
TFS - node (v8.9.0, x86)
Memory used173,848k (± 0.03%)173,897k (± 0.02%)+49k (+ 0.03%)173,807k173,952k
Parse Time1.59s (± 0.84%)1.60s (± 0.91%)+0.01s (+ 0.63%)1.56s1.62s
Bind Time0.64s (± 0.62%)0.65s (± 1.18%)+0.00s (+ 0.62%)0.63s0.67s
Check Time4.72s (± 0.60%)4.73s (± 0.81%)+0.01s (+ 0.23%)4.64s4.82s
Emit Time2.80s (± 1.20%)2.78s (± 1.04%)-0.02s (- 0.61%)2.71s2.85s
Total Time9.75s (± 0.48%)9.76s (± 0.72%)+0.01s (+ 0.08%)9.58s9.94s
material-ui - node (v8.9.0, x86)
Memory used263,437k (± 0.01%)263,689k (± 0.03%)+253k (+ 0.10%)263,563k263,857k
Parse Time2.45s (± 0.68%)2.44s (± 0.67%)-0.01s (- 0.37%)2.41s2.49s
Bind Time0.67s (± 0.92%)0.68s (± 1.30%)+0.01s (+ 1.20%)0.66s0.70s
Check Time16.47s (± 0.76%)16.40s (± 0.66%)-0.07s (- 0.44%)16.16s16.62s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time19.59s (± 0.68%)19.52s (± 0.57%)-0.07s (- 0.38%)19.26s19.76s
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
Current4023110
Baselinemaster10

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

TypeScript Bot (@typescript-bot) test this

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 25, 2020

Copy link
Copy Markdown
Contributor

Heya Ron Buckton (@rbuckton), I've started to run the extended test suite on this PR at a8e319b. You can monitor the build here.

>inputARand : ArrayLike<A> | Iterable<A>
>getEither(inputA, inputALike) : ArrayLike<A> | Iterable<A>
>getEither : <T>(in1: Iterable<T>, in2: ArrayLike<T>) => Iterable<T> | ArrayLike<T>
>getEither : <T>(in1: Iterable<T>, in2: ArrayLike<T>) => ArrayLike<T> | Iterable<T>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What’s this about?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Its likely due to the fact that we now compare a signature with a trailing void that we would have previously skipped because the minArgumentCount didn't match, and as a result we encounter ArrayLike during type checking earlier than we do Iterable. As a result, ArrayLike ends up with an earlier type id and thus its order in the union changes.

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

I'll need to fix the DT tests for es-abstract, as it has an // $ExpectType for the type of Promise which is now wrong because the order of overloads for the static resolve method changed in the lib (which is required to pick the correct overload in some cases).

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

TypeScript Bot (@typescript-bot) user test this

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 26, 2020

Copy link
Copy Markdown
Contributor

Heya Ron Buckton (@rbuckton), I've started to run the parallelized community code test suite on this PR at 3bbbe28. You can monitor the build here.

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

Wesley Wigham (@weswigham) the Parallel community tests are failing to create a PR for me to review. I know this normally happens if the PR already exists, but I can't find any PR created for this under https://github.com/typescript-bot/TypeScript/pulls. It's failing here: https://typescript.visualstudio.com/TypeScript/_build/results?buildId=83325&view=logs&j=275f1d19-1bd8-5591-b06b-07d489ea915a&t=d6d1571a-a17f-534d-f3cd-109ff09fc86e&l=33

@weswigham

Copy link
Copy Markdown
Member

Looks like the commit fails - it most commonly fails if there's no diff to commit. If there's no diff to commit, but it's still failing, it's usually because it had a crash, which is reported somewhere in the build log. Except, looking at the build log, it looks to me like there should be some diffs (some logs definitely changed, though the dockerfile tests seem to be failing on building a tarball of the compiler itself, which could indicate an issue with LKG), so it's possible the commit failed for another reason; but whatever it was, it wasn't reported on stderr...

@ExE-Boss

ExE Boss (ExE-Boss) commented Aug 26, 2020

Copy link
Copy Markdown
Contributor

Ron Buckton (@rbuckton)

I'll need to fix the DT tests for es-abstract, as it has an // $ExpectType for the type of Promise which is now wrong because the order of overloads for the static resolve method changed in the lib (which is required to pick the correct overload in some cases).

I’ve now fixed that in DefinitelyTyped/DefinitelyTyped#47049.

I’ve also future‑proofed the tests so that // $ExpectType is only used for interface‑based intrinsics (constructors and prototypes).

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

As far as I can tell, all of the community test failures are just due to differences between the baselines and the current versions of the community projects, nothing seems to stand out as being related to this change.

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

Local RWC run looks good (removes a few errors due to the assignability logic for treating void as optional, which is the intended change).

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

I will look into adding undefined | unknown | any in non-strict JS in a follow on PR.

@rbuckton
Ron Buckton (rbuckton) deleted the optionalVoidAssignability branch August 28, 2020 23:06
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Uncommitted BugPR for untriaged, rejected, closed or missing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

'void' parameters treated as optional only for calls, but not for assignability.

5 participants

@rbuckton@typescript-bot@weswigham@ExE-Boss@andrewbranch