Skip to content

Don't allow to iterate over 'never'#22964

Merged
2 commits merged intomasterfrom
for_of_never
Mar 29, 2018
Merged

Don't allow to iterate over 'never'#22964
2 commits merged intomasterfrom
for_of_never

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Mar 28, 2018

Currently the following has no compile error:

function f(stringOrStrings: string | string) {
    if (typeof stringOrStrings !== "string") {
        for (const s of stringOrStrings) {
            s.twoUpprKaze();
        }
    }
}

I meant string | string[]. The problem would be solved if never weren't considered iterable.

@ghost ghost requested review from rbuckton, sandersn and weswigham March 28, 2018 22:14
@ghost ghost force-pushed the for_of_never branch from 190bbbd to a94c420 Compare March 28, 2018 22:15
weswigham
weswigham approved these changes Mar 29, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

I mean, conceptually never is iterable. It's everything, kinda like any. But this is a good change - I just think the error message could be improved.

Comment thread src/compiler/checker.ts Outdated
function getIteratedTypeOrElementType(inputType: Type, errorNode: Node, allowStringInput: boolean, allowAsyncIterables: boolean, checkAssignability: boolean): Type {
function getIteratedTypeOrElementType(inputType: Type, errorNode: Node, allowStringInput: boolean, allowAsyncIterables: boolean, checkAssignability: boolean): Type | undefined {
if (inputType === neverType) {
reportTypeNotIterableError(errorNode, allowAsyncIterables);
Copy link
Copy Markdown
Member

@weswigham weswigham Mar 29, 2018

Choose a reason for hiding this comment

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

Hmmm... the diagnostic message Diagnostics.Type_must_have_a_Symbol_iterator_method_that_returns_an_iterator doesn't mention what type we're erroring on. Might be an improvement to include the type in the error, especially since typing x[Symbol.iterator]() isn't going to produce an error when x is never (and will have a "return type" assignable to an iterator).

@ghost
Copy link
Copy Markdown
Author

ghost commented Mar 29, 2018

@weswigham Good to go?

@weswigham
Copy link
Copy Markdown
Member

weswigham commented Mar 29, 2018

Yeah, looks good.

@ghost ghost merged commit 65659d7 into master Mar 29, 2018
@ghost ghost deleted the for_of_never branch March 29, 2018 17:02
@ghost ghost mentioned this pull request Mar 29, 2018
Merged
@mhegazy mhegazy added the Breaking Change Would introduce errors in existing code label Mar 29, 2018
@mhegazy mhegazy added this to the TypeScript 2.9 milestone Mar 29, 2018
@ericanderson
Copy link
Copy Markdown
Contributor

ericanderson commented May 21, 2018

Given that we are special casing never now, could we consider handling this too:

declare const nope: never;

declare const anArray: any[];
declare const anObject: { [key: string]: any };
declare function aMethod(arg: string): string;

anArray[nope]; // $ExpectError
anObject[nope]; // $ExpectError
aMethod(nope); // $ExpectError

const hello = "hello " + nope; // $ExpectError
const meaningOfLife = 42 + nope; // $ExpectError

@ghost
Copy link
Copy Markdown
Author

ghost commented May 21, 2018

@ericanderson Could you open a new issue?

@ericanderson ericanderson mentioned this pull request May 21, 2018
Closed
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sandersn sandersn Awaiting requested review from sandersn

Assignees

No one assigned

Labels

Breaking Change Would introduce errors in existing code

Projects

None yet

Milestone

TypeScript 2.9

Development

Successfully merging this pull request may close these issues.

4 participants

@weswigham @ericanderson @rbuckton @mhegazy