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

Array.isArray refinement loses the array's type #32497

Closed
lll000111 opened this issue Jul 20, 2019 · 12 comments · May be fixed by #49855
Closed

Array.isArray refinement loses the array's type #32497

lll000111 opened this issue Jul 20, 2019 · 12 comments · May be fixed by #49855
Labels
Fix Available A PR has been opened for this issue Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@lll000111
Copy link

lll000111 commented Jul 20, 2019

I realize there are a number of related issues. However, some where about ReadonlyArray, some didn't see directly related at all even though it was pointed there.

Also, this is a much smaller example, just a single line of code really:

Playground link

function demo(it: Iterable<number>): number[] {
    // THIS LINE: "array" is any[]
    const array = Array.isArray(it) ? it : [...it];
    // Deliberate: n.length is a string method, but no error because of type "any" for n
    return array.map(n => n.length);
}

I know people will point to this ir that implementation or design decision detail — honestly guys, you got it REVERSED. The tool should serve the purpose, not the other way around!

That the array loses all it's type information in such a simple example is A BUG.

And it is possible to do much better: See here! I'm only pointing this out because at times the responses sound like "We tried everything but it is just not possible" when the competition shows that it is actually quite possible.

@lll000111 lll000111 changed the title Array,isArray refinement loses the array's type Array.isArray refinement loses the array's type Jul 20, 2019
@AnyhowStep
Copy link
Contributor

As a workaround, add overloads to Array.isArray() using declaration merging?

@fatcerberus
Copy link

fatcerberus commented Jul 20, 2019

Don’t underestimate how difficult things like this are from a theoretical perspective. Humans are much better at figuring these things out than computers.

In the general case, the compiler can’t safely deduce that Generic1<number> —> Generic2<number> unless both are in the union to be narrowed, because the type parameter might be used for different purposes in both types.

In this specific case, though, adding an overload:

isArray<T>(x: Iterable<T>): x is Array<T>

would likely suffice. I suspect this is probably how Flow does it.

@fatcerberus
Copy link

fatcerberus commented Jul 20, 2019

Proof of concept:
Try in TS Playground

interface ArrayConstructor
{
    isArray<T>(arg: Iterable<T>): arg is Array<T>;
}

function demo(it: Iterable<number>): number[] {
    const array = Array.isArray(it) ? it : [...it];
    return array.map(n => n.length);  // error: 'n' is number
}

For the record, the current declaration of isArray is:

    isArray(arg: any): arg is Array<any>;

I wonder if that can be improved. I'll need to give that some thought and maybe I'll open a PR.

@lll000111
Copy link
Author

lll000111 commented Jul 20, 2019

Don’t underestimate how difficult things like this are from a theoretical perspective

I don't. I too write code. And...?

A problem is a problem. If you decide to solve it what's the point of saying in the middle "okay now it's too difficult". I trust that there are very capable people working on this project who will find a way. :-)

@fatcerberus
Copy link

And...? :-)

Then you should already know that things that appear trivial can, in fact, turn out to be intractable, and being combative about that helps no one.

But anyway, in this case TS indeed has a mechanism (overloads) that can solve the problem, see above.

@lll000111
Copy link
Author

lll000111 commented Jul 20, 2019

and being combative about that helps no one.

So why are you being combative? Please check a mirror. Did you a) (only) respond to the issue, or did you b) choose to unnecessarily attack my experience? Not to mention that half of your first response was about something I had already covered in my initial post.

Also, I know the problem can be solved. So your point is? If a problem can be solved using "obvious methods" it does not have to be actually done (no need to report the issue))? Are you a mathematician? The theoretical solution is enough, "I'll leave it to the reader to complete this trivial proof for themselves"?

@kitsonk
Copy link
Contributor

kitsonk commented Jul 21, 2019

So why are you being combative?

So feels like it is the other way around to me. 🤷‍♂

@RyanCavanaugh
Copy link
Member

@lll000111 your attitude here makes my day measurably worse. Lots of people here manage to be constructive and positive and I'd appreciate your effort in not starting fights with us or other contributors. I've asked you to do this once already and am not going to ask a third time - if you continue to be combative to people just trying to help, we'll be asking you to discontinue your engagement on the issue tracker.

Turning to the issue at hand, I think we need to figure out what the right solution is relative to #17002. In that issue (Array.isArray and ReadOnlyArray<T>), it seemed like the preference from people was that narrowing e.g. string | ReadonlyArray<number> with isArray should yield ReadonlyArray<number>. not Array<number>.

It'd be confusing if string | Iterable<number> narrowed to Iterable<number> but narrowing Iterable<number> produced Array<number> - the isArray type guard should uniformly either produce a subtype or filter a union, not do one or the other depending on whether the argument type has some unrelated (e.g. known-false-returning) union constituents.

Also an open question: surely it's not just Iterable and ReadonlyArray that want special treatment? Are there other types? How does this impact #28916

@RyanCavanaugh RyanCavanaugh added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Jul 22, 2019
@lll000111
Copy link
Author

@RyanCavanaugh Please don't @-mention me in threads I'm not subscribed to. I refer to what I already wrote above. Your comment is insulting and more than useless. Go and learn some manners!

@fatcerberus
Copy link

@RyanCavanaugh Interestingly, writing a type predicate x is ReadonlyArray<T> doesn’t actually work from my tests: it narrows to T[] (sans readonly-ness) anyway.

@zpdDG4gta8XKpMCd
Copy link

bitch needs a slap

@microsoft microsoft locked as too heated and limited conversation to collaborators Jul 22, 2019
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jul 23, 2019

Alright, I am going to lock this issue. We can't tolerate abrasive language or tone on this issue tracker, and it has clearly become a problem here, so I'm closing the thread. Insults, disrespectful tone, and consistently abrasive language are violations of our code of conduct.

Keep in mind that violations of the code of conduct can result in being banned. I always try to remind participants that we are all striving to make TypeScript better - but if you want to participate, the only way to do so is with respect. If you cannot do that, we cannot allow you to engage on the issue tracker.

@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Aug 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fix Available A PR has been opened for this issue Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
8 participants