-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Array.includes
type is too narrow
#26255
Comments
See #14520 for discussion of upper-bounded generics. |
Thanks, closing as duplicate. |
@OliverJAsh @RyanCavanaugh : I’m not sure if this is an exact duplicate, but more to the point, Goal Number 7 says that all runtime behavior of JS should be preserved. Since in normal JS Currently, the implementation is this: interface Array<T> {
includes(searchElement: T, fromIndex?: number): boolean;
} but it could be this: interface Array<T> {
includes<U>(searchElement: U, fromIndex?: number): U extends T ? boolean : false;
} That way, The same goes for interface Set<T> {
has<U>(value: U): U extends T ? boolean : false;
}
interface Map<K, V> {
has<U>(key: U): U extends K ? boolean : false;
} |
Please read https://stackoverflow.com/questions/41750390/what-does-all-legal-javascript-is-legal-typescript-mean |
@RyanCavanaugh Thanks for the link to Stackoverflow; as indicated on Stackoverflow:
However, I'm curious as to why TypeScript would consider: includes(searchElement: any, fromIndex?: number): boolean; to be incorrect? The
There's no indication this function should not be called with types that do not exist in the array. Somewhat to the contrary, the first step of the aforementioned SameValueZero[2] algorithm is:
As such At least based on my current understanding, it seems unusual to me that...: const KNOWN_VALUES = Object.freeze(['a', 'b', 'c'])
function foo(input?: string | null) {
if (KNOWN_VALUES.includes(input)) { // <== TS2345
// ... Do thing
}
} gives the error...:
given that the ECMAScript spec documents this as being a case in which I can safely expect Granted, I understand TypeScript goes above and beyond JS; it's inherently adding type-safety to a language (and spec) that never had it in mind. The StackOverflow answer mentioned above even specifically calls out referring to the spec as an invalid reason to impact TypeScript's design. The entire point of TypeScript of course being that it adds additional safety. However, I'm not sure under which circumstances the above error provides additional type safety. Somewhat to the contrary, to work around the error I must train myself to do something that is unsafe e.g. use a Just to clarify, my query isn't specifically regarding However, admittedly I'm approaching this from a particular mind-set based on my own experience and use-cases; which quite frankly may not be enough to understand the situation at large. Consequently, there may be something immediately apparent to others that I'm simply not seeing. As such, if I'm off base here, I would absolutely appreciate any time anyone is willing to afford me such that I can be better educated on the matter. [1] https://www.ecma-international.org/ecma-262/9.0/index.html#sec-array.prototype.includes |
Sorry, the tldr; of my post above is... If the type definition of includes(searchElement: any, fromIndex?: number): boolean; would it actually break (or cause someone to "break") a single piece of software written in TypeScript? My current thinking, is no. However, that may be incorrect. Even still, that alone isn't reason to change the signature. Assuming the change wouldn't break any real-world software. Would changing the signature worsen or improve code comprehension? I'm of the opinion (at present) that it would actually significantly improve code comprehension as it would allow a common coding pattern to be utilized without littering code with unnecessary (and technically incorrect) casts and/or unnecessary type-checks and null checks. |
⁉
You're arguing is that this code is totally fine and has no obvious problems: function foo(arr: string[], content: string, index: number) {
if (arr.includes(index)) {
// Do something important
}
} This code is in fact extremely suspicious! Type checking is not a valuable operation if it's not capable of identifying code that's very likely to be wrong. If/when we do get upper-bounded constraints, it absolutely makes sense to make
Changing every built-in function to accept |
@RyanCavanaugh That's a fairly reasonable example of suspicious code, something I admittedly hadn't considered as being a possibility. I'm not sure how frequently this sort of thing would occur in practice. I suspect this would be highly infrequent, but honestly I can't back that up. If it does occur frequently, then it may well trump everything that follows... However, my concern is that this function has been given a narrow type constraint simply because this is TypeScript, so there's a desire to add types to existing APIs. However, presumably this should only be done for the purpose of facilitating correctness, if the function already has well defined and well understood semantics, then altering these semantics may not be desirable. In this particular instance I believe it leads developers to write code that is non-obvious, unnatural, and therefore more likely to be error prone. Take for example the sanitation use case. A developer wants to check if some input is in some collection (technically this applies to sets as well as primitive arrays). The natural way to write this code is: const KNOWN_VALUES = Object.freeze(['a', 'b', 'c'])
function isKnownValue(input?: string | number) {
return KNOWN_VALUES.includes(input)
} It's not clever or complicated code, it matches the developers intention precisely, yet TypeScript rejects the code. So, what does the developer do to correct this issue? A cast, a type check? Let's try the latter: const KNOWN_VALUES = Object.freeze(['a', 'b', 'c'])
function isKnownValue(input?: string | number) {
return typeof(input) === 'string' && KNOWN_VALUES.includes(input)
} Great, it passes. We've got an extra check in there that will be compiled to JS, but "it's a small price to pay for type safety" 👍 Now consider we want to handle numbers in our known value set: const KNOWN_VALUES = Object.freeze(['a', 'b', 'c', 1, 2, 3])
function isKnownValue(input?: string | number) {
return typeof(input) === 'string' && KNOWN_VALUES.includes(input)
} Uh oh! This TypeScript compiles without errors, but it's not correct. Where as our original "naive" approach would have worked just fine. Why is that? Where is the breakdown here? It's because TypeScript's type system got in the way of the developer's initial intent. It caused us to change our code from what we intended to what it allowed. It was never the developer's intention to check that Generally TypeScript doesn't get in the way, for the most part it encourages correctness - honestly it's great. However, I think in this particular circumstance, it's not facilitating correctness or behaving how developers would expect. Note: To work around the initial TS error, I could have used a cast instead of an explicit type-check. This would be even more unnatural as we'd essentially intentionally be performing a cast that we know may be inaccurate - however, it would have the benefit that the error above would have been averted. Of course, reading the code after adding in Edit: Some details of my example were originally poorly chosen i.e. the example was constructed in a way that developer would probably have done a null check rather than a |
I'm hitting a similar issue but in union types: const allDogNames = {
django: 'django',
tom: 'tom',
buster: 'buster',
} as const;
// type -> "django" | "tom" | "buster"
type AllDogNames = (typeof allDogNames)[keyof typeof allDogNames];
const checkDogOnShortlist = (name: AllDogNames): boolean => {
return [allDogNames.django, allDogNames.tom].includes(name);
};
To get around this you have to declare the shortlist type that it "could" be any of the names even if the defined array clearly is not: const checkNameForDog = (name: AllDogNames): boolean => {
// type -> ("django" | "tom" | "buster")[]
const dogShortList: AllDogNames[] = [allDogNames.django, allDogNames.tom];
return dogShortList.includes(name);
};
Behaviour doesn't seem expected in that it would seem reasonable for typescript to determine if you are comparing against a list of string literals with a string literal then this is Ok? |
How about using this signature? function includes<T, U extends T>(arr: readonly U[], elem: T): elem is U {
return arr.includes(elem as any);
} This isn't too restrictive, and provides a nice type type guard. const ARR = ['a', 'b', 'c'] as const;
const x = 'a'; // type is "a"
if (includes(ARR, x)) {
console.log(x); // type is "a"
}
const x = 'x'; // type is "x"
if (includes(ARR, x)) {
console.log(x); // type is never
}
const x = 'doesnt matter' as string; // type is string
if (includes(ARR, x)) {
console.log(x); // type is "a" | "b" | "c"
} |
Also hit this issue when trying to look if the users locale setting was in the list of supported locales and otherwise default to the default language. A bit unintuitive having to do type casts before using It is a real world case of the example Benjamin-Dobell posted. |
My hard-coded decision for real-world cases: type DU = 'a' | 'b' | 'c';
let x: DU;
let y: string;
type THelper<T, U> = T extends string
? {[P in T]: P extends U ? P : never}[T] extends {[P in T]: unknown}
? U
: never
: U;
function includes<T, U extends THelper<T, U> extends never ? unknown : never>(
arr: readonly T[],
searchElement: U,
fromIndex?: number,
): boolean {
return (arr as any).includes(searchElement, fromIndex);
}
/*1*/ if (includes(['a', 'c'] as const, x)) {
// no error
doSmth();
}
/*2*/ if (includes(['a', 'd'] as const, x)) {
// error
doSmth();
}
/*3*/ if (includes(['a', 'c'] as const, y)) {
// no error
doSmth();
}
/*4*/ if (includes(['a', 'c'] as const, 4)) {
// error
doSmth();
}
function doSmth() {} @OliverJAsh, it seems to be working fine with your example. enum ReallyLongEnumName {
a,
b,
c,
}
let x: ReallyLongEnumName;
let y: string;
/*1*/ if ([ReallyLongEnumName.a, ReallyLongEnumName.c].includes(x)) {
// no error
doSmth();
}
function doSmth() {} |
I agree with @Benjamin-Dobell and others; it's too restrictive if we can't check for any type belonging to an array. I think there are good foundational reasons why Java's signature on all collection types allow any To give one additional example, suppose I have an exhaustive list of a union type, and I want an
Or... at the very least we could have an additional method, or a type param to allow widening the accepted type, e.g. using type defaults like:
|
The root issue here is certain functions that should act like type assertions, but do not do so. Here's the current implementation: 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: T, fromIndex?: number): boolean;
} A change of the interface to the below serves the expected purpose without making assumptions on the array (const or otherwise) and the values it contains. 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: unknown, fromIndex?: number): searchElement is T;
} What are the objections to such a change? Is a PR welcome here? I don't even know that this would be a breaking change, as existing code should "just work" given the broadening of the type. I am open to know if there are edge cases though so I can include them in the test cases for a PR. |
Please re-open this issue, the current implementation makes const Foo = {
'a': 'foo',
'b': 'bar',
'c': 'xyz',
'd': '123',
} as const;
type Foo = typeof Foo[keyof typeof Foo];
// Foo.c is not assignable to type '"foo' | 'bar'"
const hasC = [Foo.a, Foo.b].includes(Foo.c); |
I found the following trick that unblocks me right now in a rush. const FooArr = [
"Hi",
"Bye",
] as const;
const hasC1 = FooArr.includes("Chocolate"); // Doesn't work, TS is too strong in checking -- Argument of type '"Chocolate"' is not assignable to parameter of type '"Hi" | "Bye"'.(2345)
const hasC2 = (FooArr as string[]).includes("Chocolate"); // Doesn't work either -- Conversion of type 'readonly ["Hi", "Bye"]' to type 'string[]' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first. The type 'readonly ["Hi", "Bye"]' is 'readonly' and cannot be assigned to the mutable type 'string[]'.(2352)
const hasC3 = ([...FooArr] as string[]).includes("Chocolate"); // Only hasC3 works! I don't advise this, but since I am in a rush, I'm gonna leave it in my code with a link to this ticket as |
@eyedean Casting as As mentioned above, it's way too restrictive, and should be changed to a type assertion instead. P.S. IMO, title case should be for classes / React components / types, and baking types into names like with that |
While I agree this signature needs to change, I'm a little confused by all of these weird (and possibly dangerous) workarounds casting the array instead of casting the search element. Can anyone explain this to me? type ArrT = 'foo' | 'bar' | 'baz';
const a: ArrT[] = ['bar', 'baz'];
(a as any).includes('hello'); // why this (or some variant)
a.includes('hello' as any); // instead of this? Of course casting at all is not what we want here, but to me it seems like casting the search element is the lesser of two evils. Also, the type guard signature gets my vote for best temporary workaround. |
I've only been using typescript a month. So far, I'm a fan, but this particular typing for What I tried to use:
What I ended up using to make typescript happy:
Or I could have been "more correct" and used:
🤮 I hate when languages make me be clever in the name of safety, but add bloat that makes my code harder to read and easier to introduce bugs. |
@RyanCavanaugh (et. al) I see we keep referencing this issue as a duplicate to others, even referencing my comment's suggested change in some cases, but this issue remains closed. Can we at least re-open this issue if it is going to be referenced as a duplicate given TS now supports more functionality than when originally closed? Failing that, can someone also answer my previous question regarding if setting a type assertion will be accepted as a PR or at least tell me how to make a proper request for the suggested change? I'd love to help support the team and get this in the code base if it means you won't need to reference a closed ticket as duplicate any longer ;) |
@kevinpeno The way we would fix this is by implementing #14520, so we don't need two issues tracking the same thing. I typically send people here first so they can understand that they could have searched better and found this 🙂. Regarding the type guard change, please open a new issue for that -- this issue is a complaint about being unable to search for supertypes in an array (which should be valid). I don't think we would take that change verbatim since it would allow |
@RyanCavanaugh But it's not an error…? |
@vdh by that logic, almost anything should be allowed. Why can't I write TS has many checks that are based on our judgment about what's "over the line" in terms of intentional vs unintentional behavior of JavaScript. If you want If you don't like the fact that TS disallows many operations that are "allowed" by JavaScript, the good news is that JavaScript still exists. |
I do agree with your response to me that it should allow "supertypes" of the value (existing or not; e.g. Thanks for your response. |
@RyanCavanaugh That's a bit of a nasty response to make. Instead of seriously answering whether or not it's a good candidate to type it as a real type guard, you instead mock me as if I'm somehow suggesting all garbage code of any wrong type should be allowed.
Of course an edge case of an inlined I apologise for neglecting to detail the nuance of the |
Please don't misinterpret technical discussions as personal attacks. Anyway, I think you're missing the fact that we already agree here - supertypes absolutely should be allowed inputs to
Absolutely! It should be flagged as an error, by what mechanism? That's the thing here; we already agree that supertypes should be allowed inputs, it's just that we don't have any way to make supertypes allowed without also allowing everything else, which is why this issue is just a manifestation of #14520. Once that was implemented, we absolute would change the signature to something like The options on the table (without implementing #14520) are:
We (the team) think that "anything" is a bad answer here because of examples like Just to spell this out explicitly: There's no point discussing a type guard in the case where only subtypes are allowed, since that will never narrow the type of the argument. Regardless of which behavior of the two currently available with existing mechanics you think is clearly preferable, the fact is that one of them allows users to opt into the other, and the other doesn't. When we're faced with that situation, unless the choice is truly obvious (and IMO it's not), then we're going to go with the one that keeps flexibility on the user side.
That isn't the original suggestion; it's a comment. The original suggestion is at the top of this page and it just says that supertypes should be allowed inputs to |
You can also review other duplicate proposals for the type guard behavior where it has been pointed out that treating this as a type guard is unsound: 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();
} For the type guard flavor to be correct, we'd need #15048. So ultimately you're blocked one of three things here:
|
I may be overlooking some other case where this would fall apart in some other way, but from what I can tell, TypeScript 4.1's template literal types and some trickery would allow a definition for interface ReadonlyArray<T> {
includes<S, R extends `${Extract<S, string>}`>(
this: ReadonlyArray<R>,
searchElement: S,
fromIndex?: number
): searchElement is R & S;
} 1. Input is correctly refined to be one of the strings in the haystack arraydeclare let s: number | string
if ((['foo', 'bar'] as const).includes(s)) {
// s is 'foo' | 'bar' here
if (s === 'baz') {} // Error as expected, since this is would always be false
} You do need the declare let arr: ReadonlyArray<'foo' | 'bar'>
if (arr.includes(s)) { then that works too. 2. It doesn't refine other than string literal typesThe unsoundness @RyanCavanaugh mentioned above isn't here anymore, since this never refines if ((['foo', 'bar'] as const).includes(s)) {
} else {
// s is string | number here
s.toFixed(); // [ts] Property 'toFixed' does not exist on type 'string'
} For this to work properly, we need the template literal in the generics constraint extends `${ string }` With just declare var arr: ReadOnlyArray<string>
if (arr.includes(s)) {
} else {
s.toFixed(); // We want error here!
} With the template literal constraint, TS won't use our new overload of the method for normal 3. Input values that can never even be strings are caught as errors(['foo', 'bar'] as const).includes(0) // Error as expected, albeit more verbose one than we'd like. This works thanks to the Admittedly, this all overloading stuff comes with a pretty big caveat; The error messages for incorrect parameter usage become much worse declare let arr: ReadonlyArray<number>
arr.includes('a') No overload matches this call...
Still, even if this will never land in TypeScript's builtin types, it may be worth a try in userland type declarations for people following this issue. |
Please reopen the issue. I use |
TypeScript Version: 2.9.2
Search Terms: array includes es2016 widen
Code
Array.includes
should allow thesearchElement
param to be a subtype of the array element type, e.g.Expected behavior:
No error.
The text was updated successfully, but these errors were encountered: