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

Union types are not being narrowed correctly in generic functions in 4.3 #44404

Closed
TomPeters opened this issue Jun 3, 2021 · 5 comments · Fixed by #41821
Closed

Union types are not being narrowed correctly in generic functions in 4.3 #44404

TomPeters opened this issue Jun 3, 2021 · 5 comments · Fixed by #41821
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@TomPeters
Copy link

TomPeters commented Jun 3, 2021

Bug Report

Narrowing of a union type based on generic types does not work correctly in else branches in TS 4.3.

This likely relates to the changes made in #43183

🔎 Search Terms

generic, narrow, union types, 4.3

🕗 Version & Regression Information

This changed between versions 4.2.3 and 4.3

⏯ Playground Link

Playground link

💻 Code

function needsToNarrowTheType<First extends { foo: string }, Second extends { bar: string }>(thing: First | Second) {
    if (hasAFoo(thing)) {
        console.log(thing.foo);
    }
    else {
        // I would expect this to work because the type should be narrowed in this branch to `Second`
        console.log(thing.bar); // Error: Property 'bar' does not exist on type 'First | Second'.
    }

    function hasAFoo(value: First | Second): value is First {
        return "foo" in value;
    }
}

🙁 Actual behavior

In the else branch, the type of thing is First | Second.

🙂 Expected behavior

In the else branch. the type of thing is Second.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jun 10, 2021
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.4.0 (Beta) milestone Jun 10, 2021
@fabb
Copy link

fabb commented Jun 16, 2021

looks similar to my issue:

type Value = string | number | boolean | unknown[] | null
type ValueObject<V extends Value> = Record<string, V | undefined>
type ResponsiveValue<V extends Value> = V | ValueObject<V>

const isValue = <V extends Value>(object: ResponsiveValue<V>): object is V => !isObject(object)
const isValueObject = <V extends Value>(object: ResponsiveValue<V>): object is ValueObject<V> => isObject(object)
const isObject = (value: unknown): value is Record<string, unknown> => typeof value === 'object' && !Array.isArray(value)

const responsiveSome = <V extends Value>(value: ResponsiveValue<V>, callbackfn: (value: V) => boolean) => {
    if (isValue(value)) {
        return callbackfn(value)
    } else {
        // BUG in 4.3: value should have type ValueObject<V>, but has ValueObject<V> | Value
        return Object.values(value).some(v => (typeof v === 'undefined' ? false : callbackfn(v)))
    }
}
Output
"use strict";
const isValue = (object) => !isObject(object);
const isValueObject = (object) => isObject(object);
const isObject = (value) => typeof value === 'object' && !Array.isArray(value);
const responsiveSome = (value, callbackfn) => {
    if (isValue(value)) {
        return callbackfn(value);
    }
    else {
        // BUG in 4.3: value should have type ValueObject<V>, but has ValueObject<V> | Value
        return Object.values(value).some(v => (typeof v === 'undefined' ? false : callbackfn(v)));
    }
};
Compiler Options
{
  "compilerOptions": {
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "alwaysStrict": true,
    "esModuleInterop": true,
    "declaration": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "target": "ES2017",
    "jsx": "react",
    "module": "ESNext",
    "moduleResolution": "node"
  }
}

Playground Link: Provided

@thw0rted
Copy link

I found a regression that I think might be the same one. (If not I can certainly open a new ticket?)

Playground example

abstract class TwoGeneric<T, U> {
    public abstract readonly t: T;

    protected abstract _u: U;
    public get u(): U { return this._u; }
    public set u(val: U) { this._u = val; }
}

class Child1 extends TwoGeneric<number, boolean> {
    public readonly t = 3;
    protected _u = false;
}

class Child2 extends TwoGeneric<string, boolean> {
    public readonly t = "Hi";
    protected _u = false;
}


declare const x: TwoGeneric<any, any>;

if (x instanceof Child1) {
    x.u = true;
}
if (x instanceof Child2) {
    x.u = true;
}

if (x instanceof Child1 || x instanceof Child2) {
    x.u = true;  // Type 'boolean' is not assignable to type 'U'.  'U' could be instantiated with an arbitrary type which could be unrelated to 'boolean'.(2322)
}

Note that this works without errors in 4.2.x but fails in 4.3.x due to the "improved" narrowing.

The two standalone type-narrowing checks work, but the narrowed union fails. Inside the || conditional, Intellisense hints that x is Child1 | Child2. The type of x.u is the same for both, but it still fails with the error message in the comment.

weswigham added a commit to weswigham/TypeScript that referenced this issue Jul 27, 2021
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Jul 27, 2021
@andrewbranch andrewbranch added the Rescheduled This issue was previously scheduled to an earlier milestone label Aug 30, 2021
andrewbranch added a commit that referenced this issue Sep 30, 2021
…rease on change in value (#41821)

* Track source and target relationship stack depth seperately, only increase on change in value

* Add baselines for test from #43485

* Bail on unwrapping conditional constraints on the source side when the source conditional is already known to be spooling out of control

* More usage of isDeeplyNestedType to block _specifically_ conditional recursion on only one side

* Negative cases of getNarrowedType that match the exact type should be filtered out, even when generic

* Add test and fix for #44404

* Swap to manually specifying left and right recursion

* Rename Left -> Source, Right -> Target

Co-authored-by: Andrew Branch <andrew@wheream.io>
@fabb
Copy link

fabb commented Jan 15, 2022

looks similar to my issue:

type Value = string | number | boolean | unknown[] | null
type ValueObject<V extends Value> = Record<string, V | undefined>
type ResponsiveValue<V extends Value> = V | ValueObject<V>

const isValue = <V extends Value>(object: ResponsiveValue<V>): object is V => !isObject(object)
const isValueObject = <V extends Value>(object: ResponsiveValue<V>): object is ValueObject<V> => isObject(object)
const isObject = (value: unknown): value is Record<string, unknown> => typeof value === 'object' && !Array.isArray(value)

const responsiveSome = <V extends Value>(value: ResponsiveValue<V>, callbackfn: (value: V) => boolean) => {
    if (isValue(value)) {
        return callbackfn(value)
    } else {
        // BUG in 4.3: value should have type ValueObject<V>, but has ValueObject<V> | Value
        return Object.values(value).some(v => (typeof v === 'undefined' ? false : callbackfn(v)))
    }
}

Output
Compiler Options
Playground Link: Provided

@andrewbranch this bug still happens in 4.5.4. Should I create a separate issue for this?

@andrewbranch
Copy link
Member

@fabb the generics in your type guards aren’t useful as far as I can tell. Try this.

@fabb
Copy link

fabb commented Jan 18, 2022

True, thanks!
It’s still a regression, but maybe there really is no use case that cannot be transformed like 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. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
7 participants