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.

Sign up for GitHub

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

Jump to bottom

Treat void-typed properties as optional #40823

Closed
wants to merge 1 commit into from
Closed

Treat void-typed properties as optional #40823

wants to merge 1 commit into from

Conversation

rbuckton
Copy link
Member

This treats void-typed properties (or properties with a void constituent) as optional for the purpose of assignability. This is similar to how we treat trailing void parameters in signatures. The primary intended use case is to address an ECMAScript spec-compliance issue with IteratorResult, though this has possibly further-reaching consequences than just that change:

// given
interface IteratorYieldResult<TYield> {
    done?: false;
    value: TYield;
}
interface IteratorReturnResult<TReturn> {
    done: true;
    value: TReturn;
}

type IteratorResult<T, TReturn = any> = IteratorYieldResult<T> | IteratorReturnResult<TReturn>;

const myIterator: Iterator<number, void> = {
  next(value?: number) {
    return { done: true }; // `value` is not required because it is typed `void`.
  }
}

Unfortunately, simply making value optional isn't an option as it would make the result of yield* essentially always optional, as well as the result from calling .next() when iteration has completed. For example:

function * g() /*: Generator<never, number, undefined> */ {
  return 1;
}
function * f() {
  const x = yield* g(); // x should be `number`, but if `value` is optional it is `number | undefined`.
}
const result = g().next(); // IteratorResult<never, number>;
if (result.done) {
  result; // IteratorReturnResult<number>;
  result.value; // should be `number`, but if `value` is optional it is `number | undefined`.
}

I did consider changing the type of IteratorResult:

interface IteratorYieldResult<TYield> {
    done?: false;
    value: TYield;
}
interface IteratorReturnResult<TReturn> {
    done: true;
    value: TReturn;
}
interface IteratorVoidReturnResult {
  done: true;
  value?: void;
}
type IteratorResult<T, TReturn = any> =
  | IteratorYieldResult<T>
  | IteratorReturnResult<TReturn>
  | (TReturn extends void ? IteratorVoidReturnResult : never);

However that would require adding additional complexity to getIterationTypesOfIteratorResult to extract iteration types, and I'm also concerned that changing IteratorResult in this way would cause issues with assignability for existing custom iterators.

Fixes #38479

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Sep 29, 2020
@rbuckton
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2020 •
edited

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2020 •
edited

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2020 •
edited

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 29, 2020 •
edited

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..40823

