Uh oh!
There was an error while loading. Please reload this page.
Add TReturn/TNext to Iterable et al - #58243
Conversation
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.
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.
Jake Bailey (@jakebailey), Wesley Wigham (@weswigham): Am I missing something? The bot says there were interesting changes but it links to a clean pipeline result. |
Jake Bailey (jakebailey)
commented
Jul 18, 2024
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/ |
Ron Buckton (rbuckton)
commented
Jul 19, 2024
TypeScript Bot (@typescript-bot) perf test |
Starting jobs; this comment will be updated as builds start and complete.
|
Ron Buckton (rbuckton)
commented
Jul 19, 2024
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 |
Ron Buckton (rbuckton)
commented
Jul 19, 2024
TypeScript Bot (@typescript-bot) perf test |
TypeScript Bot (typescript-bot)
commented
Jul 19, 2024
Ron Buckton (@rbuckton) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ron Buckton (rbuckton)
commented
Jul 19, 2024
Jake Bailey (@jakebailey): It looks like the perf run was nominal. |
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.
what other kinds of types would benefit from this?
There was a problem hiding this comment.
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.
| type.node.typeArguments.length < typeParameterCount | ||
| ) { | ||
| while (typeParameterCount > 0) { | ||
| const typeArgument = typeArguments[typeParameterCount - 1]; |
There was a problem hiding this comment.
an incidental question, but: what is a case where typeArguments.length > 0 but also !type.node.typeArguments is true?
There was a problem hiding this comment.
type argument inference
| // 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.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| this._copyOnWrite = false; | ||
| } | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
will all generators need to return undefined now? do you have an estimate of how much code that breaks?
There was a problem hiding this comment.
from reading type baselines, it looks like their return type changes to any instead of undefined
There was a problem hiding this comment.
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).
Jake Bailey (jakebailey)
commented
Jul 19, 2024
I tested this on the most iterator-y codebase I know (effect) and it only caused one break (positively), so that's good. |
Ron Buckton (rbuckton)
commented
Jul 19, 2024
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 |
Nathan Shively-Sanders (sandersn)
left a comment
There was a problem hiding this comment.
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 :)
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
BuiltinIteratorReturnintrinsic type whose actual type is determined by the state of a newstrictBuiltinIteratorReturncompiler option:"strictBuiltinIteratorReturn": false-any(emulates current behavior forIterableIterator)"strictBuiltinIteratorReturn": true-undefinedThe
strictBuiltinIteratorReturnis astrictoption flag, meaning that it is enabled automatically when you set"strict": truein yourtsconfig.json.The new
BuiltinIteratorReturntype is passed as theTReturntype argument for built-ins usingIterableIteratororAsyncIterableIteratorto enable stricter checks against the result of callingnext()on the iterator.Enabling
strictBuiltinIteratorReturnresults in a more accurate and type-safe return type for thenext()methods of iterators produced by built-ins likeArray,Set,Map, etc.:vs
Since this is a
strictflag, there is a fair amount of existing code that will likely produce new compilation errors as a result of this change:This now fails as there is no correlation between
set.sizeand the iterator produced bykeys(), 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: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-tsbreaks will be addressed by gcanti/fp-ts#1949webpackbreaks will be addressed by webpack/webpack#18591Fixes#33353
Fixes#52998
Fixes#43750
Closes#56517
Related #58222