Add type guard for `in` keyword #15256

IdeaHunter · 2017-04-18T21:43:11Z

Added support for typeguarding for following cases:

if ("a" in (x))
if ("a" in x)
if ("a" in x.prop)
if ("a" in x.outer.prop) 
if ('a' in this)
if ('a' in this.prop)

RyanCavanaugh · 2017-04-18T22:35:20Z

src/compiler/checker.ts

+            function narrowByInKeyword(type: Type, literal: LiteralExpression, assumeTrue: boolean) {
+                if ((type.flags & (TypeFlags.Union | TypeFlags.Object)) || (type.flags & TypeFlags.TypeParameter && (type as TypeParameter).isThisType)) {
+                    const propName = literal.text;
+                    return  filterType(type, t => !!getPropertyOfType(t, propName) === assumeTrue);


Two spaces here... we should turn on a lint rule

@RyanCavanaugh fixed

RyanCavanaugh · 2017-04-18T22:45:22Z

tests/baselines/reference/inKeywordTypeguard.errors.txt

+        if ("a" in x) {
+            x.a = "1";
+        } else {
+            x.b = "1";


It isn't correct to narrow to BOpn in this branch

@RyanCavanaugh it isn't clear what should we do here since "a" in {a:undefined} === true and how we should look on it from strictNullChecks perspective...
Can you please give an advice how we should narrow this guy?

Right now we have following logic: propertyIsInType === assumeTrue. If i got you right then we should change this logic to

propertyIsOptional ? assumeTrue && propertyIsInType : propertyIsInType === assumeTrue

right?

That seems like it might do the trick

Im changing behavior to requested one and now line 57 it gives us an error that seems make sense

tests/cases/compiler/inKeywordTypeguard.ts error TS2339: Property 'b' does not exist on type 'AWithOptionalProp | BWithOptionalProp'. Property 'b' does not exist on type 'AWithOptionalProp'

IdeaHunter · 2017-04-20T23:20:54Z

@RyanCavanaugh I have updated code to match requested behavior, can you, please, review it?
I would love to hear a feedback from you since i didn't even know do I moving in the right direction or not...

RyanCavanaugh · 2017-04-24T18:07:23Z

Looks good overall to me. @ahejlsberg care to review?

IdeaHunter · 2017-05-01T16:42:51Z

ping @RyanCavanaugh @ahejlsberg

EliSnow · 2017-05-16T17:03:05Z

I'm really looking forward to this landing!

EliSnow · 2017-06-07T14:53:20Z

Is it possible this will land for the Typescript 2.4 release?

hax · 2017-09-12T16:18:18Z

Can I ask what block this PR being merged?

pelotom · 2017-11-05T00:03:33Z

Can anyone comment as to why this PR is still languishing 6 months later?

IdeaHunter · 2017-11-05T20:23:04Z

There is two reasons of it:

They don't care about it/don't like/don't have time for it/didn't yet got bitten in most precious place by a relevant bug 😆/have more important stuffs to do (reason doesn't really matter in this but the lack of motivation to spend some time on it is the only important thing, otherwise this PR would already be merged)
Me not pushing it forward, because of using to babel+flow and didn't use typescript anywhere else(lack of motivation from my side to use typescript)

RyanCavanaugh · 2017-12-04T21:58:21Z

src/compiler/checker.ts

@@ -11336,6 +11336,34 @@ namespace ts {
                return type;
            }

+            function isTypePresencePossible(type: Type, propName: string, shouldHaveProperty: boolean) {
+                const prop = getPropertyOfType(type, propName);
+                if (!prop) {


Two edits in this body - rename shouldHaveProperty to assumeTrue and reduce the if to

if (prop) { return (prop.flags & SymbolFlags.Optional) ? true : assumeTrue; } return !assumeTrue;

RyanCavanaugh · 2017-12-04T21:58:47Z

Good to merge after one edit. Sorry for the delay on this 🙁

Jessidhia · 2017-12-05T01:32:46Z

tests/baselines/reference/inKeywordTypeguard.errors.txt

+        if ("a" in x) {
+            x.b = "1";
+              ~
+!!! error TS2339: Property 'b' does not exist on type 'A'.


I'd really like to be able to use in for narrowing, but I'm not sure this is it...

Consider:

const foo = { a: true, b: '' } negativeClassesTest(foo)

foo is only valid as a B, but it would pass the in check, and then be treated as an A; but its a key is not of type string.

This means that, within the branch, the type of x should still be A|B, but you are allowed to access .a; I just don't know what type .a should have. It probably should be any, which should trip --noImplicitAny if it's not used with a type narrowing expression.

Scenarios:

function negativeClassesTest(x: A | B) { if ("a" in x) { x.a // --noImplicitAny error const y: string = x.a // ideally a --noImplicitAny error, if it's possible to have "signalling any"s const z = x.a as string // accepted because it's a cast const u: string = typeof x.a === 'string' ? x.a : '' // accepted because it's narrowed } }

It doesn't seem to be all that useful to work like this, but it at least allows you to access .a at all.

Bonus points, but probably hard to actually do in practice:

function negativeClassesTest(x: A | B) { if ("a" in x && typeof x.a === "string") { // x _should_ be an A! } }

Alternative solution: Have a way to indicate that B has "not this key". Maybe, for example, interface B { a: never; b: string } should mean that B is not allowed to have an a key at all; even if it's set to a value of type never such as undefined!.

We considered the soundness aspect and it's not very different from existing soundness issues around aliased objects with undeclared properties. IOW you can already write this code, error-free (even in flow!), which observably fails:

interface T { x: string, y?: number }; const s = { x: "", y: "uh oh" }; const not_t: { x: string } = s; const unsound: T = not_t; (unsound.y && unsound.y.toFixed());

The reality is that most unions are already correctly disjointed and don't alias enough to manifest the problem. Someone writing an in test is not going to write a "better" check; all that really happens in practice is that people add type assertions or move the code to an equally-unsound user-defined type predicate. On net I don't think this is any worse than the status quo (and is better because it induces fewer user-defined type predicates, which are error-prone if written using in due to a lack of typo-checking).

For automatically disjointed unions, see #14094.

@Kovensky im agree, this is nasty case, but it grows from the fact we have structural type system in place. If we, for example, had nominal type system, the info we provided in function definitions would exactly matching all its usages and we never had this problem in place.

Workaround is to give compiler more complete info about types given, like this:

class A { a: string } class B { b: string } class C { a: number; b: string } function z(x: A|B|C){ if("a" in x){ var num_a: number = x.a; // error TS2322: Type 'string | number' is not assignable to type 'number'. // Type 'string' is not assignable to type 'number'. var str_a: string = x.a; // error TS2322: Type 'string | number' is not assignable to type 'string'. // Type 'number' is not assignable to type 'string'. } }

IdeaHunter · 2017-12-05T11:16:31Z

Going to to do requested fixes later, after my work day ends :-)

msftclas added the cla-signed label Apr 18, 2017

RyanCavanaugh reviewed Apr 18, 2017

View reviewed changes

msftgits removed the cla-signed label Sep 26, 2017

microsoft deleted a comment from msftclas Sep 27, 2017

jovdb mentioned this pull request Nov 5, 2017

Narrow union type with 'in' operator #19754

Closed

RyanCavanaugh reviewed Dec 4, 2017

View reviewed changes

Jessidhia reviewed Dec 5, 2017

View reviewed changes

IdeaHunter added 3 commits December 6, 2017 00:02

Add initial support for 'in' typeguarding

9c3c2ad

Remove whitespace

a5d6be1

Change type narrowing for optional properties

069f73d

IdeaHunter force-pushed the in-typeguard branch from 8e79460 to 069f73d Compare December 5, 2017 21:06

Change isTypePresencePossible, accept baseline

04b9b30

RyanCavanaugh merged commit c2fc5ea into microsoft:master Dec 6, 2017

sandersn mentioned this pull request Dec 7, 2017

in keyword doesn't narrow types with string-indexers correctly #20540

Closed

ghost mentioned this pull request Dec 11, 2017

property does not exists on type never when using "in" operator #20590

Closed

massimonewsuk mentioned this pull request Jan 25, 2018

in keyword doesn't seem to narrow types facebook/flow#5717

Open

mhegazy mentioned this pull request Jan 31, 2018

Invalid TS2339 Property '<prop>' does not exist on type 'never' in if/else branch #21517

Closed

jcalz mentioned this pull request Feb 7, 2018

Suggestion: treat in operator as type guard which asserts property existence #21732

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type guard for `in` keyword #15256

Add type guard for `in` keyword #15256

IdeaHunter commented Apr 18, 2017

RyanCavanaugh Apr 18, 2017

IdeaHunter Apr 18, 2017

RyanCavanaugh Apr 18, 2017

IdeaHunter Apr 18, 2017

IdeaHunter Apr 18, 2017

RyanCavanaugh Apr 19, 2017

IdeaHunter Apr 20, 2017 •

edited

Loading

IdeaHunter commented Apr 20, 2017

RyanCavanaugh commented Apr 24, 2017

IdeaHunter commented May 1, 2017

EliSnow commented May 16, 2017

EliSnow commented Jun 7, 2017

hax commented Sep 12, 2017

pelotom commented Nov 5, 2017

IdeaHunter commented Nov 5, 2017

RyanCavanaugh Dec 4, 2017

RyanCavanaugh commented Dec 4, 2017

Jessidhia Dec 5, 2017 •

edited

Loading

RyanCavanaugh Dec 5, 2017 •

edited

Loading

IdeaHunter Dec 6, 2017

IdeaHunter commented Dec 5, 2017

Add type guard for in keyword #15256

Add type guard for in keyword #15256

Conversation

IdeaHunter commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IdeaHunter Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

IdeaHunter commented Apr 20, 2017

RyanCavanaugh commented Apr 24, 2017

IdeaHunter commented May 1, 2017

EliSnow commented May 16, 2017

EliSnow commented Jun 7, 2017

hax commented Sep 12, 2017

pelotom commented Nov 5, 2017

IdeaHunter commented Nov 5, 2017

Choose a reason for hiding this comment

RyanCavanaugh commented Dec 4, 2017

Jessidhia Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

RyanCavanaugh Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IdeaHunter commented Dec 5, 2017

Add type guard for `in` keyword #15256

Add type guard for `in` keyword #15256

IdeaHunter Apr 20, 2017 •

edited

Loading

Jessidhia Dec 5, 2017 •

edited

Loading

RyanCavanaugh Dec 5, 2017 •

edited

Loading