Fix #31155 - Remove assignability cases in getNarrowedType #32443

jack-williams · 2019-07-17T15:51:05Z

orta · 2019-07-17T17:12:35Z

tests/baselines/reference/controlFlowInstanceof.types

    }
    s;  // Set<number>
->s : Set<number>
+>s : Set<number> & Set<any>


Are these cases where TS would widen to be an any instead of the original number, this feels like something we don't want

Yes, when accessing the parametrising type you'll end up getting number & any which is going to swallow number - this is definitely a bad change. The problem is that before it was relying on the assignability check to strip out the any case, but now we don't have that.

I think I remember that there was a similar issue that came up in conditional type simplification, where T extends Foo<any> ? A : B was simplified to T & Foo<any>, causing similar problems. @weswigham what was the solution here?

jack-williams · 2019-07-22T14:38:32Z

This would probably benefit from #30161.

jack-williams · 2019-08-18T16:25:55Z

@typescript-bot run dt
@typescript-bot user test this
@typescript-bot test this

typescript-bot · 2019-08-18T16:27:33Z

Heya @jack-williams, I've started to run the parallelized Definitely Typed test suite on this PR at b6026c2. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-08-18T16:27:33Z

Heya @jack-williams, I've started to run the extended test suite on this PR at b6026c2. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-08-18T16:27:33Z

Heya @jack-williams, I've started to run the parallelized community code test suite on this PR at b6026c2. You can monitor the build here. It should now contribute to this PR's status checks.

jack-williams · 2019-08-18T17:09:13Z

I've added an additional subtype check to tame the issues of Interface<any> creeping in because these make the change unacceptable. The current baseline changes seem more reasonable.

weswigham

I think this is good - briefly cc @ahejlsberg as this is the kind of change to run by you before we merge, if only so everyone's on the same page so we don't reintroduce a similar bug with a different narrowing in the future~

orta · 2019-10-23T18:51:47Z

Ping @ahejlsberg

ahejlsberg · 2019-10-24T22:11:52Z

The old test runs have expired. Running again so I can take a look.

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

typescript-bot · 2019-10-24T22:11:53Z

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at d91d445. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-10-24T22:11:53Z

Heya @ahejlsberg, I've started to run the extended test suite on this PR at d91d445. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-10-24T22:11:53Z

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at d91d445. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2019-10-24T22:25:48Z

You're gunna need to merge this with master to get good, comparable test runs ❤️

typescript-bot · 2019-10-24T22:43:20Z

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

jack-williams · 2019-10-25T06:02:45Z

@ahejlsberg, @weswigham: I’ll merge and run those tests later today

jack-williams · 2019-10-25T08:43:11Z

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

typescript-bot · 2019-10-25T08:43:16Z

Heya @jack-williams, I've started to run the extended test suite on this PR at b24dbe6. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-10-25T08:43:16Z

Heya @jack-williams, I've started to run the parallelized Definitely Typed test suite on this PR at b24dbe6. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-10-25T08:43:16Z

Heya @jack-williams, I've started to run the parallelized community code test suite on this PR at b24dbe6. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-10-25T09:09:58Z

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

jack-williams · 2019-10-25T14:39:10Z

DTS failures look to be import/declaration related. Are these noise?

Not entirely sure how to interpret user test failures.

ahejlsberg · 2019-10-25T18:33:10Z

The DT failures are noise. Not sure how to look at the user test failures either.

weswigham · 2019-10-25T20:07:49Z

Not sure how to look at the user test failures either.

There're a little fail-y on master right now, so you'd need to diff the PR against the master PR. #34727

jack-williams · 2019-10-25T21:34:47Z

I think the diffs look clean. Only thing that looks significant is a load of errors in src/language-yaml/printer-yaml.js now removed in master - might be unrelated?

jack-williams · 2019-11-05T10:43:44Z

@typescript-bot user test this

typescript-bot · 2019-11-05T10:43:50Z

Heya @jack-williams, I've started to run the parallelized community code test suite on this PR at b24dbe6. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-11-05T11:13:36Z

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

