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

Typeguard with ""|undefined not working #31156

Closed
AnyhowStep opened this issue Apr 29, 2019 · 11 comments · Fixed by #49625
Closed

Typeguard with ""|undefined not working #31156

AnyhowStep opened this issue Apr 29, 2019 · 11 comments · Fixed by #49625
Labels
Bug A bug in TypeScript
Milestone

Comments

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Apr 29, 2019

TypeScript Version: 3.3.0, and the version on TS playground (3.4.1 as of this writing)

Search Terms: typeguard, empty string literal, undefined, union

Code

//--strictNullChecks
type Falsy<T> = (
    | Extract<0, T>
    | Extract<
        (
            | false
            | null
            | undefined
            //Actually, only 0|NaN
            //But TS does not support NaN literal
            | number
            | ""
            | 0n
            | HTMLAllCollection
        ),
        T
    >
);
declare const emptyStrOrUndefined: Falsy<string | undefined>;
declare function isFalsy<T>(
    mixed: T
): mixed is Falsy<T>;

declare const s: string | undefined;
/*
    Hovering over this isFalsy() call gives me,
    function isFalsy<string | undefined>(mixed: string | undefined): mixed is "" | undefined

    This is as expected
*/
if (isFalsy(s)) {
    //Expected: s should be ""|undefined now
    //Actual: s is undefined
    if (s == undefined) {
        //Should be undefined now
        console.log(s);
    } else {
        //Expected: Should be "" now
        //Actual: s is `never`
        //Property 'length' does not exist on type 'never'.
        console.log(s.length);
    }
}

declare function isEmptyStrOrUndefined(mixed: any): mixed is "" | undefined;
if (isEmptyStrOrUndefined(s)) {
    //Expected: s should be ""|undefined now
    //Actual: s is undefined
    if (s == undefined) {
        //Should be undefined now
        console.log(s);
    } else {
        //Expected: Should be "" now
        //Actual: s is `never`
        //Property 'length' does not exist on type 'never'.
        console.log(s.length);
    }
}

Expected behavior:

s should be ""|undefined where marked.

Also, unrelated, but this is where I would have liked a NaN literal in TS =/

Actual behavior:

s is undefined

Playground Link: Here

Related Issues: Not that I could find

@jack-williams
Copy link
Collaborator

This looks like a bug to me. The current behaviour when the source type is a union (here string | undefined) is that the source is filtered against the candidate type (here "" | undefined) removing all types not related to the candidate. The type string is not related to "" | undefined and is therefore filtered out, returning undefined (which is related to "" | undefined).

When the source type is not a union then the code intersects the source with the candidate (string & ("" | undefined)), producing the right narrowing.

I think getNarrowedType should probably map over unions first, applying narrowing to each constituent, abit like narrowTypeByTypeof.

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Apr 29, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.5.0 milestone Apr 29, 2019
@RyanCavanaugh
Copy link
Member

@andrewbranch fyi I didn't look fully into this, but generally assume @jack-williams is right 😉

@andrewbranch
Copy link
Member

@RyanCavanaugh given the discussion in #31206, what should we bump the milestone to?

@andrewbranch
Copy link
Member

Cross-posting from PR #31206 for visibility:

We discussed this in a design meeting and decided there’s not currently a solution that’s worth the disruption it will bring. A hypothetical future where we can stop treating “branded primitives” as they exist today (e.g. string & { __kind: 'path' }) as possible value-containing types will help somewhat, and we also discussed this as an indicator that people often think about their object types as being closed, when in fact the type system doesn’t represent this at all (aside from excess property checking on fresh object literals).

I may look into doing the correct thing when all constituents of both the original type and the predicate type are primitives, since intersections of those are more intuitive and fall out when empty. On the other hand, I’m not sure if we want that kind of logical branching—maybe it’s better just to keep it in its current slightly wrong but consistent state. At any rate, this PR is out of scope for that kind of change.

@andrewbranch andrewbranch removed their assignment Jun 24, 2019
@AnyhowStep
Copy link
Contributor Author

I need a sad face react =(

Thank you for the update, and the attempt!

@AnyhowStep
Copy link
Contributor Author

AnyhowStep commented Dec 31, 2019

This is a repro of a problem @G-Rath had, on TS 3.7.4,

//--strictNullChecks
declare const s: string | undefined;
declare function isEmptyStrOrUndefined(mixed: string|undefined): mixed is "" | undefined;
if (isEmptyStrOrUndefined(s)) {
    //Expected: s should be ""|undefined now
    //Actual: s is undefined
    if (s == undefined) {
        //Should be undefined now
        console.log(s);
    } else {
        //Expected: Should be "" now
        //Actual: s is `never`
        //Property 'length' does not exist on type 'never'.
        console.log(s.length);
    }
}


declare function isEmptyStr(mixed: string | undefined): mixed is "";
declare function isUndefined(mixed: string|undefined): mixed is undefined;
if (isEmptyStr(s) || isUndefined(s)) {
    //Expected: s should be ""|undefined now
    //Actual: s is ""|undefined; as expected
    if (s == undefined) {
        //Should be undefined now
        console.log(s);
    } else {
        //Expected: Should be "" now
        //Actual: s is ""; as expected
        console.log(s.length);
    }
}

Playground


It seems like type guard unions heck things up.
@G-Rath 's example is more complex and involves the ts-estree package, though.


Is this because it's checking subtypes only when the type guard is for a union type?
And it should be checking both subtypes and supertypes?


[EDIT]

It seems like his problem is different from mine. See his comment 👇 for more info.

The workaround of not using union type guards works, though, even if cumbersome.

Instead of,

myTypeGuard (x : X) : A|B;
if (myTypeGuard(x)) {
  //stuff
}

Use,

myTypeGuardA (x : X) : A;
myTypeGuardB (x : X) : B;
if (myTypeGuardA(x) || myTypeGuardB(x)) {
  //stuff
}

@G-Rath
Copy link

G-Rath commented Dec 31, 2019

This is my reproduction repo.

src/index.ts:48:7 - error TS2367: This condition will always return 'false' since the types 'AST_NODE_TYPES.Literal' and 'AST_NODE_TYPES.TemplateLiteral' have no overlap.

48 if (argument.type === AST_NODE_TYPES.TemplateLiteral) {
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I came across this when upgrading @typescript-eslint/typescript-estree from 2.12.0 to 2.13.0 as part of package update on eslint-plugin-jest.

The change that caused this issue is that StringLiteral defines value as type string, where as in the past it was type boolean | number | RegExp | string | null (which is what it is defined as on its super class LiteralBase, which hasn't changed).

If I comment out the value: string on StringLiteral, it all just works.


Here's a playground helpfully supplied by @AnyhowStep

@schani
Copy link

schani commented Nov 29, 2020

I ran into this in real-world code. This is an abbreviated reproduction of my case:

// This is necessary for the issue to occur
interface PrimitiveType {
    kind: "string"
}

interface ArrayColumnType {
    kind: "array";
    items: PrimitiveType | TableRefType;
}

// MultiRelationType is a specialized version of ArrayColumnType
interface MultiRelationType {
    kind: "array";
    items: TableRefType;
}

interface TableRefType {
    kind: "table-ref";
    tableName: string;
}

type ColumnType = PrimitiveType | ArrayColumnType | TableRefType;

export function isRelationType(t: ColumnType): t is (TableRefType | MultiRelationType) {
    return t.kind === "table-ref" || (t.kind === "array" && t.items.kind === "table-ref");
}

function makeColumnType(): ColumnType {
    return { kind: "array", items: { kind: "table-ref", tableName: "foo"}};
}

const foo = makeColumnType();
// Here foo can be one of PrimitiveType | ArrayColumnType | TableRefType.
if (isRelationType(foo)) {
    // Now we know foo is one of TableRefType | MultiRelationType.
    // But TS seems to think that foo is definitely a TableRefType:
    console.log(foo.tableName); // no error
}

@octogonz
Copy link

octogonz commented Dec 8, 2020

We encountered a vaguely similar problem in #41871. The commonalities seem to be:

  • using a type guard (in our case, the problem disappears if the type guard is inlined)
  • the type guard checks for a type union
  • the type guard receives another type union as its input, and the narrowed type is (incorrectly) too narrow

Our repro works all the way back to TypeScript 2.0.10, so this problem apparently has been around for a while.

@DetachHead
Copy link
Contributor

minimal example

declare function assert<T>(value: any): asserts value is T

declare const foo: number | string | boolean

assert<1 | string>(foo)

const bar = foo //string

@jtbandes
Copy link
Contributor

I also ran into this problem:

function isEmptyOrUndefined(str: string | undefined): str is undefined | "" {
    return str == undefined || str === "";
}

function test(s: string | undefined) {
    if (isEmptyOrUndefined(s)) {
        let len = s?.length;  // error
    }
}

I'm going to work around it for now by returning string | undefined rather than trying to use it as a type guard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
9 participants