Remove assignability cases in getNarrowedType + an isArray improvement for readonly arrays

[Orta Therox (orta)]

Fixes#31155
Fixes#17002

There are two pieces to this PR:

• The fix for 'instanceof' changes type outside of 'if' statement #31155 which came from Fix #31155 - Remove assignability cases in getNarrowedType #32443
• Unblocking Array.isArray type narrows to any[] for ReadonlyArray<T> #17002 which stopped Fix #31155 - Remove assignability cases in getNarrowedType #32443

The 2nd was blocked on :

TLDR: any[] is assignable to IAction | ReadonlyArray, but it's not a subtype. The issue here is that readonly arrays are not subtypes of mutable arrays so we end up falling back to the intersection case.

Where :

interfaceIAction{x: string;}declareconstarg: IAction|ReadonlyArray<IAction>;constx=Array.isArray(arg) ? arg : [arg];

Is now legal due to changes in the isArray definition which takes into account readonly arrays.

/cc Jack Williams (@jack-williams)

[Ryan Cavanaugh (RyanCavanaugh)]

TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) user test this

[TypeScript Bot (typescript-bot)]

Heya Ryan Cavanaugh (@RyanCavanaugh), I've started to run the parallelized community code test suite on this PR at 855fd84. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

Heya Ryan Cavanaugh (@RyanCavanaugh), I've started to run the extended test suite on this PR at 855fd84. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

[Ryan Cavanaugh (RyanCavanaugh)]

TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) user test this

[TypeScript Bot (typescript-bot)]

Heya Ryan Cavanaugh (@RyanCavanaugh), I've started to run the parallelized community code test suite on this PR at ed01085. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

Heya Ryan Cavanaugh (@RyanCavanaugh), I've started to run the extended test suite on this PR at ed01085. You can monitor the build here.

[Ryan Cavanaugh (RyanCavanaugh)]

I don't think this is going to work. See https://mseng.visualstudio.com/Typescript/_git/Typescript/pullrequest/560730?_a=files for RWC breaks

[Jack Williams (jack-williams)]

Seems to be tripping up when the guarded value has type any and it picks both parts of the conditional when it should really pick the false branch. Could something like the following work?

declarefunctionisArray<T>(arg: T|{}): arg is Textendsreadonly(infer U)[] ? GroundArray<T,U>: any[];typeGroundArray<Textendsreadonlyunknown[],U>=[T]extends[U[]] ? any[] : T;

[Wesley Wigham (weswigham)]

isArray<T>(arg: T|{}): arg is Textendsreadonlyany[] ? (unknownextendsT ? never : readonlyany[]) : any[];

should also work to filter out any from the true branch.

[Orta Therox (orta)]

TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) user test this

[TypeScript Bot (typescript-bot)]

Heya Orta Therox (@orta), I've started to run the parallelized community code test suite on this PR at c69b255. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

Heya Orta Therox (@orta), I've started to run the extended test suite on this PR at c69b255. You can monitor the build here.

[Orta Therox (orta)]

OK, so that does clean up most of the errors in the RWC in the azure side - it looks like just a few rxjs issues and they all stem to their own custom isArray function which looks like:

exportconstisArray=(()=>Array.isArray||(<T>(x: any): x is T[]=>x&&typeofx.length==='number'))();

We're still seeing issues in this PR: typescript-bot#53

[XX:XX:XX] Error: /vscode/src/vs/base/browser/ui/actionbar/actionbar.ts(628,9): Type '(readonly IAction[] & any[]) | (IAction & any[]) | (IAction | readonly IAction[])[]' is not assignable to type 'readonly IAction[]'.
Type '(IAction | readonly IAction[])[]' is not assignable to type 'readonly IAction[]'.
Type 'IAction | readonly IAction[]' is not assignable to type 'IAction'.
Type 'readonly IAction[]' is missing the following properties from type 'IAction': id, label, tooltip, class, and 4 more

this could be because they have their own isArray in vscode which wouldn't propagate the readonly:

exportfunctionisArray(array: any): array is any[]{returnArray.isArray(array);}

I could update the vscode definition to the new definition to see if that fixes it

[Wesley Wigham (weswigham)]

Yeah, the custom isArray functions would need to adopt the same type, both to be assignment compatible and to behave the same way.

[Orta Therox (orta)]

Looks like I need a build to verify rxjs

TypeScript Bot (@typescript-bot) pack this

[TypeScript Bot (typescript-bot)]

Heya Orta Therox (@orta), I've started to run the tarball bundle task on this PR at c69b255. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

Hey Orta Therox (@orta), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/79666/artifacts?artifactName=tgz&fileId=4DD38BEE4F055DD0797EA952DE69E613AD7160224DB562CEDFF4A4AC7DFDA0FA02&fileName=/typescript-4.0.0-insiders.20200713.tgz"
}
}

and then running npm install.

---

There is also a playground for this build.

[Orta Therox (orta)]

TypeScript Bot (@typescript-bot) pack this

[Orta Therox (orta)]

I'll merge on tuesday when I'm off DT rotation 👍🏻

[Orta Therox (orta)]

Today is that tuesday, I'm going to merge 👍🏻

[Ryan Cavanaugh (RyanCavanaugh)]

Example for blog post

declareconstp: string|ReadonlyArray<string>;if(Array.isArray(p)){// 4.0: p: any[]// 4.1: p: readonly string[];// 4.0: OK// 4.1: Errorp.push(0);}

[Han Seoul-Oh (laughinghan)]

FWIW, possibly a better example for the blogpost is one where TypeScript narrowed the type incorrectly and unsafely, rather than merely providing a weaker-than-desired type, e.g.:

declarevarfoo: string[]|readonlynumber[]|nullif(fooinstanceofArray){foo;// at runtime, this may be string[] or readonly number[],// however, TypeScript <4.1 would narrow to just string[]foo.push('str');// incorrect and unsafe, but allowed by <4.1, now errors in Nightly}else{foo;// at runtime, strictly null, however TypeScript <4.1 would// narrow to readonly number[] | null}

(Playground Link)

[Felix Becker (felixfbecker)]

Orta Therox (@orta) I think this broke passing Array.isArray to other type-guard-accepting functions, e.g. array.filter(): #41610

[Victor Garcia (victorgarciaesgi)]

Orta Therox (@orta) I think this broke passing Array.isArray to other type-guard-accepting functions, e.g. array.filter(): #41610

It brokes a lot de array.map for me too. Only happens on recursive types for me #41609

[Martin Johns (MartinJohns)]

Orta Therox (@orta) / Ryan Cavanaugh (@RyanCavanaugh) This PR has the Breaking Change label, but I don't see any mention of it on the "Breaking Changes" documentation:

https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#typescript-41

[Yudhi Seymour (yseymour)]

This is broken for generics and other nontrivial types:

declarefunctionf1<Textendsany[]>(array: T): void;functionf2<T>(thing: T){if(Array.isArray(thing)){f1(thing);// Argument of type 'T & (T extends readonly any[] ? unknown extends T ? never : readonly any[] : any[])' is not assignable to parameter of type 'any[]'.}}

In 4.0, the type of thing was narrowed to T & any[] which was OK.

[Martin Johns (MartinJohns)]

Another issue caused by the improvement: #41714