jack-williams · 2019-11-05T11:20:34Z

I think the vscode change looks related to this:

[XX:XX:XX] Error: /vscode/src/vs/base/browser/ui/actionbar/actionbar.ts(625,9): Type '(IAction & any[]) | (readonly 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.
[XX:XX:XX] Error: /vscode/src/vs/base/browser/ui/actionbar/actionbar.ts(625,9): Type '(IAction & any[]) | (readonly IAction[] & any[]) | (IAction | readonly IAction[])[]' is not assignable to type 'readonly IAction[]'.
[XX:XX:XX] Error: /vscode/src/vs/base/browser/ui/actionbar/actionbar.ts(625,9): Type '(IAction & any[]) | (readonly 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.
[XX:XX:XX] Error: /vscode/src/vs/base/browser/ui/actionbar/actionbar.ts(625,9): Type '(IAction & any[]) | (readonly IAction[] & any[]) | (IAction | readonly IAction[])[]' is not assignable to type 'readonly IAction[]'

jack-williams · 2019-11-05T11:37:16Z

Minimal repro:

interface IAction {
    x: string;
}

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

Previously x was any[], now it's (IAction & any[]) | (ReadonlyArray<IAction> & any[]) | ( IAction | ReadonlyArray<IAction>)[]

TLDR: any[] is assignable to IAction | ReadonlyArray<IAction>, 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.

I feel slightly reluctant to make any changes to the solution here for a type-guard that has multiple issues open about improving it; though, I don't think this break is acceptable to push into master. Need to come up with some sort of resolution here, which may be trying to resolve isArray first.

jack-williams · 2020-01-08T16:15:07Z

I think this is potentially blocked on #17002.

sandersn · 2020-03-11T15:56:30Z

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here.

Remove assignability cases in getNarrowedType

343f3df

orta reviewed Jul 17, 2019

View reviewed changes

jack-williams added 2 commits July 17, 2019 10:19

Merge remote-tracking branch 'upstream/master' into instanceof-narrowing

d1d895f

Fix linting

798405c

RyanCavanaugh assigned weswigham Aug 16, 2019

jack-williams added 2 commits August 18, 2019 15:10

Merge branch 'master' into instanceof-narrowing

2b8ce2d

Add subtype case

b6026c2

orta approved these changes Aug 19, 2019

View reviewed changes

weswigham approved these changes Aug 19, 2019

View reviewed changes

typescript-bot mentioned this pull request Oct 24, 2019

🤖 User test baselines have changed for instanceof-narrowing jack-williams/TypeScript#8

Closed

Merge remote-tracking branch 'origin' into instanceof-narrowing

b24dbe6

jack-williams force-pushed the instanceof-narrowing branch from d91d445 to b24dbe6 Compare October 25, 2019 08:20

typescript-bot mentioned this pull request Oct 25, 2019

🤖 User test baselines have changed for instanceof-narrowing jack-williams/TypeScript#9

Closed

typescript-bot mentioned this pull request Nov 5, 2019

🤖 User test baselines have changed for instanceof-narrowing jack-williams/TypeScript#10

Open

sandersn added this to Gallery in Pall Mall Jan 30, 2020

sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020

sandersn removed this from Gallery in Pall Mall Mar 2, 2020

sandersn added this to Waiting on author in PR Backlog Mar 2, 2020

sandersn closed this Mar 11, 2020

PR Backlog automation moved this from Waiting on author to Done Mar 11, 2020

orta mentioned this pull request Jun 25, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #31155 - Remove assignability cases in getNarrowedType #32443

Fix #31155 - Remove assignability cases in getNarrowedType #32443

jack-williams commented Jul 17, 2019 •

edited

Loading

orta Jul 17, 2019

jack-williams Jul 18, 2019

jack-williams commented Jul 22, 2019

jack-williams commented Aug 18, 2019

typescript-bot commented Aug 18, 2019 •

edited

Loading

typescript-bot commented Aug 18, 2019 •

edited

Loading

typescript-bot commented Aug 18, 2019 •

edited

Loading

jack-williams commented Aug 18, 2019

