Skip to content

.includes or .indexOf does not narrow the type #36275

Not planned
@minderov

Description

@minderov

TypeScript Version: 3.7.x-dev.201xxxxx

Search Terms: .includes type narrowing, .indexOf type narrowing

Code

interface TextMessage {
    type: 'text',
    text: string
}

interface ImageMessage {
    type: 'image',
    url: string
}

type Message = TextMessage | ImageMessage;

// This is an example to reproduce the error in Playground.
// In practice, assume this message comes from outside our control (e.g. HTTP request)
const message: Message = JSON.parse(prompt('') as string) as Message;

if (message.type === 'text') {
    // No error here
    message.text = message.text.trim();
}

// Same for ['text'].includes(message.type)
if (['text'].indexOf(message.type) > -1) {
    // Error: Property 'text' does not exist on type 'ImageMessage'
    message.text = message.text.trim();
}

Expected behavior:
I expect message to narrow its type to TextMessage inside if (['text'].indexOf(message.type) > -1).

Same way it does inside if (message.type === 'text')

Actual behavior:
message is typed as TextMessage | ImageMessage inside the if block

if (['text'].indexOf(message.type) > -1) {
    // Error: Property 'text' does not exist on type 'ImageMessage'
    message.text = message.text.trim();
}

Playground Link: Provided

Related Issues: #9842

My argument is that if (message.type === 'text') should be considered equivalent to if (['text'].includes(message.type)).

It might seem irrelevant on a small example like this, but if the array (['text']) is large, the workaround is difficult to maintain.

Activity

added
SuggestionAn idea for TypeScript
Too ComplexAn issue which adding support for may be too complex for the value it adds
on Jan 21, 2020
RyanCavanaugh

RyanCavanaugh commented on Jan 21, 2020

@RyanCavanaugh
Member

This pattern doesn't occur often enough in ways that would produce useful narrowings to justify implementing whatever we'd have to do to detect it

iansan5653

iansan5653 commented on Jun 12, 2020

@iansan5653

If #36352 were revisited, the includes function could be a lot more useful if the return type were an assertion, like:

interface Array<T> {
-    includes(searchElement: T, fromIndex?: number): boolean;
+    includes(searchElement: any, fromIndex?: number): searchElement is T;
}

Then, this would be fine:

if(["text"].includes(message.type)) {
  message.type; // "text"
}

IMO this is a much better way to type it than the current restriction, considering that the way it's currently typed makes includes pretty much useless with arrays of enumerated types.

indexOf is a bit trickier and I wouldn't necessarily support changing that definition.

JGJP

JGJP commented on Oct 1, 2020

@JGJP

@RyanCavanaugh What is the reason for practically making .includes() useless in conditionals? Is it really "we don't think enough people do this"? What is this assumption based on?

RyanCavanaugh

RyanCavanaugh commented on Oct 1, 2020

@RyanCavanaugh
Member

@JGJP We didn't make it behave this way; this is the behavior absent the implementation of a feature that would cause it behave the way the OP is proposing. None of the existing narrowing mechanics apply here; we're talking about probably a thousand lines of new code to correctly detect and narrow arrays based on these methods, along with a performance penalty paid by every program because the control flow graph would have to be more granular to set up the possible narrowings caused by any method call, along with a bug trail and confusion trail caused by people expecting non-method versions of these functions to also behave the same way.

The proposed includes definition above is not correct because that's now how type guards work. You can trivially induce an unsoundness to occur using that definition:

interface Array<T> {
    includes2(searchElement: any, fromIndex?: number): searchElement is T;
}

declare let s: string | number;
if (["foo"].includes2(s)) {

} else {
    // 's' might be "bar" or any other string that's not "foo"
    s.toFixed();
}
JGJP

JGJP commented on Oct 2, 2020

@JGJP

@RyanCavanaugh Thanks for your detailed reply.

I can appreciate that it could be super difficult to implement what is suggested in this issue, but how about just not implicitly typing the array? Why are other variables implicitly typed as any but these arrays here aren't any[]? I would personally prefer that behavior, because it would allow us to use .includes() for its intended purpose (IMO) and we could configure the type error with noImplicitAny. Typescript is supposed to be building on Javascript, but in this case it's restrictive, and I don't see how the default behavior is useful (if not assigned to a variable).

RyanCavanaugh

RyanCavanaugh commented on Oct 2, 2020

@RyanCavanaugh
Member

I'm not sure what you're proposing.

The current typing detects many kinds of errors, like

function fn(s: string) {
  const bannedUsers = ["bob", "alice"];
  if (bannedUsers.includes(s.toLowerCase)) {
    // you are banned
JGJP

JGJP commented on Nov 18, 2020

@JGJP

@RyanCavanaugh
Sorry for the late reply. What we want to be able to do is something like this:

function fn(s: string | null) {
	const bannedUsers = ["bob", "alice"]
	if (bannedUsers.includes(s)) {
		// you are banned
	}
}

Here, bannedUsers is being implicitly cast as string[], which IMO is useful in some cases, but here the includes() call gives:

Argument of type 'string | null' is not assignable to parameter of type 'string'.
  Type 'null' is not assignable to type 'string'.

It also won't let us do things like bannedUsers.push(somethingThatsNotAString)

What solves these issues is changing the bannedUsers definition to:

const bannedUsers = ["bob", "alice"] as any[]

I would personally prefer it if any[] was the default type of an array, instead of Typescript trying to type it without being able to look into the future to check usage. I can understand, however, that other users won't want this as the default behavior, so how about being able to set an option in tsconfig.json?

RyanCavanaugh

RyanCavanaugh commented on Nov 18, 2020

@RyanCavanaugh
Member

@JGJP see #26255; the problem here is that includes should really be bivariant instead of covariant. There's no way we would add an option for making all arrays any[]; just use JavaScript if you don't like type errors 😉

JGJP

JGJP commented on Nov 19, 2020

@JGJP

@RyanCavanaugh I'm wondering why you're linking that issue, because it just shows the community making reasonable arguments and trying to come up with a solution, whereas your general stance seems to just be just use Javascript. Is this really the best we can do?

RyanCavanaugh

RyanCavanaugh commented on Nov 20, 2020

@RyanCavanaugh
Member

@JGJP Why doesn't TypeScript already have every feature it will eventually need? The answer is that we haven't designed or completed those features yet, which is why we have an issue tracker and are employing a large team of developers to work on it. Having humans work on the product at a finite speed is in fact the best we can do, for now.

22 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    SuggestionAn idea for TypeScriptToo ComplexAn issue which adding support for may be too complex for the value it adds

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @iansan5653@HolgerJeromin@jaredrethman@JGJP@RyanCavanaugh

        Issue actions

          .includes or .indexOf does not narrow the type · Issue #36275 · microsoft/TypeScript