Typescript: Use control flow analysis for type guards

[jedmao]

TS 1.4

Type guard fails to narrow the union type when returning early.

function foo(x: number | string) {
    if (typeof x === 'string') {
        return;
    }
    // x should now be a number
    if (2 % x !== 0) {
        // do something
    }
}

Unfortunately, I get the following compiler error:

The right-hand side of an arithmetic operation must be of type 'any', 'number' or an enum type.

Clearly, the compiler should know at this point that it can't possibly be dealing with a string, as that scenario has already been returned.

[RyanCavanaugh]

This would be nice. We should probably have a single issue for "Use control flow analysis for type guards" to unify all the various permutations here.

[vilic]

+1 for this feature.

[jedmao]

@RyanCavanaugh I've updated the issue title, as you suggested. Happy to discuss it here.

[zspitz]

+1
Suggestion: an if block containing a throw statement should be treated the same way:

function foo(x: number | string) {
    if (typeof x === 'string') { throw 'No strings attached';}
    return x * 2;
}

[blakeembrey]

:+1: I prefer writing code without using else so this would be pretty amazing

[zpdDG4gta8XKpMCd]

:+1:

[ghost]

:+1:

[falsandtru]

+1 It is a basic coding style.

[mjohnsonengr]

:+1: this would be really awesome to have. +1 as well for the same with a throw.

[alvivi]

Right now I am using type coercion to avoid falling into a pyramid of doom, so +1.

[ivogabe]

I've opened a PR at #6959.

[TheTedAdams]

Just ran in to this issue today and had to wrap my code in an else (dirty). Would love to see this implemented!

[mhegazy]

We are actually looking into making this work as intended. marking it as such.

[Arnavion]

Please also support the get-or-put cache idiom:

declare function valueFromKey(key: string): number;
declare const cache: Map<string, number>();

function getFromCache(key: string): number {
    var value = cache.get(key); // number|undefined, assuming Map.get() is updated in es6.d.ts to return V | undefined
    if (value === undefined) {
        value = valueFromKey(key); // Should now realize that value cannot be undefined
        cache.set(key, value);
        console.log(value.length);
    }

    return value; // Should now realize that value cannot be undefined
}

---

And if possible, recognize an Array.filter() that filters out nulls or undefineds, and strip | null or | undefined from the result array type:

var arr1: (string | null)[] = ["a", null, "b"];
var arr2 = arr1.filter(el => el !== null); // arr2 should be string[]
arr2.map(el => el + "a"); // Should not complain

[JsonFreeman]

@Arnavion I think in order to get the filter behavior, you would have to change the signature of Array.filter to have a type parameter <U extends T>. Additionally, this case might be blocked on #241.

I agree about get-or-put though.

[RyanCavanaugh]

@Arnavion re: filter, see #7657

[ivogabe]

@ahejlsberg Could you take a look at #6959? I have completed most work and updated it for your awesome work on nullability, dotted names and this-keyword type guards.

[ahejlsberg]

@ivogabe Your work in #6959 is really cool and I have found it quite inspiring. The core ideas are the same in the control flow work I'm implementing right now, but I have chosen to build a separate implementation, partly to fully understand the intricacies of the algorithms and partly to ensure we get the best possible performance. Early indicators are good, performance is the same if not a little bit better than the current compiler. I'm not exactly sure when I will be able to put up a PR since I'm busy with preparation for my talk at the Build conference next week, but once I do I would love to get your feedback so we can compare and contrast. Thanks again for your great work!

[ivogabe]

@ahejlsberg Thanks for the update. I'm curious how you could achieve that performance. Is the code available somewhere or do you have it in a local branch?

[ahejlsberg]

@ivogabe I have created a branch with my control flow work as it stands now:

https://github.com/Microsoft/TypeScript/tree/controlFlowTypes

Comments are welcome, but it's not quite ready for a PR yet. For example, I'm not handling destructuring assignments. The performance delta from the current compiler looks to be within +/-5% depending on the particular test.

[ivogabe]

