Skip to content
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

[feature request] Better Array.prototype.includes #17

Open
ssssota opened this issue Oct 15, 2022 · 17 comments
Open

[feature request] Better Array.prototype.includes #17

ssssota opened this issue Oct 15, 2022 · 17 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@ssssota
Copy link

ssssota commented Oct 15, 2022

The standard TypeScript type requires the first argument of the includes function to be a value in an array.
This requires that the type of the argument be known beforehand for union-type arrays (e.g. [1,2,3] as const).
We can see that this is not a semantic error, but we may find this inconvenient.

So please consider introducing the following types into this project:

interface Array<T> {
  /**
   * Determines whether an array includes a certain element, returning true or false as appropriate.
   * @param searchElement The element to search for.
   * @param fromIndex The position in this array at which to begin searching for searchElement.
   */
  includes(searchElement: any, fromIndex?: number): searchElement is T;
}

interface ReadonlyArray<T> {
  /**
   * Determines whether an array includes a certain element, returning true or false as appropriate.
   * @param searchElement The element to search for.
   * @param fromIndex The position in this array at which to begin searching for searchElement.
   */
  includes(searchElement: any, fromIndex?: number): searchElement is T;
}
@uhyo uhyo added the enhancement New feature or request label Oct 19, 2022
@uhyo uhyo self-assigned this Oct 19, 2022
@uhyo
Copy link
Owner

uhyo commented Oct 19, 2022

Thank you for suggestion!

I see that the suggested signatures make include much more convenient.
It would indeed start to allow some mistakes (like strings.includes(num)), but it wouldn't deviate from type safety.

Let's add this.

@graphemecluster
Copy link
Contributor

@uhyo Wait – By adding this it means that we will also need to deal with methods like findIndex – see microsoft/TypeScript#36554 (comment) and microsoft/TypeScript#26255. To maintain type safety, perhaps we can use a generic until microsoft/TypeScript#9252 is implemented:

interface Array<T> {
  includes<U>(searchElement: T extends U ? U : T, fromIndex?: number): searchElement is T extends U ? T : T;
}

@uhyo
Copy link
Owner

uhyo commented Oct 20, 2022

@graphemecluster Thank you for pointing this out. Personally I wouldn't extend this to methods like findIndex because we need to corrently type callback parameters. Anyway the generics approach is nice 🙂

@uhyo
Copy link
Owner

uhyo commented Dec 5, 2022

I tacked this and couldn't get to a perfectly working implementation. 😇 As mentioned above, we should only narrow in then-branch of include checks but it doesn't seem currently possible. Concretely the below testcase cannot pass.

  function checkNarrowing(val: "pika" | "chu" | "pikachu") {
    const strarr: ("pika" | "chu")[] = [];
    if (strarr.includes(val)) {
      expectType<"pika" | "chu">(val);
    } else {
      expectType<"pika" | "chu" | "pikachu">(val);
    }
  }

@uhyo uhyo added the help wanted Extra attention is needed label Dec 5, 2022
@uhyo uhyo removed their assignment Dec 5, 2022
uinz added a commit to uinz/better-typescript-lib that referenced this issue Jan 4, 2023
@aaditmshah
Copy link
Contributor

I tacked this and couldn't get to a perfectly working implementation. 😇 As mentioned above, we should only narrow in then-branch of include checks but it doesn't seem currently possible. Concretely the below testcase cannot pass.

  function checkNarrowing(val: "pika" | "chu" | "pikachu") {
    const strarr: ("pika" | "chu")[] = [];
    if (strarr.includes(val)) {
      expectType<"pika" | "chu">(val);
    } else {
      expectType<"pika" | "chu" | "pikachu">(val);
    }
  }

I think it's all right to narrow the type in the else branch as well. If strarr.includes(val) is false then we know that val can't be "pika" or "chu". Hence, the type of val can be narrowed to just "pikachu".

Consider the following example. Playground Link

const expectType = <A>(a: A): A => a;

type Subtype<A, B> = A extends B ? A : never;

type Supertype<A, B> = A extends B ? B : unknown;