weswigham left a comment •

edited

Loading

orta commented Oct 23, 2019

ahejlsberg commented Oct 24, 2019

typescript-bot commented Oct 24, 2019 •

edited

Loading

typescript-bot commented Oct 24, 2019 •

edited

Loading

typescript-bot commented Oct 24, 2019 •

edited

Loading

weswigham commented Oct 24, 2019

typescript-bot commented Oct 24, 2019

jack-williams commented Oct 25, 2019

jack-williams commented Oct 25, 2019

typescript-bot commented Oct 25, 2019 •

edited

Loading

typescript-bot commented Oct 25, 2019 •

edited

Loading

typescript-bot commented Oct 25, 2019 •

edited

Loading

typescript-bot commented Oct 25, 2019

jack-williams commented Oct 25, 2019

ahejlsberg commented Oct 25, 2019

weswigham commented Oct 25, 2019

jack-williams commented Oct 25, 2019

jack-williams commented Nov 5, 2019

typescript-bot commented Nov 5, 2019 •

edited

Loading

typescript-bot commented Nov 5, 2019

jack-williams commented Nov 5, 2019

jack-williams commented Nov 5, 2019 •

edited

Loading

jack-williams commented Jan 8, 2020

sandersn commented Mar 11, 2020

Fix #31155 - Remove assignability cases in getNarrowedType #32443

Fix #31155 - Remove assignability cases in getNarrowedType #32443

Conversation

jack-williams commented Jul 17, 2019 • edited Loading

orta Jul 17, 2019

Choose a reason for hiding this comment

jack-williams Jul 18, 2019

Choose a reason for hiding this comment

jack-williams commented Jul 22, 2019

jack-williams commented Aug 18, 2019

typescript-bot commented Aug 18, 2019 • edited Loading

typescript-bot commented Aug 18, 2019 • edited Loading

typescript-bot commented Aug 18, 2019 • edited Loading

jack-williams commented Aug 18, 2019

weswigham left a comment • edited Loading

Choose a reason for hiding this comment

orta commented Oct 23, 2019

ahejlsberg commented Oct 24, 2019

typescript-bot commented Oct 24, 2019 • edited Loading

typescript-bot commented Oct 24, 2019 • edited Loading

typescript-bot commented Oct 24, 2019 • edited Loading

weswigham commented Oct 24, 2019

typescript-bot commented Oct 24, 2019

jack-williams commented Oct 25, 2019

jack-williams commented Oct 25, 2019

typescript-bot commented Oct 25, 2019 • edited Loading

typescript-bot commented Oct 25, 2019 • edited Loading

typescript-bot commented Oct 25, 2019 • edited Loading

typescript-bot commented Oct 25, 2019

jack-williams commented Oct 25, 2019

ahejlsberg commented Oct 25, 2019

weswigham commented Oct 25, 2019

jack-williams commented Oct 25, 2019

jack-williams commented Nov 5, 2019

typescript-bot commented Nov 5, 2019 • edited Loading

typescript-bot commented Nov 5, 2019

jack-williams commented Nov 5, 2019

jack-williams commented Nov 5, 2019 • edited Loading

jack-williams commented Jan 8, 2020

sandersn commented Mar 11, 2020

jack-williams commented Jul 17, 2019 •

edited

Loading

typescript-bot commented Aug 18, 2019 •

edited

Loading

typescript-bot commented Aug 18, 2019 •

edited

Loading

typescript-bot commented Aug 18, 2019 •

edited

Loading

weswigham left a comment •

edited

Loading

typescript-bot commented Oct 24, 2019 •

edited

Loading

typescript-bot commented Oct 24, 2019 •

edited

Loading

typescript-bot commented Oct 24, 2019 •

edited

Loading

typescript-bot commented Oct 25, 2019 •

edited

Loading

typescript-bot commented Oct 25, 2019 •

edited

Loading

typescript-bot commented Oct 25, 2019 •

edited

Loading

typescript-bot commented Nov 5, 2019 •

edited

Loading

jack-williams commented Nov 5, 2019 •

edited

Loading