@ahejlsberg I found a minor issue, you might already have seen it. When a union type is assigned to a variable with a union type, narrowing does not work. Example:

let x: string | boolean | number;
x = "" || 42;
// x is string | boolean | number here, expected: string | number

If you want, I could contribute to your branch, by implementing destructuring for example, or I could copy the tests from my branch. But I don't want to hamper you of course.

[ahejlsberg]

@ivogabe Thanks, I've fixed that issue. Appreciate the offer to help out. I definitely think the tests from your branch could be useful.

[ahejlsberg]

@ivogabe Again, thanks for your work on the tests. I have fixed the issues they uncovered (except for the destructuring assignments) and updated the branch.

[ivogabe]

@ahejlsberg I found one minor bug. In the following example, the type of x in the then-block is string, whereas it would be a number at runtime:

let x: string | number | boolean;
if (typeof x === "string" && (x = 42)) {
    x
}

This is because the control flow graph looks like this: assignment -> type guard -> then-block. This can be fixed by changing the graph (by adding the operands of the && as separate type guards) or by falling back to original type if a type guard contains an assignment.

[paul-go]

Is it within scope to make this feature also operate on type intersections?

class A { a: string; }
class B { b: string; }

function fn(param: A & B)
{
    if (param instanceof A)
        return;

    param.a; // Generate a type error here?
}

[RyanCavanaugh]

@paul-go what's the intent of the code here? I would expect param: A | B given the instanceof check. It seems impossible for a A & B to actually exist in a codebase where there was checking with instanceof

[ghost]

@RyanCavanaugh You can get an A & B when both A and B are classes.

class AB extends A implements B {
  b = 'b';
}

Of course instanceof gets unreliable when you implement classes rather than extend them. This seems like a bad idea in general, but possible.

The same problem exists with interfaces, but then you can't use instanceof.

This also raises that instanceof can tell you that something _is_ a kind of thing, but not that it _is not_ that kind of thing. I'm not sure how this relates to type guards, or if it is a scenario that should be supported.

[jods4]

@paul-go The code is not a type error, since param: A & B means that param.a _has to (always) be valid_ or your typing is flaky.

Maybe it's dead code if you think you can never fail the param instanceof A test, but that's another issue.

[paul-go]

@jods4 By "Generate a type error here?", I actually meant "Can we generate a type error here?"

[paul-go]

@RyanCavanaugh: This is going to be controversial, but I think you may be making an assumption that all usages of & use Object the underlying constructor. This isn't the case. As a library designer, we've generally favored using & over |, because | requires an assertion before the members can be discovered. While on many occasions | technically more correct, we've found this to create tediousness. Most notably, in cases where a function is used widely throughout a code base, but the developer rarely wants to handle the other side of the |.

And not that we should be striving to appease the completeness gods, it also seems like a pretty gapping completeness hole :-)

[RyanCavanaugh]

FWIW I don't have strong beliefs about constructors.

In the example you have there, though, I don't understand why we would _want_ to generate an error there. If something is A & B then it doesn't matter what it's instanceof; that type concretely always has an a member.

[paul-go]

We use & as an overloading mechanism that the user doesn't need to assert before gaining access to members. If you think of it like that, then it makes sense that when the user guards with an instanceof, the other side should be culled. But I guess the intents of | and & don't exactly fit this use case.

[Arnavion]

@paul-go The point is that this is valid in TS:

class Foo { bar: string }
var foo: Foo = { bar: "a" };
console.log(foo instanceof Foo); // false even though foo is a Foo according to the type system

Thus in your example even when a is not instanceof A, it can still be an A for the purpose of type-checking. Only the inverse is true - if a is instanceof A then it must be an A for the purpose of type-checking.

[RyanCavanaugh]

If you changed the A & B to A | B, then (once the PR goes in) inside the if you'd see only the members of A, and after the if block you'd see only the members of B. That seems 100% optimal for your use case

[Arnavion]

@RyanCavanaugh

after the if block you'd see only the members of B

Just B ? Not A | B ?

[paul-go]