interface Includes<A> {
    includes: <B>(searchElement: Supertype<A, B>) => searchElement is Subtype<A, B>;
}

declare const array: Includes<"foo" | "bar">;

declare const subtypeElement: "foo";

declare const sametypeElement: "foo" | "bar";

declare const supertypeElement: "foo" | "bar" | "baz";

declare const othertypeElement: "baz";

if (array.includes(subtypeElement)) {
    expectType<"foo">(subtypeElement);
} else {
    expectType<never>(subtypeElement);
}

if (array.includes(sametypeElement)) {
    expectType<"foo" | "bar">(sametypeElement);
} else {
    expectType<never>(sametypeElement);
}

if (array.includes(supertypeElement)) {
    expectType<"foo" | "bar">(supertypeElement);
} else {
    expectType<"baz">(supertypeElement);
}

if (array.includes(othertypeElement)) {
    expectType<never>(othertypeElement);
} else {
    expectType<"baz">(othertypeElement);
}

@aaditmshah
Copy link
Contributor

Oh, wait. No, I'm mistaken. Didn't see that strarr is empty here. Nevermind. Disregard my previous comment.

@aaditmshah
Copy link
Contributor

@ssssota You might not require narrowing at all. Consider using the following find function instead of Array#includes.

Playground Link

type Option<A> = { some: A } | null;

const find = <A>(needle: unknown, haystack: A[]): Option<A> => {
    for (const element of haystack) {
        if (element === needle) {
            return { some: element };
        }
    }
    return null;
};

declare const haystack: ("foo" | "bar")[];

declare const needle: "foo" | "bar" | "baz";

const option = find(needle, haystack);

const expectType = <A>(a: A): A => a;

if (option !== null) {
    expectType<{ some: "foo" | "bar" }>(option);
    expectType<"foo" | "bar" | "baz">(needle);
} else {
    expectType<null>(option);
    expectType<"foo" | "bar" | "baz">(needle);
}

@ssssota
Copy link
Author

ssssota commented Apr 3, 2023

@aaditmshah
Of course!
This Issue was created only as a "nice to have".
Naturally, it cannot be used in actual coding, so we have taken workarounds.
@uhyo
If this issue is not feasible, please feel free to close it.
The discussion here has been very useful to me. Thank you!

@graphemecluster
Copy link
Contributor

I just revisited this and thought of a solution (which’s overly ordinary that you may even have thought of it before) while replying @cm-ayf’s post tweet:
https://twitter.com/graphemecluster/status/1700470364000956844
However, the defect is that this doesn’t give any error directly; it just types the parameter never instead if T is not related to U.
I am not sure if there are any pitfalls though.
I have tried producing an error if T & U is never but none of my trials work. So I guess it’s still the best to play around with the compiler – my first thought is to add type ErrorIfNever<T> = T; to a library file and let the compiler handle the remaining things.
お二人とも東大生だ、憧れます……

@aaditmshah
Copy link
Contributor

aaditmshah commented Sep 14, 2023

I don't think it's possible to refine the type of the searchElement. However, we can at least widen the type of the input.

interface Array<T> {
  includes<U>(searchElement: T extends U ? U : T, fromIndex?: number): boolean;
}

This would allow you to write the following.

declare const array: ("foo" | "bar")[];

declare const searchElement: "foo" | "bar" | "baz";

if (array.includes(searchElement)) {
    expectType<"foo" | "bar" | "baz">(searchElement);
} else {
    expectType<"foo" | "bar" | "baz">(searchElement);
}

This works because "foo" | "bar" | "baz" is a supertype of "foo" | "bar".

However, it would prevent the following.

declare const array: ("foo" | "bar")[];

declare const searchElement: "baz";

if (array.includes(searchElement)) { // type error
    expectType<"baz">(searchElement);
} else {
    expectType<"baz">(searchElement);
}

This doesn't work because "baz" is neither a supertype nor a subtype of "foo" | "bar".

Playground Link

@ehoogeveen-medweb
Copy link

