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

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

Merged
merged 4 commits into from Sep 8, 2020

Conversation

orta
Copy link
Contributor

@orta orta commented Jun 25, 2020

Fixes #31155
Fixes #17002

There are two pieces to this PR:

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 :

interface IAction {
    x: string;
}

declare const arg: IAction | ReadonlyArray<IAction>;
const x = Array.isArray(arg) ? arg : [arg];

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

/cc @jack-williams

@RyanCavanaugh
Copy link
Member

@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 25, 2020

Heya @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
Copy link
Collaborator

typescript-bot commented Jun 25, 2020

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

@typescript-bot
Copy link
Collaborator

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

@RyanCavanaugh
Copy link
Member

@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 25, 2020

Heya @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
Copy link
Collaborator

typescript-bot commented Jun 25, 2020

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

Copy link
Member

@RyanCavanaugh RyanCavanaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

jack-williams commented Jun 26, 2020

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?

declare function isArray<T>(arg: T | {}): arg is T extends readonly (infer U)[] ? GroundArray<T,U>: any[];
type GroundArray<T extends readonly unknown[], U> = [T] extends [U[]] ? any[] : T;

@weswigham
Copy link
Member

isArray<T>(arg: T | {}): arg is T extends readonly any[] ? (unknown extends T ? never : readonly any[]) : any[];

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

@orta
Copy link
Contributor Author

orta commented Jul 8, 2020

@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 8, 2020

Heya @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
Copy link
Collaborator

typescript-bot commented Jul 8, 2020

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

@orta
Copy link
Contributor Author

orta commented Jul 13, 2020

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:

export const isArray = (() => Array.isArray || (<T>(x: any): x is T[] => x && typeof x.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:

export function isArray(array: any): array is any[] {
	return Array.isArray(array);
}

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

@weswigham
Copy link
Member

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

@orta
Copy link
Contributor Author

orta commented Jul 13, 2020

Looks like I need a build to verify rxjs

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 13, 2020

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 13, 2020

Hey @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
Copy link
Contributor Author

orta commented Jul 30, 2020

@typescript-bot pack this

@orta
Copy link
Contributor Author

orta commented Sep 3, 2020

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

@orta
Copy link
Contributor Author

orta commented Sep 8, 2020

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

@orta orta merged commit fa89ce6 into microsoft:master Sep 8, 2020
PR Backlog automation moved this from Needs merge to Done Sep 8, 2020
sandersn added a commit to sandersn/vue-next that referenced this pull request Sep 23, 2020
Typescript 4.1 will have [stricter rules for type predicates](microsoft/TypeScript#39258) like

```ts
export const isObject = (val: unknown): val is Record<any, any> =>
  val !== null && typeof val === 'object'
```

As a result of these rules, an expression in collectionHandlers.ts in
the reactivity package doesn't get narrowed to the type it did in TS 4.0
and below. Instead it gets an intersection type, which doesn't work with
subsequent code.

I restored the old type using a cast, since that's essentially what the
old version of Typescript was doing here.
@RyanCavanaugh RyanCavanaugh added the Breaking Change Would introduce errors in existing code label Oct 19, 2020
@RyanCavanaugh
Copy link
Member

Example for blog post

declare const p: string | ReadonlyArray<string>;
if (Array.isArray(p)) {
  // 4.0: p: any[]
  // 4.1: p: readonly string[];

  // 4.0: OK
  // 4.1: Error
  p.push(0);
}

@laughinghan
Copy link

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.:

declare var foo: string[] | readonly number[] | null

if (foo instanceof Array) {
    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)

@felixfbecker
Copy link

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

@victorgarciaesgi
Copy link

@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

@MartinJohns
Copy link
Contributor

MartinJohns commented Nov 20, 2020

@orta / @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

@yseymour
Copy link

This is broken for generics and other nontrivial types:

declare function f1<T extends any[]>(array: T): void;
function f2<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.

@MartinJohns
Copy link
Contributor

Another issue caused by the improvement: #41714

orta added a commit to orta/TypeScript that referenced this pull request Dec 7, 2020
orta added a commit to orta/TypeScript that referenced this pull request Dec 7, 2020
orta pushed a commit that referenced this pull request Dec 7, 2020
orta pushed a commit that referenced this pull request Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team Breaking Change Would introduce errors in existing code For Milestone Bug PRs that fix a bug with a specific milestone
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

'instanceof' changes type outside of 'if' statement Array.isArray type narrows to any[] for ReadonlyArray<T>