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

TypeGuarding not applied with explicit instanceof comparison to true/false #32819

Open
mjbvz opened this issue Aug 12, 2019 · 7 comments

Comments

@mjbvz
Copy link
Contributor

commented Aug 12, 2019

From microsoft/vscode#77921

TypeScript Version: 3.6.0-dev.20190810

Search Terms:

  • type guard
  • instanceof

Code

function foo(target: Number | String) {
    if (target instanceof String === true) {
        target.toLowerCase();
    }
}

Expected behavior:
No compile error.

Type of target in the conditional statement should be String

Actual behavior:
Error: Property 'toLowerCase' does not exist on type 'String | Number'. Property 'toLowerCase' does not exist on type 'Number'.

The guarding works properly if you omit the explicit === true:

function foo(target: Number | String) {
    if (target instanceof String) {
        target.toLowerCase();
    }
}

Playground Link:

Related Issues:

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Narrowing forms have to be identified syntactically, so we pay a perf penalty for every possible form that's checked for. Since there's no real reason to write it this way, we don't consider === true forms

@oliversalzburg

This comment has been minimized.

Copy link

commented Aug 12, 2019

I feel like the code example in my original ticket was completely reasonable and the current behavior very counter-intuitive. I understand that changing the behavior is undesireable if it implies a performance penalty, but it would really help me out if someone could point out my mistake or what I should be doing differently to work around the issue.

Thanks in advance

@mjbvz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@oliversalzburg You can write:

if( !(target instanceof HTMLElement) ) {
	return;
}
@falsandtru

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Narrowing forms have to be identified syntactically, so we pay a perf penalty for every possible form that's checked for. Since there's no real reason to write it this way, we don't consider === true forms

if( !(target instanceof HTMLElement) ) {
return;
}

@ahejlsberg said the similar thing when he implemented this type guard.

@oliversalzburg

This comment has been minimized.

Copy link

commented Aug 13, 2019

What about cases like this one?

function foo(target: Number | String) {
    const targetIsString = target instanceof String;
    if (targetIsString) {
        target.toLowerCase();
    }
}

We are currently writing TS definitions for parts of our code base and it's not uncommon that a parameter to a JS function can be of several types, especially if some arguments are optional. This causes us to write code that checks the type of the argument and branches accordingly.

This often leads to TS reporting errors as discussed in this issue, even though the code is perfectly valid. This is confusing developers and forces them to write code to adapt to limitations of TS, which is unfortunate.

Regardless, thanks for the explanations. The cleanest solution appears to be to write multiple functions that are stricter in their expected argument types.

@fatcerberus

This comment has been minimized.

Copy link

commented Aug 14, 2019

@oliversalzburg That's a different thing - TS doesn't track dependencies between variables like that, it can only narrow variables directly included in the conditional expression. There are many, many tickets about that exact pattern; but it's actually a very big can of worms to make it work in general ("What does the (narrowed) type of this variable imply for all the other ones in scope?"), so it's largely considered a design limitation that that doesn't work.

@OliverJAsh

This comment has been minimized.

Copy link

commented Aug 18, 2019

@RyanCavanaugh

Since there's no real reason to write it this way,

I think have a good reason for writing x === false instead of !x: I find the logical NOT operator hurts readability—I've lost count of how many times I've missed a ! when skimming through code. For this reason I would like to see narrowing work equally when using both forms.

Related: #32956

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.