Skip to content

A union including non-iterable types is not iterable #40350

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.

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

Merged
merged 3 commits into from
Sep 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33444,8 +33444,9 @@ namespace ts {
if (iterationTypes === noIterationTypes) {
if (errorNode) {
reportTypeNotIterableError(errorNode, type, !!(use & IterationUse.AllowsAsyncIterablesFlag));
errorNode = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I kept looping was to provide some kind of useful type for iteration once they fixed the error. The problem the issue mentioned was that no error was reported.

Copy link
Contributor

Choose a reason for hiding this comment

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

(btw: I'm not saying this change is wrong, just expressing the reasoning behind the existing logic).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that makes sense in theory, but the problem here was that errorNode was intentionally not supplied, so the non-iterableness of the type was completely lost.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about the baseline change?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems fine. I do wonder about the | null case. If it were data[Symbol.iterator]() we would have errored on data with Object is possibly 'null'..

}
setCachedIterationTypes(type, cacheKey, noIterationTypes);
return undefined;
}
else {
allIterationTypes = append(allIterationTypes, iterationTypes);
Expand Down Expand Up @@ -33787,6 +33788,28 @@ namespace ts {
return methodName === "next" ? anyIterationTypes : undefined;
}

// If the method signature comes exclusively from the global iterator or generator type,
// create iteration types from its type arguments like `getIterationTypesOfIteratorFast`
// does (so as to remove `undefined` from the next and return types). We arrive here when
// a contextual type for a generator was not a direct reference to one of those global types,
// but looking up `methodType` referred to one of them (and nothing else). E.g., in
// `interface SpecialIterator extends Iterator<number> {}`, `SpecialIterator` is not a
// reference to `Iterator`, but its `next` member derives exclusively from `Iterator`.
if (methodType?.symbol && methodSignatures.length === 1) {
const globalGeneratorType = resolver.getGlobalGeneratorType(/*reportErrors*/ false);
const globalIteratorType = resolver.getGlobalIteratorType(/*reportErrors*/ false);
const isGeneratorMethod = globalGeneratorType.symbol?.members?.get(methodName as __String) === methodType.symbol;
const isIteratorMethod = !isGeneratorMethod && globalIteratorType.symbol?.members?.get(methodName as __String) === methodType.symbol;
if (isGeneratorMethod || isIteratorMethod) {
const globalType = isGeneratorMethod ? globalGeneratorType : globalIteratorType;
const { mapper } = methodType as AnonymousType;
return createIterationTypes(
getMappedType(globalType.typeParameters![0], mapper!),
getMappedType(globalType.typeParameters![1], mapper!),
methodName === "next" ? getMappedType(globalType.typeParameters![2], mapper!) : undefined);
}
}

// Extract the first parameter and return type of each signature.
let methodParameterTypes: Type[] | undefined;
let methodReturnTypes: Type[] | undefined;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment4.ts(5,7): error TS2548: Type 'number[] | null' is not an array type or does not have a '[Symbol.iterator]()' method that returns an iterator.


==== tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment4.ts (1 errors) ====
// #35497


declare const data: number[] | null;
const [value] = data; // Error
~~~~~~~
!!! error TS2548: Type 'number[] | null' is not an array type or does not have a '[Symbol.iterator]()' method that returns an iterator.

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
//// [destructuringArrayBindingPatternAndAssignment4.ts]
// #35497


declare const data: number[] | null;
const [value] = data; // Error


//// [destructuringArrayBindingPatternAndAssignment4.js]
"use strict";
// #35497
var __read = (this && this.__read) || function (o, n) {
var m = typeof Symbol === "function" && o[Symbol.iterator];
if (!m) return o;
var i = m.call(o), r, ar = [], e;
try {
while ((n === void 0 || n-- > 0) && !(r = i.next()).done) ar.push(r.value);
}
catch (error) { e = { error: error }; }
finally {
try {
if (r && !r.done && (m = i["return"])) m.call(i);
}
finally { if (e) throw e.error; }
}
return ar;
};
var _a = __read(data, 1), value = _a[0]; // Error
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
=== tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment4.ts ===
// #35497


declare const data: number[] | null;
>data : Symbol(data, Decl(destructuringArrayBindingPatternAndAssignment4.ts, 3, 13))

const [value] = data; // Error
>value : Symbol(value, Decl(destructuringArrayBindingPatternAndAssignment4.ts, 4, 7))
>data : Symbol(data, Decl(destructuringArrayBindingPatternAndAssignment4.ts, 3, 13))

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
=== tests/cases/conformance/es6/destructuring/destructuringArrayBindingPatternAndAssignment4.ts ===
// #35497


declare const data: number[] | null;
>data : number[] | null
>null : null

const [value] = data; // Error
>value : any
>data : number[] | null

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//// [generatorReturnTypeIndirectReferenceToGlobalType.ts]
interface I1 extends Iterator<0, 1, 2> {}

function* f1(): I1 {
const a = yield 0;
return 1;
}


//// [generatorReturnTypeIndirectReferenceToGlobalType.js]
"use strict";
function* f1() {
const a = yield 0;
return 1;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
=== tests/cases/conformance/generators/generatorReturnTypeIndirectReferenceToGlobalType.ts ===
interface I1 extends Iterator<0, 1, 2> {}
>I1 : Symbol(I1, Decl(generatorReturnTypeIndirectReferenceToGlobalType.ts, 0, 0))
>Iterator : Symbol(Iterator, Decl(lib.es2015.iterable.d.ts, --, --))

function* f1(): I1 {
>f1 : Symbol(f1, Decl(generatorReturnTypeIndirectReferenceToGlobalType.ts, 0, 41))
>I1 : Symbol(I1, Decl(generatorReturnTypeIndirectReferenceToGlobalType.ts, 0, 0))

const a = yield 0;
>a : Symbol(a, Decl(generatorReturnTypeIndirectReferenceToGlobalType.ts, 3, 7))

return 1;
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
=== tests/cases/conformance/generators/generatorReturnTypeIndirectReferenceToGlobalType.ts ===
interface I1 extends Iterator<0, 1, 2> {}

function* f1(): I1 {
>f1 : () => I1

const a = yield 0;
>a : 2
>yield 0 : 2
>0 : 0

return 1;
>1 : 1
}

6 changes: 3 additions & 3 deletions tests/baselines/reference/generatorYieldContextualType.types
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ declare function f2<T, R, S>(gen: () => Generator<R, T, S> | AsyncGenerator<R, T
f2<0, 0, 1>(async function* () {
>f2<0, 0, 1>(async function* () { const a = yield 0; return 0;}) : void
>f2 : <T, R, S>(gen: () => Generator<R, T, S> | AsyncGenerator<R, T, S>) => void
>async function* () { const a = yield 0; return 0;} : () => AsyncGenerator<0, 0, 1>
>async function* () { const a = yield 0; return 0;} : () => AsyncGenerator<0, 0, 1 | undefined>

const a = yield 0;
>a : 1
>yield 0 : 1
>a : 1 | undefined
Copy link
Member Author

Choose a reason for hiding this comment

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

In a sense, this is not wrong—gen could have its next() called with no arguments, making a undefined at runtime. But previously, this fell into a special case of ignoring that optionality for convenience.

Now, when we try to get the iteration types of Generator<R, T, S> | AsyncGenerator<R, T, S> as an async iterable, we get noIterationTypes, because the former doesn’t have a [Symbol.asyncIterator]() member. Instead, we fall back to getting the iteration types of this union as an async iterator, using its next/return/throw method signatures to determine T, TReturn, and TNext. Because we have a union signature, it doesn’t count as a reference to one of the global generator/iterator types, and so it doesn’t take the fast path that ignores the optionality. Instead, it correctly sees that the next() method can take no arguments or [1], and so it infers the type 1 | undefined.

More work could be done to make union signatures take the fast path that drops the optionality if every signature in the union is a reference to one of the global generator/iterator types, but I felt like this was getting fairly complex for limited utility—the results it produces with less complexity are strictly more correct given the limitations we have for modeling this.

>yield 0 : 1 | undefined
>0 : 0

return 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ tests/cases/conformance/types/asyncGenerators/types.asyncGenerators.es2018.2.ts(
tests/cases/conformance/types/asyncGenerators/types.asyncGenerators.es2018.2.ts(70,42): error TS2322: Type 'AsyncGenerator<number, any, undefined>' is not assignable to type 'Iterator<number, any, undefined>'.
The types returned by 'next(...)' are incompatible between these types.
Type 'Promise<IteratorResult<number, any>>' is not assignable to type 'IteratorResult<number, any>'.
Property 'value' is missing in type 'Promise<IteratorResult<number, any>>' but required in type 'IteratorYieldResult<number>'.
Type 'Promise<IteratorResult<number, any>>' is missing the following properties from type 'IteratorReturnResult<any>': done, value
tests/cases/conformance/types/asyncGenerators/types.asyncGenerators.es2018.2.ts(74,12): error TS2504: Type '{}' must have a '[Symbol.asyncIterator]()' method that returns an async iterator.


Expand Down Expand Up @@ -191,8 +191,7 @@ tests/cases/conformance/types/asyncGenerators/types.asyncGenerators.es2018.2.ts(
!!! error TS2322: Type 'AsyncGenerator<number, any, undefined>' is not assignable to type 'Iterator<number, any, undefined>'.
!!! error TS2322: The types returned by 'next(...)' are incompatible between these types.
!!! error TS2322: Type 'Promise<IteratorResult<number, any>>' is not assignable to type 'IteratorResult<number, any>'.
!!! error TS2322: Property 'value' is missing in type 'Promise<IteratorResult<number, any>>' but required in type 'IteratorYieldResult<number>'.
!!! related TS2728 /.ts/lib.es2015.iterable.d.ts:33:5: 'value' is declared here.
!!! error TS2322: Type 'Promise<IteratorResult<number, any>>' is missing the following properties from type 'IteratorReturnResult<any>': done, value
yield 1;
}
async function * yieldStar() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// #35497

// @target: es5
// @downlevelIteration: true
// @lib: es6
// @strict: true

declare const data: number[] | null;
const [value] = data; // Error
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// @strict: true
// @target: esnext

interface I1 extends Iterator<0, 1, 2> {}

function* f1(): I1 {
const a = yield 0;
return 1;
}