Metric master 40823 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 349,616k (± 0.01%) 349,665k (± 0.03%) +49k (+ 0.01%) 349,497k 349,945k
Parse Time 2.00s (± 0.26%) 2.01s (± 0.42%) +0.01s (+ 0.50%) 1.98s 2.02s
Bind Time 0.82s (± 0.81%) 0.82s (± 0.91%) 0.00s ( 0.00%) 0.81s 0.85s
Check Time 4.90s (± 0.77%) 4.89s (± 0.49%) -0.01s (- 0.16%) 4.85s 4.96s
Emit Time 5.19s (± 0.52%) 5.17s (± 0.51%) -0.02s (- 0.35%) 5.09s 5.22s
Total Time 12.91s (± 0.43%) 12.90s (± 0.33%) -0.01s (- 0.11%) 12.81s 13.00s
Monaco - node (v10.16.3, x64)
Memory used 354,341k (± 0.03%) 354,311k (± 0.02%) -30k (- 0.01%) 354,162k 354,413k
Parse Time 1.56s (± 0.52%) 1.56s (± 0.51%) -0.00s (- 0.13%) 1.54s 1.58s
Bind Time 0.71s (± 0.52%) 0.71s (± 0.69%) -0.00s (- 0.14%) 0.70s 0.72s
Check Time 5.07s (± 0.61%) 5.06s (± 0.50%) -0.01s (- 0.14%) 4.99s 5.12s
Emit Time 2.76s (± 0.86%) 2.75s (± 0.59%) -0.01s (- 0.43%) 2.72s 2.79s
Total Time 10.10s (± 0.49%) 10.08s (± 0.22%) -0.02s (- 0.23%) 10.03s 10.13s
TFS - node (v10.16.3, x64)
Memory used 307,541k (± 0.04%) 307,560k (± 0.02%) +19k (+ 0.01%) 307,429k 307,667k
Parse Time 1.21s (± 0.56%) 1.21s (± 0.49%) 0.00s ( 0.00%) 1.20s 1.22s
Bind Time 0.67s (± 0.99%) 0.67s (± 1.21%) -0.00s (- 0.45%) 0.64s 0.68s
Check Time 4.54s (± 0.49%) 4.54s (± 0.46%) -0.00s (- 0.07%) 4.48s 4.58s
Emit Time 2.90s (± 1.00%) 2.92s (± 0.81%) +0.02s (+ 0.62%) 2.85s 2.97s
Total Time 9.33s (± 0.46%) 9.34s (± 0.47%) +0.01s (+ 0.12%) 9.21s 9.41s
material-ui - node (v10.16.3, x64)
Memory used 489,001k (± 0.01%) 488,962k (± 0.01%) -39k (- 0.01%) 488,898k 489,067k
Parse Time 1.98s (± 0.45%) 1.98s (± 0.69%) -0.00s (- 0.10%) 1.96s 2.02s
Bind Time 0.65s (± 0.86%) 0.65s (± 0.53%) -0.00s (- 0.15%) 0.64s 0.65s
Check Time 13.50s (± 0.78%) 13.59s (± 0.66%) +0.09s (+ 0.64%) 13.32s 13.73s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.14s (± 0.64%) 16.22s (± 0.60%) +0.08s (+ 0.50%) 15.93s 16.38s
Angular - node (v12.1.0, x64)
Memory used 326,839k (± 0.03%) 326,684k (± 0.09%) -155k (- 0.05%) 325,639k 327,041k
Parse Time 1.99s (± 0.51%) 1.99s (± 0.61%) +0.00s (+ 0.15%) 1.97s 2.03s
Bind Time 0.81s (± 0.64%) 0.81s (± 0.45%) -0.00s (- 0.37%) 0.80s 0.81s
Check Time 4.80s (± 0.57%) 4.79s (± 0.40%) -0.01s (- 0.19%) 4.75s 4.84s
Emit Time 5.36s (± 0.68%) 5.35s (± 0.82%) -0.00s (- 0.04%) 5.29s 5.51s
Total Time 12.95s (± 0.46%) 12.94s (± 0.46%) -0.01s (- 0.08%) 12.85s 13.15s
Monaco - node (v12.1.0, x64)
Memory used 336,492k (± 0.01%) 336,509k (± 0.03%) +18k (+ 0.01%) 336,355k 336,697k
Parse Time 1.54s (± 0.86%) 1.54s (± 0.84%) -0.00s (- 0.13%) 1.51s 1.57s
Bind Time 0.70s (± 1.84%) 0.69s (± 0.32%) -0.01s (- 1.29%) 0.69s 0.70s
Check Time 4.87s (± 0.47%) 4.87s (± 0.51%) +0.00s (+ 0.08%) 4.83s 4.92s
Emit Time 2.83s (± 0.76%) 2.81s (± 0.81%) -0.02s (- 0.67%) 2.77s 2.88s
Total Time 9.93s (± 0.47%) 9.91s (± 0.38%) -0.03s (- 0.26%) 9.84s 10.03s
TFS - node (v12.1.0, x64)
Memory used 291,819k (± 0.02%) 291,850k (± 0.02%) +31k (+ 0.01%) 291,708k 291,960k
Parse Time 1.23s (± 0.66%) 1.23s (± 0.59%) -0.00s (- 0.16%) 1.22s 1.25s
Bind Time 0.64s (± 0.90%) 0.65s (± 1.15%) +0.00s (+ 0.62%) 0.64s 0.67s
Check Time 4.44s (± 0.36%) 4.45s (± 0.45%) +0.01s (+ 0.16%) 4.40s 4.50s
Emit Time 2.94s (± 0.96%) 2.93s (± 0.75%) -0.00s (- 0.07%) 2.90s 2.98s
Total Time 9.25s (± 0.42%) 9.26s (± 0.34%) +0.01s (+ 0.12%) 9.18s 9.31s
material-ui - node (v12.1.0, x64)
Memory used 466,976k (± 0.05%) 466,897k (± 0.05%) -78k (- 0.02%) 466,019k 467,077k
Parse Time 2.00s (± 0.38%) 2.00s (± 0.35%) -0.00s (- 0.15%) 1.99s 2.02s
Bind Time 0.64s (± 0.58%) 0.64s (± 0.81%) +0.00s (+ 0.63%) 0.63s 0.65s
Check Time 12.01s (± 0.69%) 12.12s (± 0.66%) +0.11s (+ 0.93%) 12.00s 12.28s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.65s (± 0.59%) 14.76s (± 0.60%) +0.11s (+ 0.76%) 14.62s 14.94s
Angular - node (v8.9.0, x64)
Memory used 346,316k (± 0.02%) 346,375k (± 0.03%) +59k (+ 0.02%) 346,082k 346,566k
Parse Time 2.55s (± 0.37%) 2.55s (± 0.29%) +0.01s (+ 0.20%) 2.54s 2.57s
Bind Time 0.86s (± 0.58%) 0.87s (± 1.17%) +0.00s (+ 0.46%) 0.85s 0.90s
Check Time 5.54s (± 0.73%) 5.55s (± 0.46%) +0.01s (+ 0.16%) 5.48s 5.59s
Emit Time 6.21s (± 1.36%) 6.13s (± 1.42%) -0.09s (- 1.38%) 5.92s 6.34s
Total Time 15.17s (± 0.51%) 15.09s (± 0.55%) -0.07s (- 0.47%) 14.86s 15.31s
Monaco - node (v8.9.0, x64)
Memory used 355,579k (± 0.02%) 355,628k (± 0.01%) +50k (+ 0.01%) 355,564k 355,690k
Parse Time 1.90s (± 0.44%) 1.89s (± 0.47%) -0.01s (- 0.47%) 1.87s 1.91s
Bind Time 0.89s (± 0.82%) 0.90s (± 0.67%) +0.00s (+ 0.34%) 0.88s 0.91s
Check Time 5.62s (± 0.55%) 5.60s (± 0.57%) -0.01s (- 0.25%) 5.50s 5.67s
Emit Time 3.31s (± 1.50%) 3.31s (± 1.15%) +0.00s (+ 0.12%) 3.19s 3.36s
Total Time 11.71s (± 0.55%) 11.70s (± 0.58%) -0.01s (- 0.12%) 11.50s 11.80s
TFS - node (v8.9.0, x64)
Memory used 309,276k (± 0.01%) 309,310k (± 0.01%) +34k (+ 0.01%) 309,218k 309,401k
Parse Time 1.56s (± 0.32%) 1.55s (± 0.54%) -0.01s (- 0.45%) 1.54s 1.58s
Bind Time 0.68s (± 0.95%) 0.68s (± 0.86%) -0.00s (- 0.15%) 0.66s 0.69s
Check Time 5.30s (± 0.59%) 5.29s (± 0.37%) -0.00s (- 0.08%) 5.25s 5.33s
Emit Time 2.93s (± 0.80%) 2.93s (± 0.48%) -0.00s (- 0.14%) 2.90s 2.95s
Total Time 10.46s (± 0.28%) 10.45s (± 0.21%) -0.02s (- 0.14%) 10.38s 10.48s
material-ui - node (v8.9.0, x64)
Memory used 493,347k (± 0.02%) 493,314k (± 0.01%) -33k (- 0.01%) 493,128k 493,459k
Parse Time 2.40s (± 0.36%) 2.40s (± 0.44%) +0.00s (+ 0.04%) 2.38s 2.43s
Bind Time 0.81s (± 1.17%) 0.82s (± 0.71%) +0.00s (+ 0.37%) 0.80s 0.83s
Check Time 17.92s (± 0.84%) 18.09s (± 0.92%) +0.17s (+ 0.97%) 17.67s 18.42s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 21.14s (± 0.68%) 21.32s (± 0.77%) +0.18s (+ 0.85%) 20.89s 21.65s
Angular - node (v8.9.0, x86)
Memory used 198,593k (± 0.02%) 198,625k (± 0.02%) +32k (+ 0.02%) 198,529k 198,736k
Parse Time 2.48s (± 0.81%) 2.47s (± 0.43%) -0.01s (- 0.44%) 2.44s 2.49s
Bind Time 1.00s (± 0.93%) 1.00s (± 1.15%) +0.00s (+ 0.50%) 0.97s 1.03s
Check Time 4.99s (± 0.61%) 5.00s (± 0.43%) +0.01s (+ 0.14%) 4.95s 5.05s
Emit Time 5.92s (± 0.90%) 5.93s (± 1.24%) +0.01s (+ 0.17%) 5.80s 6.14s
Total Time 14.39s (± 0.56%) 14.40s (± 0.47%) +0.01s (+ 0.08%) 14.23s 14.57s
Monaco - node (v8.9.0, x86)
Memory used 201,394k (± 0.01%) 201,381k (± 0.02%) -13k (- 0.01%) 201,277k 201,437k
Parse Time 1.93s (± 0.81%) 1.93s (± 0.69%) -0.00s (- 0.21%) 1.90s 1.96s
Bind Time 0.71s (± 0.91%) 0.71s (± 0.78%) 0.00s ( 0.00%) 0.70s 0.72s
Check Time 5.46s (± 1.24%) 5.50s (± 1.37%) +0.04s (+ 0.79%) 5.38s 5.68s
Emit Time 3.03s (± 3.23%) 2.97s (± 3.51%) -0.06s (- 1.91%) 2.65s 3.08s
Total Time 11.13s (± 0.64%) 11.10s (± 0.71%) -0.02s (- 0.22%) 10.92s 11.30s
TFS - node (v8.9.0, x86)
Memory used 176,811k (± 0.02%) 176,806k (± 0.03%) -5k (- 0.00%) 176,732k 176,961k
Parse Time 1.58s (± 0.84%) 1.58s (± 0.53%) -0.00s (- 0.13%) 1.57s 1.60s
Bind Time 0.65s (± 1.23%) 0.64s (± 0.90%) -0.01s (- 0.77%) 0.63s 0.66s
Check Time 4.79s (± 0.56%) 4.78s (± 0.65%) -0.00s (- 0.10%) 4.73s 4.85s
Emit Time 2.80s (± 1.10%) 2.79s (± 1.14%) -0.01s (- 0.36%) 2.73s 2.90s
Total Time 9.82s (± 0.40%) 9.80s (± 0.47%) -0.02s (- 0.17%) 9.71s 9.88s
material-ui - node (v8.9.0, x86)
Memory used 277,775k (± 0.01%) 277,740k (± 0.02%) -35k (- 0.01%) 277,637k 277,879k
Parse Time 2.46s (± 0.82%) 2.46s (± 0.54%) -0.00s (- 0.04%) 2.43s 2.50s
Bind Time 0.71s (± 5.86%) 0.74s (± 7.55%) +0.02s (+ 3.51%) 0.67s 0.87s
Check Time 16.38s (± 0.69%) 16.55s (± 0.74%) +0.17s (+ 1.01%) 16.37s 16.92s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 19.55s (± 0.74%) 19.74s (± 0.69%) +0.19s (+ 0.96%) 19.48s 20.07s
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 40823 10
Baseline master 10