Our use case is to show the union of both member lists in the unguarded case, and a single member list in the guarded case. As it stands, the intersection of both member lists are shown in the unguarded case:

[ghost]

I think @paul-go is using type intersections to model what are more accurately type unions, because he feels that type unions are too cumbersome to use properly. I don't think "modelling it incorrectly because the correct way is inconvenient" is a good use-case to support.

[jods4]

@paul-go from your description I think that you don't use intersection types the way they are intended... And I have the impression that your request is twisting & to match your perception/usage of the feature, which doesn't seem correct.

let's take your last example:

// or is it A & B ??
function fn(param: A | B) {
}

You have to decide if param _always_ has members of _both_ A and B (which is the meaning of A & B) or if it can be _either_ a A or a B (which is the meaning of A | B).

Accessing members of _both_ in an unguarded way would be a bug if the object is sometimes a A, sometimes a B. It is only correct if param _always_ satisfies both contracts. And if it does, type guards are meaningless, because the object satisfies all contracts all the time!

Accessing members behind a guard kind of implies that param is _not always_ both types. This would be A | B and TS guards are well adapted for consuming this object in a safe, simple way without casts.

If you actually have A & B and can access all members in an unguarded way but want to reduce IntelliSense for some weird reason, you can always use a cast:

let param: A & B;
param.a = 1; // ok
param.b = 2; // ok
let restricted = <A>param;
restricted.a = 3; // ok
restricted.b = 4; // type error, no b on A. (actually ok at runtime, though)

[paul-go]

Yes, I'm aware of how type guards work. But all of this is pointless banter as TS isn't going to support our unfortunately bizarre use case.

[RyanCavanaugh]

@paul-go Reading between the lines here, it seems like you'd like possibly-present members of union types to show up in the completion list. We already have this functionality for JavaScript code and could think about moving it to TypeScript if that'd be useful. One caveat would be that obviously these maybe-present members wouldn't actually work without a type guard of some sort, but it seems like you're writing that code anyway (just _later_). As in JS, we could show those with a sort of 'warning' formatting connotation. Thoughts?

[ghost]

@RyanCavanaugh Is what you're proposing similar to generating an implicit interface that is a supertype of all types in the union, with the non-common properties marked optional?

[paul-go]

@RyanCavanaugh By "wouldn't actually work", I'm assuming you mean "will emit a compiler error when used"? If that's the case, unless I'm misunderstanding, I'm not sure if it's helpful to display provably-faulty members (at least as far as TS is concerned) in a completion list.

[jods4]

@RyanCavanaugh Doesn't seem like a high-priority to me, but would probably fall into the "nice to have" bucket to me.

If the IntelliSense presentation is well-done: dimmed member (because it's not legal to use at this point) with the containing types on the right (to provide insight as to which guard is required) would give an interesting glance at the underlying types. Better than, say, a completely empty IntelliSense popup.

[DanielRosenwasser]

I'm not sure if it's helpful to display provably-faulty members (at least as far as TS is concerned) in a completion list

This is actually why we don't display _all_ the members from union types. We don't know what type you have, so until you find a way to tell us, we're going to have to give an error if you use any member that the other types don't have. That's basically the issue.

For the record, there was a lot of discussion on the completion list scenario when we were first thinking of union types. Exploratory coding is _really_ helpful, and keeping friction low to leverage the IDE to do so makes users happy. Check #5334 out for something similar. Maybe this could be part of that suggestion.

But if this is something we'd like to see, let's open another issue since the conversation has moved into something orthogonal to the main suggestion here.

[JsonFreeman]

There's room for debate on this question, but I have a feeling that showing the extra properties (even grayed out), and then erroring on them is a bad experience. Though I suppose an argument could be made for showing the properties in the list (along with the associated type), but refusing to auto-complete them, but I think this would also be odd.

[ahejlsberg]

@ivogabe I just pushed some changes to the controlFlowTypes branch that break && and || operators down to their actual control flows. This should fix the issue you pointed out. Also, I have done more work on destructuring assignments. I expect to put up the official pull request for all of this next week.