Flow: Flow doesn't check object properties when used as part of a if statement condition

[almost]

type X = {
    foo: string;
};

var x:X = {foo: "bar"};

x.baz === "barry"; // FLOW ERROR HERE AS EXPECTED

if (x.baz === "barry") { // WHY NO FLOW ERROR HERE?
}

http://tryflow.org/#ad2e29575cd9b299e42f388d48a1d20b

It seem like it doesn't check the properties of an object are valid if you're accessing them from within an if statement condition. I do see that it's useful for doing duck typing stuff, in regularly javascript that's a valid question to ask, but I don't think Flow should let it through. It makes it easy to typo property names and just never be told about it.

[samwgoldman]

Thanks for the report! Looking at the code here, I'm pretty confident this is because that if statement looks like a sentinel check for tagged union discrimination (introduced in f0196287f7842164108c8dacbb1b5682a176a2e6). Indeed, if I go back before that commit, I see an error where you expect one.

[samwgoldman]

Actually, I did a bit of investigating, and this is the intended behavior, as introduced in 48a5482d839699bb0e43acb36972297f20a7a05c.

[almost]

This seems unfortunate, it can easily mask bugs of the exact type you'd hope and expect Flow to catch (eg typos, misspellings and misunderstandings about what type a variable is). This would seem like a place where it would be a better to require the user to cast to any first or something else like that. Alternatively it might make sense to allow an == null check but not other forms.

[samwgoldman]

I see your point. @avikchaudhuri, @mroch, what do you think about losing the ability to catch typos in conditional statements like this?

[samwgoldman]

So, I recently ran into the same issue. I had a test in a conditional that I expected to do one thing, but the property did not exist on the object I was testing, so the code behaved differently.

I was able to work around this by adding a type assertion around my code, e.g., if ((foo.bar : string) === "something") {}. The type assertion circumvents the code that skips the property existence check.

[rileylark]

So I like to use Flow like this:

/* @flow */

type ExpectedType = {
    topLevelProp: {
        val: number
    }
};

function doProcess(x: ExpectedType) {
    console.log("I'm so confident prop exists that I'm just gonna print it " + x.topLevelProp.val);
}

I don't like checking for null and other type problems in my code, so I'd like Flow to at least try to ensure that the above will work. When I refactor my code, I'd like Flow to let me know if there are any problems.

This behavior undercuts my trust that Flow can do that, and if I can't trust that Flow can always do that I basically have to _never_ trust that Flow can do that.

Consider:

import val from 'wherever';

if (val.oldPropertyIHaveSinceRefactoredAway) {
    doProcess(val.oldPropertyIHaveSinceRefactoredAway);
}

This code checks, and that gives me a lot less trust in my types. Here I've managed to pass a who-knows-what into my function. Flow doesn't mind at all - it doesn't check the types in this if condition, and once we're past the if condition, I guess Flow has refined the type of this old property to any or something, and doesn't have any idea what it's passing into doProcess.

I kind of get that it's unlikely I will still ever have oldPropertyIHaveSinceRefactoredAway defined, so this is probably just dead code. I won't end up calling doProcess in practice. But do you see what I mean about the confidence level when I can just completely halt type checking just by testing the value of a variable?

[samwgoldman]

I think it's worth re-opening this issue, because clearly there's more discussion to be had here. I know I have personally run into bugs because of this behavior, but since this behavior is intentional, I'm certainly not going to change it without hearing @avikchaudhuri's thoughts.

[rileylark]

I don't understand the "sentinel property" idea in 48a5482 . I get that a type may have separate shapes (as in a union type), but won't the property I'm checking for be in the type?

var val = {x: string} | {y: string} = {x: 'a'};

if (val.y) // sure, let this check even though val.y isn't defined
   doThingsAssumingY(val);

if (val.z) // why should this check for any reason?
   doCrazyUndefinedCraziness(val);

[mroch]

i think this predated more formal sentinel property checking, and probably should be fixed.

[sberan]

I would expect any sane type-checking system to flag this as an error. If a developer wanted to check for the existence of a property, flow could consider allowing this in a typeof check, which is a less error prone way of accomplishing this:

if (typeof x.baz !== 'undefined') {
 ...

This could still allow sentinel checking, while still catching many common type errors which could occur in if statements.

[raquo]

Any updates on this?

Do we have a consensus that this should be fixed, or is there still something to discuss?

I've lost a lot of confidence in Flow's type system running into this issue. I agree with sberan that no type system should allow such unsoundness.

If this issue is fixed, those who relied on this unsoundness will see a big list of places where they need to apply a trivial fix – e.g. add explicit undefined checks. On the other hand, if the issue remains unfixed, those of us who have and will accidentally introduce erroneous if-conditions have no way of listing all existing errors or making sure that we don't introduce new ones.

I think we should favor soundness in this case.

[mroch]

I have a diff for this, I'll try to get it landed when I get back to the office next week.

[sberan]

Thank you @mroch, looking forward to it!

[JunyuanZheng]

Can someone explain to me why this 'feature' is enabled again?

[philippotto]

@samwgoldman @mroch It seems as if the fix to that issue got lost again?

We regularly stumble about lacking typechecking due to this issue, which is really annoying and frustrating for our team. Is there anyway to resolve this?

Example:

type X = {
    foo: string;
};

var x: X = {foo: "bar"};

if (x.doesNotExist === "bar") {} // no error!