@rbuckton rbuckton mentioned this pull request Sep 29, 2020
Closed
@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 typescript-bot mentioned this pull request Sep 29, 2020
Open
@sandersn sandersn added this to Not started in PR Backlog Oct 5, 2020
@sandersn
Copy link
Member

sandersn commented Oct 9, 2020

@rbuckton where did we land on this? I remember discussing something like this in the design meeting, but not whether we decided to go forward with it.

@sandersn sandersn moved this from Not started to Waiting on author in PR Backlog Oct 9, 2020
@brainkim brainkim mentioned this pull request Feb 9, 2021
Open
5 tasks
@sandersn
Copy link
Member

@rbuckton do you mind if I close this until we come up with a comprehensive reform of void? At least, my memory of our last discussion of this is that we're kind of stuck on this particular change. I might be mis-remembering, though.

@weswigham weswigham removed their request for review March 4, 2022 19:21
@sandersn
Copy link
Member

Closing this for now.

@sandersn sandersn closed this Mar 10, 2022
PR Backlog automation moved this from Waiting on author to Done Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

@ahejlsberg ahejlsberg Awaiting requested review from ahejlsberg

Assignees

@rbuckton rbuckton

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
PR Backlog
Archived in project
PR Backlog
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IteratorResult<T> definition conflicts with the JS spec
3 participants
@rbuckton @typescript-bot @sandersn