Fix 21732: "in" operator could widden type

[ShuiRuTian]

Fixes #21732

[ShuiRuTian]

Some questions would stop PR from merging in my thought. Have no idea about these for now, I would add new comments when idea comes.

1. The code structure
   This PR would let "in" operator has the ability to add something to a type. In other words, it would "expand" or "widden" a type rather than "narrow" it.
   Then, is it acceptable or toleratable to has one branch like narrowOrWiddenTypeByInOperator in function narrowType?
   If not, what should the code structure be like?

[ShuiRuTian]

So, what about a helper file for Type to narrow their type?

[sandersn]

There are a lot of properties that the control flow code needs to maintain that this change will violate. A full change will involve quite a lot of change to the control flow code, not just the narrowing code.

The best place to start is to add tests for in in complex control flows, like if or switch nested in while. Look at the existing control flow tests for examples.

[ShuiRuTian]

Oh my.....

Thanks a lot to the review and suggestion, I find a simple example that works wrongly.

var tmp = { a: 1, b: "foo" }
tmp; // add "e" to the type.
if ("d" in tmp) {
    tmp;
    if ("e" in tmp) {
        tmp;
    }
    tmp;
}

I would have a try to change CFA, but it is great if there is any help to explain how does it works briefly, I am totally new on this area.

-- edit:
I found this belongs to immutable things rather than CFA, and it seems work well now, just like other narrow statements!

I would continue check, but it would still be very helpful if there is an error example to analytics on CFA.

[Kingwl]

You can see __debugFlowFlags property in flowNode. That shows how the data flow works for the current code.

[ShuiRuTian]

@Kingwl Thanks for the kind help!

And...... could I ask a question? ---- What do you think of the issue? Why this PR need to change CFA?
I think 'in' operator just works like other narrow statement (like discriminant), if it has CFA issue, it should already have.

[sandersn]

@ShuiRuTian do you want to keep working on this?

[ShuiRuTian]

@sandersn Sure, but I think I need feedback -- Why this PR need to change control flow?
I think 'in' operator just works like other narrow statements (like discriminant, instanceof, typeof ...), if it has an CFA issue, it should already have.

And I do test some cases, it seems they work correctly.

[sandersn]

@weswigham can you take a look and give some direction on this?

[ShuiRuTian]

@weswigham Sorry for bothering, could you help this PR find direction when have spare time? It lost at the sea....

[ShuiRuTian]

up!

[weswigham]

The key issue we normally flag when thinking about this is branch reconciliation. Take something like this:

var tmp = { a: 1, b: "foo" };
if ("c" in tmp) {
    tmp;
}
else if ("d" in tmp) {
    tmp;
}
else {
    return;
}
tmp; // should this be `{a: number, b: string}` or `{a: number, b: string, c: unknown} | {a: number, b: string, d: unknown}`

and then there's the question of what happens when you both discriminate and "expand":

declare var x: {type: "a", f1: string} | {type: "b", f2: string};
if (x.type === "a") {
  if ("f2" in x) {
    x; // is this `never`? is this `{type: "a", f1: string, f2: unknown}` ?
  }
}
x; // needs to have gone back to `{type: "a", f1: string} | {type: "b", f2: string}` here, and not be something wonky like `({type: "a", f1: string, f2: unknown}) | {type: "b", f2: string}`

[weswigham]

This recursively merges the symbol table deeply (so overlapping symbols within the table get combined, and have a merge pointer at the new, combined symbol). You don't want that. Rather than most of this logic, you should probably just use getSpreadType, rather than everything after the // if type is intersection comment, since it already combines object types where appropriate, since you guarantee you're adding a new property in the second object anyway. (Re)Using it should also allow you to cut out a lot of the other structural changes around intersection construction in this PR, too.

[ShuiRuTian]

Thanks for the review and suggestion!

Here are the types this PR could get:
PS: In this PR(for now), use intersection rather than just "add" into origional type. which does not influence us discussing about behaviour, right?

function f(){
    var tmp = { a: 1, b: "foo" };
    if ("c" in tmp) {
        tmp; // {a: number;b: string;} & {c: unknown;}
    }
    else if ("d" in tmp) {
        tmp; // {a: number;b: string;} & {d: unknown;}
    }
    else {
        return;
    }
    tmp; // ({a: number;b: string;} & {c: unknown;}) | ({a: number;b: string;} & {d: unknown;})
}

declare var x: {type: "a", f1: string} | {type: "b", f2: string};
if (x.type === "a") {
  if ("f2" in x) {
    x; // {type: "a";f1: string;} & {f2: unknown;}
  }
}
x; // {type: "a";f1: string;} | {type: "b";f2: string;}

I was confused how to make choice between the commnets in the example code. It seems both are reasonable to some degree for me. And finally I decide to keep it just same with other narrow statements, at least it is not a bad decision in my thought, it keeps consistant.

However, I am not sure whether it is good choice on earth. My concern might be simple, sometimes navie. What do you think?

PS: need a little time to get getSpreadType into use, trying!

[sandersn]

@ShuiRuTian do you want to keep working on this?

[sandersn]

I'm going to close this PR to reduce clutter. Let me know if you want to restart work on it.