GeezerVison is showing github.com — Add TReturn/TNext to Iterable et al by rbuckton · Pull Request #58243 · microsoft/TypeScript
Conversation
We avoided making this change in the past as it was very breaky, but at some point we need to address this discrepancy. Having incorrect types here is also causing problems with properly typing the Iterator Helpers proposal.
This also adds a new BuiltinIteratorReturn intrinsic type whose actual type is determined by the state of a new strictBuiltinIteratorReturn compiler option:
"strictBuiltinIteratorReturn": false - any (emulates current behavior for IterableIterator)"strictBuiltinIteratorReturn": true - undefined
The strictBuiltinIteratorReturn is a strict option flag, meaning that it is enabled automatically when you set "strict": true in your tsconfig.json.
The new BuiltinIteratorReturn type is passed as the TReturn type argument for built-ins using IterableIterator or AsyncIterableIterator to enable stricter checks against the result of calling next() on the iterator.
Enabling strictBuiltinIteratorReturn results in a more accurate and type-safe return type for the next() methods of iterators produced by built-ins like Array, Set, Map, etc.:
// @strictBuiltinIteratorReturn: false
// NOTE: matches current behavior
const set = new Set(["a"]);
const result = set.keys().next();
// ^? result: IteratorResult<string, any>
const value = result.value;
// ^? value: any
if (!result.done) {
const value = result.value;
// ^? value: string
} else {
const value = result.value;
// ^? value: any
}vs
// @strictBuiltinIteratorReturn: true
const set = new Set(["a"]);
const result = set.keys().next();
// ^? result: IteratorResult<string, undefined>
const value = result.value;
// ^? value: string | undefined
if (!result.done) {
const value = result.value;
// ^? value: string
} else {
const value = result.value;
// ^? value: undefined
}Since this is a strict flag, there is a fair amount of existing code that will likely produce new compilation errors as a result of this change:
// @strict: true
function only<T>(set: Set<T>): T {
if (set.size !== 1) throw new TypeError();
return set.keys().next().value; // worked previously since result was `any`, but is now an error
}This now fails as there is no correlation between set.size and the iterator produced by keys(), so the compiler is unaware that this constraint has been validated. If you are certain iterator will always yield at least one value, you can use a non-null assertion operator to strip off the | undefined:
// @strict: true
function only<T>(set: Set<T>): T {
if (set.size !== 1) throw new TypeError();
return set.keys().next().value!;
}A follow-on PR to TypeScript-DOM-lib-generator can be found here: microsoft/TypeScript-DOM-lib-generator#1713
DefinitelyTyped breaks will be addressed by DefinitelyTyped/DefinitelyTyped#69632
fp-ts breaks will be addressed by gcanti/fp-ts#1949
webpack breaks will be addressed by webpack/webpack#18591
Fixes #33353
Fixes #52998
Fixes #43750
Closes #56517
Related #58222
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
tonivj5, felixfbecker, jarpoole, danvk, and tocque reacted with thumbs up emoji
jakebailey, tonivj5, MattiasBuelens, felixfbecker, danvk, and KisaragiEffective reacted with heart emoji
All reactions
-
👍
5 reactions
-
❤️
6 reactions
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I believe this is what we decided on in the meeting and seems sound; I think that all that's left here is just to run perf on the final state (I don't think its been done in a bit, just the extended tests).
At least, once ADO is back online: https://status.dev.azure.com/
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
typescript-bot reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Starting jobs; this comment will be updated as builds start and complete.
| Command |
Status |
Results |
perf test |
❌ Error: `Error: |
|
<style type='text/css'>
body {
margin: 0;
color: #000;
font-family: "-apple-system",BlinkMacSystemFont,"Segoe UI",` | |
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
The perf shouldn't have changed since the only real changes since it was last run were swapping around what type to use when the flag is enabled, but I'll try it again when the AzDO outage is over
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
typescript-bot reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Starting jobs; this comment will be updated as builds start and complete.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ron Buckton (@rbuckton)
The results of the perf run you requested are in!
Here they are:
tsc
Comparison Report - baseline..pr
| Metric |
baseline |
pr |
Delta |
Best |
Worst |
p-value |
| Compiler-Unions - node (v18.15.0, x64) |
|---|
| Errors |
30 |
30 |
~ |
~ |
~ |
p=1.000 n=6 |
| Symbols |
62,153 |
62,153 |
~ |
~ |
~ |
p=1.000 n=6 |
| Types |
50,242 |
50,242 |
~ |
~ |
~ |
p=1.000 n=6 |
| Memory used |
193,447k (± 0.94%) |
192,835k (± 0.72%) |
~ |
192,226k |
195,677k |
p=0.378 n=6 |
| Parse Time |
1.96s (± 1.27%) |
1.95s (± 0.75%) |
~ |
1.93s |
1.97s |
p=0.222 n=6 |
| Bind Time |
1.06s (± 0.84%) |
1.06s (± 0.77%) |
~ |
1.05s |
1.07s |
p=0.550 n=6 |
| Check Time |
13.97s (± 0.38%) |
13.95s (± 0.39%) |
~ |
13.89s |
14.01s |
p=0.687 n=6 |
| Emit Time |
4.03s (± 0.73%) |
3.99s (± 0.97%) |
~ |
3.92s |
4.02s |
p=0.076 n=6 |
| Total Time |
21.02s (± 0.20%) |
20.95s (± 0.32%) |
-0.08s (- 0.36%) |
20.88s |
21.07s |
p=0.045 n=6 |
| angular-1 - node (v18.15.0, x64) |
|---|
| Errors |
5 |
7 |
🔻+2 (+40.00%) |
~ |
~ |
p=0.001 n=6 |
| Symbols |
944,250 |
945,484 |
+1,234 (+ 0.13%) |
~ |
~ |
p=0.001 n=6 |
| Types |
407,076 |
409,343 |
+2,267 (+ 0.56%) |
~ |
~ |
p=0.001 n=6 |
| Memory used |
1,218,683k (± 0.00%) |
1,220,806k (± 0.00%) |
+2,124k (+ 0.17%) |
1,220,745k |
1,220,853k |
p=0.005 n=6 |
| Parse Time |
8.02s (± 0.56%) |
7.90s (± 0.45%) |
-0.13s (- 1.56%) |
7.84s |
7.93s |
p=0.005 n=6 |
| Bind Time |
2.23s (± 0.34%) |
2.24s (± 0.77%) |
~ |
2.22s |
2.26s |
p=0.404 n=6 |
| Check Time |
36.38s (± 0.63%) |
36.34s (± 0.45%) |
~ |
36.18s |
36.60s |
p=0.872 n=6 |
| Emit Time |
17.88s (± 0.72%) |
17.94s (± 0.42%) |
~ |
17.84s |
18.05s |
p=0.470 n=6 |
| Total Time |
64.52s (± 0.52%) |
64.42s (± 0.33%) |
~ |
64.20s |
64.77s |
p=0.810 n=6 |
| mui-docs - node (v18.15.0, x64) |
|---|
| Errors |
0 |
0 |
~ |
~ |
~ |
p=1.000 n=6 |
| Symbols |
2,276,156 |
2,276,267 |
+111 (+ 0.00%) |
~ |
~ |
p=0.001 n=6 |
| Types |
948,845 |
949,009 |
+164 (+ 0.02%) |
~ |
~ |
p=0.001 n=6 |
| Memory used |
2,207,580k (± 0.00%) |
2,207,979k (± 0.00%) |
+398k (+ 0.02%) |
2,207,770k |
2,208,072k |
p=0.005 n=6 |
| Parse Time |
9.67s (± 0.44%) |
9.67s (± 0.35%) |
~ |
9.61s |
9.70s |
p=0.468 n=6 |
| Bind Time |
3.38s (± 0.74%) |
3.38s (± 1.29%) |
~ |
3.32s |
3.43s |
p=0.935 n=6 |
| Check Time |
105.60s (± 0.57%) |
105.62s (± 0.41%) |
~ |
105.18s |
106.43s |
p=1.000 n=6 |
| Emit Time |
0.20s (± 2.62%) |
0.20s (± 4.01%) |
~ |
0.19s |
0.21s |
p=0.140 n=6 |
| Total Time |
118.85s (± 0.54%) |
118.87s (± 0.37%) |
~ |
118.50s |
119.74s |
p=0.936 n=6 |
| self-build-src - node (v18.15.0, x64) |
|---|
| Errors |
0 |
0 |
~ |
~ |
~ |
p=1.000 n=6 |
| Symbols |
1,227,332 |
1,230,214 |
+2,882 (+ 0.23%) |
~ |
~ |
p=0.001 n=6 |
| Types |
261,851 |
265,676 |
+3,825 (+ 1.46%) |
~ |
~ |
p=0.001 n=6 |
| Memory used |
2,401,304k (± 6.04%) |
2,465,455k (± 7.46%) |
+64,151k (+ 2.67%) |
2,345,723k |
2,703,780k |
p=0.031 n=6 |
| Parse Time |
6.08s (± 0.92%) |
6.08s (± 0.60%) |
~ |
6.04s |
6.13s |
p=0.630 n=6 |
| Bind Time |
2.26s (± 0.46%) |
2.26s (± 1.14%) |
~ |
2.22s |
2.28s |
p=0.871 n=6 |
| Check Time |
40.54s (± 0.72%) |
40.62s (± 0.60%) |
~ |
40.29s |
40.91s |
p=0.810 n=6 |
| Emit Time |
3.82s (± 1.38%) |
3.89s (± 1.49%) |
~ |
3.83s |
3.97s |
p=0.092 n=6 |
| Total Time |
52.72s (± 0.60%) |
52.84s (± 0.45%) |
~ |
52.53s |
53.11s |
p=0.748 n=6 |
| self-build-src-public-api - node (v18.15.0, x64) |
|---|
| Errors |
0 |
0 |
~ |
~ |
~ |
p=1.000 n=6 |
| Symbols |
1,227,332 |
1,230,214 |
+2,882 (+ 0.23%) |
~ |
~ |
p=0.001 n=6 |
| Types |
261,851 |
265,676 |
+3,825 (+ 1.46%) |
~ |
~ |
p=0.001 n=6 |
| Memory used |
2,415,224k (± 0.03%) |
2,420,788k (± 0.03%) |
+5,564k (+ 0.23%) |
2,420,129k |
2,421,854k |
p=0.005 n=6 |
| Parse Time |
5.27s (± 1.34%) |
5.27s (± 0.95%) |
~ |
5.22s |
5.34s |
p=0.688 n=6 |
| Bind Time |
1.69s (± 0.69%) |
1.70s (± 0.78%) |
~ |
1.68s |
1.71s |
p=0.206 n=6 |
| Check Time |
34.95s (± 0.35%) |
35.15s (± 0.28%) |
+0.20s (+ 0.57%) |
35.02s |
35.32s |
p=0.013 n=6 |
| Emit Time |
3.37s (± 1.69%) |
3.34s (± 1.73%) |
~ |
3.29s |
3.45s |
p=0.377 n=6 |
| Total Time |
45.28s (± 0.38%) |
45.46s (± 0.22%) |
~ |
45.27s |
45.57s |
p=0.092 n=6 |
| self-compiler - node (v18.15.0, x64) |
|---|
| Errors |
0 |
0 |
~ |
~ |
~ |
p=1.000 n=6 |
| Symbols |
258,516 |
258,669 |
+153 (+ 0.06%) |
~ |
~ |
p=0.001 n=6 |
| Types |
104,901 |
105,403 |
+502 (+ 0.48%) |
~ |
~ |
p=0.001 n=6 |
| Memory used |
427,887k (± 0.02%) |
428,292k (± 0.02%) |
+404k (+ 0.09%) |
428,174k |
428,457k |
p=0.005 n=6 |
| Parse Time |
4.14s (± 0.43%) |
4.14s (± 0.37%) |
~ |
4.12s |
4.16s |
p=1.000 n=6 |
| Bind Time |
1.62s (± 1.68%) |
1.64s (± 0.75%) |
~ |
1.62s |
1.65s |
p=0.491 n=6 |
| Check Time |
22.18s (± 0.22%) |
22.23s (± 0.25%) |
~ |
22.17s |
22.31s |
p=0.142 n=6 |
| Emit Time |
2.00s (± 1.03%) |
2.01s (± 0.75%) |
~ |
1.98s |
2.02s |
p=0.371 n=6 |
| Total Time |
29.94s (± 0.25%) |
30.01s (± 0.17%) |
~ |
29.96s |
30.08s |
p=0.145 n=6 |
| ts-pre-modules - node (v18.15.0, x64) |
|---|
| Errors |
35 |
35 |
~ |
~ |
~ |
p=1.000 n=6 |
| Symbols |
224,565 |
224,852 |
+287 (+ 0.13%) |
~ |
~ |
p=0.001 n=6 |
| Types |
93,734 |
94,040 |
+306 (+ 0.33%) |
~ |
~ |
p=0.001 n=6 |
| Memory used |
369,634k (± 0.03%) |
369,874k (± 0.04%) |
+240k (+ 0.06%) |
369,726k |
370,159k |
p=0.008 n=6 |
| Parse Time |
3.47s (± 0.43%) |
3.44s (± 0.48%) |
-0.03s (- 0.91%) |
3.41s |
3.46s |
p=0.015 n=6 |
| Bind Time |
1.93s (± 0.63%) |
1.93s (± 1.07%) |
~ |
1.91s |
1.96s |
p=0.935 n=6 |
| Check Time |
19.33s (± 0.52%) |
19.25s (± 0.25%) |
~ |
19.18s |
19.31s |
p=0.128 n=6 |
| Emit Time |
0.00s |
0.00s |
~ |
~ |
~ |
p=1.000 n=6 |
| Total Time |
24.73s (± 0.42%) |
24.62s (± 0.20%) |
~ |
24.55s |
24.67s |
p=0.108 n=6 |
| vscode - node (v18.15.0, x64) |
|---|
| Errors |
0 |
4 |
🔻+4 (+ ∞%) |
~ |
~ |
p=0.001 n=6 |
| Symbols |
2,973,349 |
2,974,196 |
+847 (+ 0.03%) |
~ |
~ |
p=0.001 n=6 |
| Types |
1,021,157 |
1,023,116 |
+1,959 (+ 0.19%) |
~ |
~ |
p=0.001 n=6 |
| Memory used |
3,097,712k (± 0.00%) |
3,099,803k (± 0.00%) |
+2,091k (+ 0.07%) |
3,099,760k |
3,099,855k |
p=0.005 n=6 |
| Parse Time |
11.65s (± 0.24%) |
11.63s (± 0.51%) |
~ |
11.55s |
11.70s |
p=0.688 n=6 |
| Bind Time |
3.56s (± 0.33%) |
3.59s (± 2.19%) |
~ |
3.55s |
3.75s |
p=0.804 n=6 |
| Check Time |
68.75s (± 0.35%) |
68.67s (± 0.27%) |
~ |
68.39s |
68.86s |
p=0.688 n=6 |
| Emit Time |
17.27s (± 0.78%) |
17.48s (± 0.66%) |
+0.20s (+ 1.17%) |
17.27s |
17.58s |
p=0.030 n=6 |
| Total Time |
101.25s (± 0.29%) |
101.36s (± 0.29%) |
~ |
100.96s |
101.69s |
p=0.575 n=6 |
| webpack - node (v18.15.0, x64) |
|---|
| Errors |
0 |
0 |
~ |
~ |
~ |
p=1.000 n=6 |
| Symbols |
267,370 |
267,518 |
+148 (+ 0.06%) |
~ |
~ |
p=0.001 n=6 |
| Types |
108,862 |
109,024 |
+162 (+ 0.15%) |
~ |
~ |
p=0.001 n=6 |
| Memory used |
411,934k (± 0.01%) |
412,274k (± 0.01%) |
+340k (+ 0.08%) |
412,209k |
412,344k |
p=0.005 n=6 |
| Parse Time |
3.19s (± 0.57%) |
3.17s (± 0.33%) |
~ |
3.16s |
3.19s |
p=0.249 n=6 |
| Bind Time |
1.41s (± 0.37%) |
1.41s (± 0.39%) |
~ |
1.40s |
1.41s |
p=0.640 n=6 |
| Check Time |
14.37s (± 0.59%) |
14.29s (± 0.52%) |
~ |
14.18s |
14.41s |
p=0.128 n=6 |
| Emit Time |
0.00s |
0.00s |
~ |
~ |
~ |
p=1.000 n=6 |
| Total Time |
18.96s (± 0.53%) |
18.87s (± 0.43%) |
~ |
18.75s |
19.00s |
p=0.128 n=6 |
| xstate-main - node (v18.15.0, x64) |
|---|
| Errors |
0 |
0 |
~ |
~ |
~ |
p=1.000 n=6 |
| Symbols |
510,573 |
510,625 |
+52 (+ 0.01%) |
~ |
~ |
p=0.001 n=6 |
| Types |
161,621 |
161,746 |
+125 (+ 0.08%) |
~ |
~ |
p=0.001 n=6 |
| Memory used |
448,287k (± 0.09%) |
448,438k (± 0.03%) |
~ |
448,274k |
448,642k |
p=0.936 n=6 |
| Parse Time |
3.90s (± 0.56%) |
3.90s (± 0.45%) |
~ |
3.87s |
3.91s |
p=1.000 n=6 |
| Bind Time |
1.45s (± 0.52%) |
1.44s (± 0.52%) |
-0.01s (- 0.92%) |
1.43s |
1.45s |
p=0.024 n=6 |
| Check Time |
21.18s (± 0.67%) |
21.23s (± 0.26%) |
~ |
21.16s |
21.32s |
p=0.810 n=6 |
| Emit Time |
0.00s |
0.00s |
~ |
~ |
~ |
p=1.000 n=6 |
| Total Time |
26.53s (± 0.61%) |
26.56s (± 0.25%) |
~ |
26.51s |
26.69s |
p=0.810 n=6 |
System info unknown
Scenarios
- Compiler-Unions - node (v18.15.0, x64)
- angular-1 - node (v18.15.0, x64)
- mui-docs - node (v18.15.0, x64)
- self-build-src - node (v18.15.0, x64)
- self-build-src-public-api - node (v18.15.0, x64)
- self-compiler - node (v18.15.0, x64)
- ts-pre-modules - node (v18.15.0, x64)
- vscode - node (v18.15.0, x64)
- webpack - node (v18.15.0, x64)
- xstate-main - node (v18.15.0, x64)
| Benchmark |
Name |
Iterations |
| Current |
pr |
6 |
| Baseline |
baseline |
6 |
tsserver
Comparison Report - baseline..pr
| Metric |
baseline |
pr |
Delta |
Best |
Worst |
p-value |
| Compiler-UnionsTSServer - node (v18.15.0, x64) |
|---|
| Req 1 - updateOpen |
2,298ms (± 0.34%) |
2,302ms (± 0.34%) |
~ |
2,292ms |
2,310ms |
p=0.575 n=6 |
| Req 2 - geterr |
5,171ms (± 0.43%) |
5,136ms (± 0.53%) |
~ |
5,101ms |
5,162ms |
p=0.128 n=6 |
| Req 3 - references |
263ms (± 0.29%) |
262ms (± 1.10%) |
~ |
258ms |
264ms |
p=0.672 n=6 |
| Req 4 - navto |
227ms (± 0.23%) |
227ms (± 0.56%) |
~ |
226ms |
229ms |
p=0.931 n=6 |
| Req 5 - completionInfo count |
1,357 |
1,357 |
~ |
~ |
~ |
p=1.000 n=6 |
| Req 5 - completionInfo |
87ms (± 6.25%) |
88ms (± 3.68%) |
~ |
84ms |
92ms |
p=0.807 n=6 |
| CompilerTSServer - node (v18.15.0, x64) |
|---|
| Req 1 - updateOpen |
3,576ms (± 0.74%) |
3,593ms (± 0.55%) |
~ |
3,572ms |
3,619ms |
p=0.261 n=6 |
| Req 2 - geterr |
5,674ms (± 0.68%) |
5,671ms (± 0.42%) |
~ |
5,641ms |
5,698ms |
p=0.936 n=6 |
| Req 3 - references |
413ms (± 1.56%) |
410ms (± 0.40%) |
~ |
408ms |
412ms |
p=0.809 n=6 |
| Req 4 - navto |
341ms (± 0.72%) |
340ms (± 0.25%) |
~ |
338ms |
340ms |
p=0.115 n=6 |
| Req 5 - completionInfo count |
1,519 |
1,519 |
~ |
~ |
~ |
p=1.000 n=6 |
| Req 5 - completionInfo |
122ms (± 5.06%) |
121ms (± 5.40%) |
~ |
108ms |
125ms |
p=0.934 n=6 |
| xstate-main-1-tsserver - node (v18.15.0, x64) |
|---|
| Req 1 - updateOpen |
5,178ms (± 0.35%) |
5,181ms (± 0.17%) |
~ |
5,172ms |
5,195ms |
p=0.810 n=6 |
| Req 2 - geterr |
1,119ms (± 1.27%) |
1,110ms (± 1.01%) |
~ |
1,099ms |
1,131ms |
p=0.128 n=6 |
| Req 3 - references |
88ms (± 4.22%) |
89ms (± 3.32%) |
~ |
86ms |
92ms |
p=0.797 n=6 |
| Req 4 - navto |
452ms (± 0.18%) |
442ms (± 0.18%) |
-9ms (- 2.07%) |
441ms |
443ms |
p=0.004 n=6 |
| Req 5 - completionInfo count |
3,417 |
3,417 |
~ |
~ |
~ |
p=1.000 n=6 |
| Req 5 - completionInfo |
814ms (± 0.53%) |
805ms (± 0.95%) |
~ |
794ms |
816ms |
p=0.053 n=6 |
System info unknown
Scenarios
- CompilerTSServer - node (v18.15.0, x64)
- Compiler-UnionsTSServer - node (v18.15.0, x64)
- xstate-main-1-tsserver - node (v18.15.0, x64)
| Benchmark |
Name |
Iterations |
| Current |
pr |
6 |
| Baseline |
baseline |
6 |
startup
Comparison Report - baseline..pr
| Metric |
baseline |
pr |
Delta |
Best |
Worst |
p-value |
| tsc-startup - node (v18.15.0, x64) |
|---|
| Execution time |
228.66ms (± 0.16%) |
228.52ms (± 0.14%) |
-0.14ms (- 0.06%) |
226.93ms |
230.79ms |
p=0.000 n=600 |
| tsserver-startup - node (v18.15.0, x64) |
|---|
| Execution time |
347.82ms (± 0.31%) |
348.05ms (± 0.31%) |
+0.23ms (+ 0.07%) |
340.17ms |
356.16ms |
p=0.000 n=600 |
| tsserverlibrary-startup - node (v18.15.0, x64) |
|---|
| Execution time |
227.77ms (± 0.13%) |
227.69ms (± 0.15%) |
-0.08ms (- 0.04%) |
226.18ms |
232.43ms |
p=0.000 n=600 |
| typescript-startup - node (v18.15.0, x64) |
|---|
| Execution time |
226.19ms (± 0.15%) |
226.17ms (± 0.16%) |
~ |
224.42ms |
230.49ms |
p=0.358 n=600 |
System info unknown
Scenarios
- tsc-startup - node (v18.15.0, x64)
- tsserver-startup - node (v18.15.0, x64)
- tsserverlibrary-startup - node (v18.15.0, x64)
- typescript-startup - node (v18.15.0, x64)
| Benchmark |
Name |
Iterations |
| Current |
pr |
6 |
| Baseline |
baseline |
6 |
Developer Information:Download Benchmarks |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
jakebailey and sandersn reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
|
if (type.target.typeParameters) {
|
|
|
typeParameterCount = Math.min(type.target.typeParameters.length, typeArguments.length);
|
|
|
|
|
|
// Maybe we should do this for more types, but for now we only elide type arguments that are
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what other kinds of types would benefit from this?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially any type since this simplifies the emit output to avoid repeating type arguments for type parameters with defaults, but rather than take that on in this PR I focused on this subset to improve back compat.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
type.node.typeArguments.length < typeParameterCount
|
|
|
) {
|
|
|
while (typeParameterCount > 0) {
|
|
|
const typeArgument = typeArguments[typeParameterCount - 1];
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an incidental question, but: what is a case where typeArguments.length > 0 but also !type.node.typeArguments is true?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
// Maybe we should do this for more types, but for now we only elide type arguments that are
|
|
|
// identical to their associated type parameters' defaults for `Iterable`, `IterableIterator`,
|
|
|
// `AsyncIterable`, and `AsyncIterableIterator` to provide backwards-compatible .d.ts emit due
|
|
|
// to each now having three type parameters instead of only one.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall this seems like an odd way to support this backward compatibility, but I don't understand the problem space well enough to suggest anything else
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realistically, this is all about what the type "looks" like, right? Or are we really needing to depend on this emit still emitting the "old thing" to continue to work right for downstream users of d.ts files?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider an application that is currently built using TS 5.5 that depends on a library that is also being built using TS 5.5. The library chooses to upgrade its version of TypeScript to 5.6, and the new emit produces Iterable<T, any, any> where it previously produced Iterable<T>. If the library makes no other code changes and publishes their new build, the application cannot take the update without also updating TypeScript because the expected arity of Iterable has changed.
The library can't just ship a forward declaration of interface Iterable<T, TReturn = any, TNext = any> {}, because our checker performs arity checks on built-ins, and the application might not be running with skipLibCheck, so one way or another they would have errors they cannot silence without possibly giving up the dependency verification that skipLibChecks: false offers.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
this._copyOnWrite = false;
|
|
|
}
|
|
|
}
|
|
|
return undefined;
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will all generators need to return undefined now? do you have an estimate of how much code that breaks?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from reading type baselines, it looks like their return type changes to any instead of undefined
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specifically because this is an implementation of the native Map<K, V> interface, which is now specified to return IterableIterator<T, BuiltinIteratorReturn> (which is IterableIterator<T, undefined> under the new flag).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I tested this on the most iterator-y codebase I know (effect) and it only caused one break (positively), so that's good.
|
sandersn, rbuckton, and acutmore reacted with hooray emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
I tested this on the most iterator-y codebase I know (effect) and it only caused one break (positively), so that's good.
In all of the PRs I've created so far for top libraries, it's usually only been one or two cases where they were (possibly unknowingly) depending on the result of set.next().value being typed as any by default.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have a couple of background questions. I think this PR is worth trying in the beta to see if it's too breaky. But all the first-order breaks look good, and the higher-order ones are, I bet, rare. (like, just fpt-ts :)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 19, 2024
Reviewers
+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Milestone
No milestone
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|
Nathan Shively-Sanders (sandersn)
left a comment