I think the above might be too permissive, because it allows search elements like string | { some: "random object" } as they extend the array type. From reading other discussions on this topic, that seems to be what the typescript developers are trying to avoid.

I'd like to suggest the following modification, which allows search elements like string for string literal tuples while disallowing unrelated union types:

type IfLiteralGetPrimitive<T> =
  T extends string ? string : T extends number ? number : T extends bigint ? bigint : T extends boolean ? boolean : T;

interface Array<T> {
  includes<U extends IfLiteralGetPrimitive<T>>(searchElement: T extends U ? U : T, fromIndex?: number): boolean;
}

Playground Link (I modified the original playground because it worked a bit differently and didn't permit the union - this playground matches the behavior I see when I add the overload to the global Array interface)

@graphemecluster
Copy link
Contributor

@ehoogeveen-medweb This would prevent searching an array of strings for a PropertyKey.

@ehoogeveen-medweb
Copy link

ehoogeveen-medweb commented Jan 31, 2024

PropertyKey as in string | number | symbol? Well, I guess you could modify IfLiteralGetPrimitive to something like

type IfLiteralGetPrimitive<T> =
  T extends string ? number | string | symbol : T extends number ? number : T extends bigint ? bigint : T extends boolean ? boolean : T;

so that all object keys are allowed when an array contains strings. I usually work with somewhat narrow object types like Record<string, ...> and it's not needed for those.

Playground link

Edit: I'm also not sure searching an array for a full PropertyKey really comes up in practice, as you usually get the keys through a method like Object.keys which only returns string keys. But maybe I'm missing something there.

@karlismelderis-mckinsey
Copy link

karlismelderis-mckinsey commented Jun 13, 2024

would it be possible treat searchElement as unknown?
I can imagine that before .includes call you don't really know what that value is or should not even care

it's totally valid case to do this and build, e.g. a type guard from it

const nums = [1,2,3] as const
nums.includes(4)

@KisaragiEffective
Copy link
Contributor

KisaragiEffective commented Jul 26, 2024

I tacked this and couldn't get to a perfectly working implementation. 😇 As mentioned above, we should only narrow in then-branch of include checks but it doesn't seem currently possible. Concretely the below testcase cannot pass.

  function checkNarrowing(val: "pika" | "chu" | "pikachu") {
    const strarr: ("pika" | "chu")[] = [];
    if (strarr.includes(val)) {
      expectType<"pika" | "chu">(val);
    } else {
      expectType<"pika" | "chu" | "pikachu">(val);
    }
  }

Not sure if this is best thing that we can have, but somehow works:

declare namespace NarrowBlocker {
    const dummy: unique symbol;
}

interface Array<T> {
  includes(searchElement: unknown, fromIndex?: number): searchElement is T & {[NarrowBlocker.dummy]: never};
}

function checkNarrowing(val: "pika" | "chu" | "pikachu") {
    const strarr: ("pika" | "chu")[] = [];
    if (strarr.includes(val)) {
            val;
        // ^?
    } else {
            val;
        // ^?
    }
}

playground.
image

@KisaragiEffective
Copy link
Contributor

I think the above is one of the best thing that we can have, as same "properties" are shown in following minimized case:
image

@cobaltt7
Copy link

I've found this to work well:

interface Array<T> {
    includes(
        searchElement: T | (WidenLiteral<T>),
        fromIndex?: number,
    ): boolean;
}

interface ReadonlyArray<T> {
    includes(
        searchElement: T | (WidenLiteral<T>),
        fromIndex?: number,
    ): searchElement is T;
}

type WidenLiteral<T> =
    T extends string ? string
    : T extends number ? number
    : T extends boolean ? boolean
    : T extends bigint ? bigint
    : T extends symbol ? symbol
    : NonNullable<T>;

It does not narrow Array#includes's return type since an array type does not necessarily represent exactly what is in the array, as mentioned above - we should only narrow the then branch. That isn't possible, but IMO a fair trade-off is narrowing the return type anyway on ReadonlyArrays. The type of constant arrays tends to equal its contents much more often, in my experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

8 participants