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

Current type for Array.prototype.includes may reject valid use cases #53904

Closed
yw662 opened this issue Apr 18, 2023 · 11 comments
Closed

Current type for Array.prototype.includes may reject valid use cases #53904

yw662 opened this issue Apr 18, 2023 · 11 comments
Labels
Duplicate An existing issue was already created

Comments

@yw662
Copy link

yw662 commented Apr 18, 2023

lib Update Request

Configuration Check

My compilation target is ESNEXT and my lib is ESNEXT.

Missing / Incorrect Definition

This is lib.es2016.array

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

Sample Code

Problem may relevant:

const a = [1, 2, 3, 'a', 'b', 'c'] as const
const b = [1, 2, 3, 4, 5, 6] as const

const n = b[Math.floor(Math.random()*6)]
a.includes(n) // type error but should pass 

Suggested fix:

interface Array<T> {
    includes<I = T, E = T>(this: readonly I[], v: E, from?: number): v is I & E
}

And for ReadonlyArray as well.

Documentation Link

Array.prototype.includes, but irrelevant.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Apr 18, 2023
@RyanCavanaugh
Copy link
Member

Please search for duplicates before logging new issues

@yw662
Copy link
Author

yw662 commented Apr 18, 2023

@RyanCavanaugh I cannot find any open issue on this

@fatcerberus
Copy link

@RyanCavanaugh All the dupes of this tend to get closed and duped to #14520; anyone searching for open issues about this is unlikely to find that one organically.

@yw662
Copy link
Author

yw662 commented Apr 18, 2023

And I have no idea why this is considered a dup of #14520. That one has nothing to do with this since 1 | 2 | 3 | 4 | 5 | 6 is not a subtype of 1 | 2 | 3 | "a" | "b" | "c" or the opposite.
It is like, even if the array type T has no intersection with the search element type E, it does not always mean an error. It will always be a false (in that case) but it should be like "a constant condition may indicate something wrong", not the Array.prototype.includes part.

@RyanCavanaugh
Copy link
Member

There are five duplicate issues with Array.prototype.includes in the title.

The missing type system feature to represent this would be #14520.

@yw662
Copy link
Author

yw662 commented Apr 18, 2023

I am not requesting a type system feature, I am requesting a change to the signature.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Apr 18, 2023

The proposed signature allows any argument, which we consider to be too loose:

a.includes({ foo: 23 }) // should fail, but doesn't

In general most feedback we get is that TS should be stricter by default (preferring false positives over false negatives) and people should have to "do something" to get looser behavior. The current includes behavior follows that line of thinking.

@yw662
Copy link
Author

yw662 commented Apr 18, 2023

Ok I guess I need to find a way to "if some type is never then raise a type error" to get this pass.

@yw662 yw662 closed this as completed Apr 18, 2023
@RyanCavanaugh
Copy link
Member

Just to note, you can always add in whatever signature you like into a global declaration merge. It'll be selected first if it matches.

@Mateusnasciment
Copy link

To enforce stricter type checking, you can manually add the suggested fix to a global declaration merge. This will ensure that the custom signature takes precedence and provides the desired type checking behavior.

Please note that this fix specifically addresses the behavior of Array.prototype.includes and does not cover broader type system features. For further information on the general representation of type system features, refer to issue #14520.

@RyanCavanaugh
Copy link
Member

To be clear, the thing that you do if you have the global signature merge is make type checking less strict, not more strict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants