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

instanceof does not type guard when result is checked against Boolean (true/false) #47891

Closed
clshortfuse opened this issue Feb 14, 2022 · 5 comments
Labels
Duplicate An existing issue was already created

Comments

@clshortfuse
Copy link

Bug Report

instanceof check only works with truthy and falsy values, not the boolean that is returned from the operation.

🔎 Search Terms

instanceof boolean type guard

🕗 Version & Regression Information

Issue appears from 3.3.3 to 4.6 (nightly).

This is the behavior in every version I tried, and I reviewed the FAQ for entries about instanceof

⏯ Playground Link

https://www.typescriptlang.org/play?ts=4.6.0-dev.20220116#code/GYVwdgxgLglg9mABBBA3ApgJygITnAG3QEMwAVdAZygAo4AuazGMAcwB8ARYqddsEAFsARlgCU9ASKyIA3gFgAUIhWIYwRDQCEdNWGqkI6OBu68xYuUtU3EUABaY4Ad0Rh0rgKKYnmGgHIAeTACAE9EM3REQOEAK3RoSn8xa1UAX1SVTHQoEEwkOAA6VhyyGEF0GjEAbiUMxSVQSFgERHQARxBiAjxCEnIqWgYmFg5I-iFRTAkpKatlVXVNOD0DSGNTHiiAXl3EYG7KdEsFBds7Rxc3D0RvXwDgsIit6LiEqCSUs8R6m2zc-KIIolKBlCpVWqKeqNcDQeBIMBwKCeTrdXpEUgUah0RhQZhsLhbCbSaaSSYyU42Ja6FhrIwmZ68RBaPZ4kDHebnFQOJyudxeHxwPxBELhSKveKJZKZH4y-55ArFUrlSo1OpKIA

💻 Code

function convertBooleanTest(o:string|Date|number):number {
    if (!(o instanceof Date)) {
        throw new Error('Only Date Objects')
    }
    return o.getTime();
}

function equalBooleanTest(o:string|Date|number):number {
    if (o instanceof Date === false) {
        throw new Error('Only Date Objects')
    }
    return o.getTime(); // <-- Error
}

function notEqualBooleanTest(o:string|Date|number):number {
    if (o instanceof Date !== true) {
        throw new Error('Only Date Objects')
    }
    return o.getTime(); // <-- Error
}

🙁 Actual behavior

It seems instanceof is internally considered truthy, not a Boolean. To test against false you have to convert the operation with ! instead of just false.

🙂 Expected behavior

object instanceof Type === false should be handled. Wrapping with if (!(object instanceOf Type)) lacks code clarity and is easily missed. It's also a needless converstion/cast just to satisfy Typescript.

instanceof already applies ! ToBoolean internally (https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html#sec-instanceofoperator) and will either return a Boolean or throw a TypeError.

@nmain
Copy link

nmain commented Feb 14, 2022

I believe this is a case of a nonstandard narrowing pattern: the compiler only applies narrowing to certain recognized syntactic patterns. Each pattern has a cost to recognize, so uncommonly used ones aren't supported. What's the value of doing o instanceof Date === false vs !(o instanceof Date)?

@clshortfuse
Copy link
Author

clshortfuse commented Feb 14, 2022

What's the value of doing o instanceof Date === false vs !(o instanceof Date)?

It lacks code clarity and is easily missed. You have to wrap the result of o instanceof Type in parentheses

For example:

if (!event.target || event.target instanceof Element === false) throw TypeError('Expected Element');
if (event.target instanceof HTMLElement === false)) handleElement();
if (event.target instanceof HTMLAnchorElement) handleAnchor();
if (event.target instanceof HTMLButton) handleButton();
if (event.target instanceof HTMLElement) handleHTMLElement();

vs

if (!event.target || !(event.target instanceof Element)) throw TypeError('Expected Element');
if (!(event.target instanceof HTMLElement)) handleElement();
if (event.target instanceof HTMLAnchorElement) handleAnchor();
if (event.target instanceof HTMLButton) handleButton();
if (event.target instanceof HTMLElement) handleHTMLElement();

Also, checking JS code that already uses this syntax doesn't work and has to be rewritten just for type-checking.

@clshortfuse
Copy link
Author

After a bit of search, I believe the change could be done here:

return isNarrowableOperand(expr.left);

I'm still trying to understand the code, but I think it's around here. I'll have to figure out how to debug Typescript to try it out.

@RyanCavanaugh
Copy link
Member

Duplicate #44366

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Feb 14, 2022
@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants