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 type guard not working in 5.0 #53395

Closed
Hypnosphi opened this issue Mar 20, 2023 · 12 comments
Closed

Array.isArray type guard not working in 5.0 #53395

Hypnosphi opened this issue Mar 20, 2023 · 12 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@Hypnosphi
Copy link

Hypnosphi commented Mar 20, 2023

Bug Report

πŸ”Ž Search Terms

Operator '>' cannot be applied to types

πŸ•— Version & Regression Information

  • This changed between versions 4.9.5 and 5.0.2

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

declare const a: readonly number[] | number

const isPositive = Array.isArray(a) ? a.every(x => x > 0) : a > 0

πŸ™ Actual behavior

Operator '>' cannot be applied to types 'number | readonly number[]' and 'number'.

πŸ™‚ Expected behavior

No error, as Array.isArray(a) is true for arrays

@ahejlsberg
Copy link
Member

ahejlsberg commented Mar 20, 2023

The thing that changed here is that we more strictly check the > operator. In both 4.9 and 5.0, the type of a in the false branch is readonly number[] | number and not just readonly number[]. The reason for that is that Array.isArray checks against a non-readonly any[].

@zardoy
Copy link
Contributor

zardoy commented Mar 20, 2023

The reason for that is that Array.isArray is checks against a non-readonly any[].

Also run into it recently, but #17002 captures the issue here. This workaround #17002 (comment) fixes your problem

@fatcerberus
Copy link

fatcerberus commented Mar 21, 2023

The reason for that is that Array.isArray is checks against a non-readonly any[].

Hmm, so the upshot is that there's a (built-in!) type predicate that can return true for something which is known to not match the check type (readonly number[] is not a valid any[])... seems like that's something that should be fixed.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 21, 2023

There's no defect here. The above example is basically the same as

declare const a: Animal | Plane;
const p = Cat.isCat(c) ? c.meow() : c.fly()

But obviously a could be a Dog, which can neither meow nor fly.

If you made a new (structural) type that had all the members of Array except push, then that'd be legal to inhabit a, but if Array.isArray returned true for it, then you'd be allowed to call push on it, which is wrong, but if Array.isArray returned false for it, then we'd incorrectly say that a is a number in the false case. What people really want is a function that says "does it have all the non-mutating members of Array?", but that isn't what Array.isArray does.

I think the intuition is that readonly number[] means "A fully-real Array, but I pinky-promise not to mutate it" but, FBOFW, that's not what it is.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 21, 2023
@fatcerberus
Copy link

@RyanCavanaugh I’m not sure I follow the analogy - Cat.isCat() would clearly return false for a Dog, but Array.isArray returns true for all possible inhabitants of readonly number[]. So it’s more like calling Animal.isAnimal(). At any rate this feels like it makes Array.isArray largely useless as a type guard.

It still feels like a defect because effectively what we have here looks like function isAnimal(x: unknown): x is Cat

@RyanCavanaugh
Copy link
Member

but Array.isArray returns true for all possible inhabitants of readonly number[]

According to our type system, it doesn't. Any object with all the non-mutating members of Array<T> is a legal readonly T[].

@fatcerberus
Copy link

According to our type system, it doesn't.

Indeed, so what we have in practice is an isAnimal masquerading as an isCat, because basically 100% of readonly T[] in real code are going to be real arrays at runtime. My recent comment in another issue about β€œsleight of hand” in the type system has never felt more relevant πŸ˜…

@RyanCavanaugh
Copy link
Member

It's funny (like actual ha-ha funny) to me how doing the theoretically-correct thing is either clearly correct on its own merits, or total wonky weirdness, depending on who's looking at it when. I know my own view bounces around all the time on that kind of thing!

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@jcalz
Copy link
Contributor

jcalz commented Aug 2, 2023

For what it's worth I think it's unfortunate that this behavior is "working as intended". I have a hard time worrying about theoretical non-Array objects that structurally match readonly any[], when nobody worries about theoretical non-Array objects that structurally match any[]. And it is theoretical, right? Does anyone know of real-world code that makes use of such a monster and which also uses Array.isArray() on it?

@crizzis
Copy link

crizzis commented Aug 2, 2023

I think the intuition is that readonly number[] means "A fully-real Array, but I pinky-promise not to mutate it" but, FBOFW, that's not what it is.

This misses the point of what the purpose of type guards is. A type guard is supposed to answer the question: 'is this type safely assignable to X', and of course, a T[] is safely assignable to a readonly T[], because it has a narrower interface.

(the other way around would of course be wrong, but that's not what is being discussed here)

(also, isn't I pinky-promise not to mutate it the whole idea behind readonly arrays? If I am a function and my parameter is of type readonly, then I am making a promise to reference the parameter as if it were readonly, there's literally nothing more to it than that).

@jcalz
Copy link
Contributor

jcalz commented Aug 2, 2023

If there were a non-Array object o assignable to readonly any[] but not assignable to any[], then Array.isArray(o) would certainly return false. Currently that means "o is not an any[]", which is valid. But the suggestion here would make that mean "o is not a readonly any[]", which is invalid. So the suggestion does seem to hinge on the prevalence of these theoretical non-Array objects that conform to readonly any[] but do not conform to any[]. I am skeptical that such things exist, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

8 participants