Skip to content

Improvement to getNarrowedType changes lodash's isArray #52827

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

Closed
sandersn opened this issue Feb 17, 2023 · 1 comment · Fixed by #52984
Closed

Improvement to getNarrowedType changes lodash's isArray #52827

sandersn opened this issue Feb 17, 2023 · 1 comment · Fixed by #52984
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.

Comments

@sandersn
Copy link
Member

declare function isArrayLike(value: any): value is { length: number };
declare const value: { [index: number]: boolean, length: number } | undefined;

if (isArrayLike(value)) {
    const result: { length: number } = value;

} else {
    const result: undefined = value;
}

Before #52282: value: undefined in the false branch.
After #52282: value: { [index: number]: boolean, length: number } | undefined in the false branch.

This is more consistent, although the full repro at DefinitelyTyped/DefinitelyTyped#64406 seems less consistent than the 4.9 semantics.

@sandersn sandersn added the Needs Investigation This issue needs a team member to investigate its status. label Feb 17, 2023
@sandersn sandersn added this to the TypeScript 5.0.1 milestone Feb 17, 2023
@ahejlsberg
Copy link
Member

There are a lot of subtleties at play here. Some facts:

  1. Following Improvements to strictSubtypeRelation and getNarrowedType #52282, when the original and asserted types are non-identical mutual subtypes (in the regular subtype relationship), we favor the asserted type in the true branch, but preserve the original type in the false branch.
  2. The types { length: number } and { [index: number]: boolean, length: number } are mutual subtypes in the regular subtype relationship.
  3. Type { length: number } is the supertype of { [index: number]: boolean, length: number } in the strict subtype relationship.
  4. When control flows join we perform subtype reduction on the union of the branch types according to the strict subtype relationship.
  5. When the type in one of the branches of joining control flows is declared type of the variable, we forego subtype reduction and just continue with the declared type.

In the following example, value ends up with its original type after the if statement because of (5):

declare function isArrayLike(value: any): value is { length: number };
declare const value: { [index: number]: boolean, length: number } | undefined;

if (isArrayLike(value)) {
    value;  // { length: number }
}
else {
    value;  // { [index: number]: boolean, length: number } | undefined
}
value;  // { [index: number]: boolean, length: number } | undefined

However, below the exact declared type doesn't occur in either of the branches, so we subtype reduce according to (4):

declare function isArrayLike(value: any): value is { length: number };
declare const value: { [index: number]: boolean, length: number } | string;

if (isArrayLike(value)) {
    value;  // { length: number } | string
}
else {
    value;  // { [index: number]: boolean, length: number }
}
value;  // { length: number } | string

So, we generally do a better job of getting back to the original type in control flow joins, but it still isn't perfect. The real culprit here is our desire to go with the asserted type in the true branch when we have mutual subtypes. A more consistent strategy would be to only go with the asserted type when it is a pure subtype of the original type (because it'll then always be removed by subtype reduction). Basically the state we were in before #50044. But that brings about other issues...

I think we're okay for 5.0 but we should continue to think about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants