Control flow based type analysis #8010

Merged
merged 70 commits into from Apr 22, 2016

Projects

None yet
@ahejlsberg
Member

This PR introduces control flow based type analysis for local variables and parameters as initially suggested in #2388 and prototyped in #6959. Previously, the type analysis performed for type guards was limited to if statements and ?: conditional expressions and didn't include effects of assignments and control flow constructs such as return and break statements. With this PR, the type checker analyses all possible flows of control in statements and expressions to produce the most specific type possible (the narrowed type) at any given location for a local variable or parameter that is declared to have a union type.

Some examples:

function foo(x: string | number | boolean) {
    if (typeof x === "string") {
        x; // type of x is string here
        x = 1;
        x; // type of x is number here
    }
    x; // type of x is number | boolean here
}

function bar(x: string | number) {
    if (typeof x === "number") {
        return;
    }
    x; // type of x is string here
}

Control flow based type analysis is particuarly relevant in --strictNullChecks mode because nullable types are represented using union types:

function test(x: string | null) {
    if (x === null) {
        return;
    }
    x; // type of x is string in remainder of function
}

Furthermore, in --strictNullChecks mode, control flow based type analysis includes definite assignment analysis for local variables of types that don't permit the value undefined.

function mumble(check: boolean) {
    let x: number; // Type doesn't permit undefined
    x; // Error, x is undefined
    if (check) {
        x = 1;
        x; // Ok
    }
    x; // Error, x is possibly undefined
    x = 2;
    x; // Ok
}

The narrowed type of a local variable or parameter at a given source code location is computed by starting with the initial type of the variable and then following each possible code path that leads to the given location, narrowing the type of the variable as appropriate based on type guards and assignments.

  • The initial type of local variable is undefined.
  • The initial type of a parameter is the declared type of the parameter.
  • The initial type of an outer local variable or a global variable is the declared type of that variable.
  • A type guard narrows the type of a variable in the code path that follows the type guard.
  • An assignment (including an initializer in a declaration) of a value of type S to a variable of type T changes the type of that variable to T narrowed by S in the code path that follows the assignment.
  • When multiple code paths lead to a particular location, the narrowed type of a given variable at that location is the union type of the narrowed types of the variable in those code paths.

The type T narrowed by S is computed as follows:

  • If T is not a union type, the result is T.
  • If T is a union type, the result is the union of each constituent type in T to which S is assignable.

Thanks to @ivogabe for providing inspiration and tests for this PR.

Fixes #2388.

ahejlsberg and others added some commits Mar 22, 2016
@ahejlsberg ahejlsberg Initial implementation of control flow based type analysis e67d15a
@ahejlsberg ahejlsberg Add type annotations to suppress circularity errors afa1714
@ahejlsberg ahejlsberg Fixing tests 7c45c7b
@ahejlsberg ahejlsberg Accepting new baselines 80c2e5e
@ahejlsberg ahejlsberg Adding a few optimizations 33985b2
@ahejlsberg ahejlsberg Handle assignment of union types in getAssignmentReducedType ed5002c
@ahejlsberg ahejlsberg Remove incorrect type predicate (could be true even when result is fa…
…lse)
6d25a42
@ahejlsberg ahejlsberg Fix overly aggressive optimization bf78470
@ivogabe ivogabe Add control flow tests 4f936c4
@ahejlsberg ahejlsberg Merge pull request #7690 from ivogabe/controlFlowTypesTest
Adds tests to control flow types branch
7f02357
@ahejlsberg ahejlsberg Fix issues in analysis of do..while and for..in/for..of 9e965d4
@ahejlsberg ahejlsberg Fix comment in test 9de0a5d
@ahejlsberg ahejlsberg Accepting new baselines 560bc3f
@ahejlsberg ahejlsberg Fixing some tests 0820249
@ahejlsberg ahejlsberg Accepting new baselines 5a5d89a
@ahejlsberg ahejlsberg Use type {} for vacuous type guards / New getTypeWithFacts function 424074b
@ahejlsberg ahejlsberg Remove unnecessary cast c6f4de3
@ahejlsberg ahejlsberg Fix some tests e53f390
@ahejlsberg ahejlsberg Accepting new baselines a38d863
@ahejlsberg ahejlsberg Delete removeNullableKind, use getTypeWithFacts instead 3d0fa31
@ahejlsberg ahejlsberg Support unknown types (host object names) in typeof type guards ce81ba5
@ahejlsberg ahejlsberg Separate error messages for 'null', 'undefined', or both. 354fd10
@ahejlsberg ahejlsberg Flow analysis of &&, ||, and destructuring assignments 5179dd6
@ahejlsberg ahejlsberg Accepting new baselines 019f5bd
@ahejlsberg ahejlsberg Handle shorthand property assignments f13c92f
@ahejlsberg ahejlsberg Accepting new baselines b03d087
@ahejlsberg ahejlsberg Support destructuring declarations in control flow analysis 7a32129
@ahejlsberg ahejlsberg Accepting new baselines 6fb9424
@ahejlsberg ahejlsberg Adding test e45bac8
@ahejlsberg ahejlsberg Fixing fourslash tests 7dfcad6
@ahejlsberg ahejlsberg Fixing tests 92df029
@ahejlsberg ahejlsberg Accepting new baselines 32e6464
@ahejlsberg ahejlsberg Fix linting errors 560e768
@ahejlsberg ahejlsberg Merge branch 'master' into controlFlowTypes
Conflicts:
	src/compiler/checker.ts
	tests/baselines/reference/typeAssertions.errors.txt
b1e9f43
@ahejlsberg ahejlsberg Accepting new baselines
4c250d0
@weswigham weswigham commented on the diff Apr 11, 2016
src/compiler/binder.ts
- const preWhileState =
- n.expression.kind === SyntaxKind.FalseKeyword ? Reachability.Unreachable : currentReachabilityState;
- const postWhileState =
- n.expression.kind === SyntaxKind.TrueKeyword ? Reachability.Unreachable : currentReachabilityState;
-
- // bind expressions (don't affect reachability)
- bind(n.expression);
-
- currentReachabilityState = preWhileState;
- const postWhileLabel = pushImplicitLabel();
- bind(n.statement);
- popImplicitLabel(postWhileLabel, postWhileState);
+ function isNarrowableReference(expr: Expression): boolean {
+ return expr.kind === SyntaxKind.Identifier ||
+ expr.kind === SyntaxKind.ThisKeyword ||
+ expr.kind === SyntaxKind.PropertyAccessExpression && isNarrowableReference((<PropertyAccessExpression>expr).expression);
@weswigham
weswigham Apr 11, 2016 Contributor

Should ElementAccessExpressions be considered narrowable if their expression is narrowable, just like PropertyAccessExpressions?

@ivogabe
ivogabe Apr 11, 2016 Contributor

That could be done, but only if the expression between the brackets is a constant (string/number literal, enum value etc)

@weswigham
weswigham Apr 11, 2016 Contributor

I think we do that kind of special-casing for literal element access expressions elsewhere - we should probably do it here, as well.

@mhegazy
mhegazy Apr 15, 2016 Contributor

literals and well-known symbols as well.

@ahejlsberg
ahejlsberg Apr 15, 2016 Member

Yes, it would be good to support element access expressions with string literals and well known symbols. I think we can cover it in a separate PR though.

@weswigham weswigham commented on the diff Apr 11, 2016
src/compiler/checker.ts
+ if (callExpression.expression.kind === SyntaxKind.PropertyAccessExpression &&
+ isMatchingReference((<PropertyAccessExpression>callExpression.expression).expression, target)) {
+ return true;
+ }
+ return false;
+ }
+
+ function getFlowTypeCache(flow: FlowNode): Map<Type> {
+ if (!flow.id) {
+ flow.id = nextFlowId;
+ nextFlowId++;
+ }
+ return flowTypeCaches[flow.id] || (flowTypeCaches[flow.id] = {});
+ }
+
+ function isNarrowableReference(expr: Node): boolean {
@weswigham
weswigham Apr 11, 2016 Contributor

This function is duplicated both here and in the binder. Should it be shared?

@weswigham weswigham commented on the diff Apr 11, 2016
src/compiler/utilities.ts
@@ -1504,7 +1529,7 @@ namespace ts {
}
// True if the given identifier, string literal, or number literal is the name of a declaration node
- export function isDeclarationName(name: Node): name is Identifier | StringLiteral | LiteralExpression {
+ export function isDeclarationName(name: Node): boolean {
@weswigham
weswigham Apr 11, 2016 Contributor

Why was the type-guardiness of this function removed?

@ivogabe
ivogabe Apr 11, 2016 Contributor

Probably because this function can also return true if the node is some identifier, but not used as a declaration. With the control flow checks, the code after an if with a return will be considered to be the else block, which might have caused some issues.

@weswigham
weswigham Apr 11, 2016 Contributor

If that was the case, then wouldn't the name of the function be misleading?

@ivogabe
ivogabe Apr 11, 2016 Contributor

Not in my opinion, it checks whether a node is a declaration name, but not every identifier is a declaration name. So the type annotation was wrong, not the name.

@weswigham
weswigham Apr 11, 2016 Contributor

Seems fair.

@ahejlsberg
ahejlsberg Apr 11, 2016 Member

Yes, @ivogabe is exactly right about why I removed the type predicate annotation.

@weswigham weswigham commented on the diff Apr 11, 2016
src/compiler/parser.ts
@@ -1811,7 +1811,7 @@ namespace ts {
function parseEntityName(allowReservedWords: boolean, diagnosticMessage?: DiagnosticMessage): EntityName {
let entity: EntityName = parseIdentifier(diagnosticMessage);
while (parseOptional(SyntaxKind.DotToken)) {
- const node = <QualifiedName>createNode(SyntaxKind.QualifiedName, entity.pos);
+ const node: QualifiedName = <QualifiedName>createNode(SyntaxKind.QualifiedName, entity.pos); // !!!
@weswigham
weswigham Apr 11, 2016 Contributor

What's the new type annotation and // !!! comment for?

@ivogabe
ivogabe Apr 11, 2016 Contributor

Since Entity is a type alias of a union, it will be narrowed after its assignment. This creates a circular dependency during the type analysis. Because of this, node would be typed as any if the type annotation was not given.

@ahejlsberg Would it be possible to give the inference of the type of the initializer precedence over the narrowing after assignments?

@ahejlsberg
ahejlsberg Apr 11, 2016 Member

The exact problem here is the following: We infer the type of node from the call to createNode. To evaluate that call we need to know the type of entity so we can find the type of the pos property. To find the type of entity we look at the preceding code paths. One leads from the top of the function. In that code path, entity has type Identifier, which is less than its full declared type (Identifier | QualifiedName). Therefore we need to also analyze the second code path that comes from the bottom of the while statement (the loop around case). In that code path, we have the assignment entity = finishNode(node) which requires us to know the type of node. Boom! Circularity. Not exactly clear what rule we'd introduce to break that circularity.

@DanielRosenwasser
DanielRosenwasser Apr 12, 2016 Member

Would just assuming that entity has its original declared type be good enough?

@JsonFreeman
JsonFreeman Apr 12, 2016 Contributor

One other idea is that while doing the analysis, if you hit a type assertion, ignore the RHS, and just use the type associated with the type assertion.

@ahejlsberg
ahejlsberg Apr 12, 2016 Member

@DanielRosenwasser But by exactly what mechanism would we sometimes just use the declared type and other times compute the narrowed type?

@JsonFreeman Yes, but we'd then have to introduce a distinction between resolving the type of an expression (in which case we could defer and ignore the RHS) and checking the type of an expression. We don't have such a distinction currently.

@weswigham
weswigham Apr 12, 2016 Contributor

@ahejlsberg - this doesn't seem circular to me, it seems sequential. I'm imagining if the first iteration of the loop were unrolled - don't we just need to take the partially resolved type for entity from the first iteration and use it as input for the type of entity for the second iteration? Once you've reached a stable signature (or cycle of signatures) by this method of loop-unrolling, you'd just need the union of all types made in each iteration for the overall type at that location, no?

@ahejlsberg
ahejlsberg Apr 12, 2016 Member

@weswigham Yes, but the compiler is by and large constructed with the assumption that once we compute a symbol's declared type, that type can and will be cached and never changes. However, to do what you suggest we'd have to compute a new declared type for the node symbol in each iteration because entity depends on node, which depends on entity, etc. We're simply not equipped to do that currently.

@JsonFreeman
JsonFreeman Apr 12, 2016 Contributor

I think the problem of circularities isn't new, this is just a new instance of it. We've looked at this in the past with regard to inferring types of symbols that recursively reference themselves. For these cases, I believe we decided that it's sometimes helpful to resolve an expression without fully diving into it. But it was a challenge to formalize and implement. I think this boils down to the same thing.

@weswigham
weswigham Apr 13, 2016 Contributor

Just to make the issue into a concrete, self-contained test, I'll drop this here:

interface First {first: 'first'}
interface Second {second: 'second'}
interface Third {third: 'third'}

declare function next(obj: Third): undefined;
declare function next(obj: Second): Third;
declare function next(obj: First): Second;

declare function isThird(x: any): x is Third;

var x: First | Second | Third;
x = {first: 'first'};
x.first;
while (true) {
    x = next(x);
    x.second; // Should be forbidden (x is Third in the second iteration), but is allowed
    if (isThird(x)) {
        break;
    }
}

x.third; // Should be allowed (loop always exits when x is Third), but is forbidden

Even if we don't fix the issue, the current behavior can be awkward. (x ends up typed as Second after the loop, which is very incorrect, given it will never be Second at that point.)

@JsonFreeman
JsonFreeman Apr 13, 2016 Contributor

@weswigham good example! I think even if it the system is not always correct, it's better if it is conservative. So in your example, I could envision an acceptable solution where at the two property accesses, x is of type Second | Third. That way the implementation could get to a point where it is always either correct, or is too conservative because of a reassignment in a loop. And then loops could be fixed up later while people try out the feature.

@weswigham
weswigham Apr 13, 2016 Contributor

@JsonFreeman we can't be too conservative, though - for example, in my example above, if I omitted

    if (isThird(x)) {
        break;
    }

from the while loop, then next(x) could return undefined (as x could be Three at the start of the loop) - which isn't assignable to x under strict null mode - which should cause an error within the loop! Making it non-cumbersome to use overloads like this could be difficult without a complete fix for the issue.

@JsonFreeman
JsonFreeman Apr 13, 2016 Contributor

Yes that's a good point. It's not a simple matter of using the declared type. But I think the overloaded next function wouldn't even accept a union type as an argument.

@weswigham
weswigham Apr 13, 2016 Contributor

No, it wouldn't with how TS resolves signatures at present - at least not without another signature declaring it as taking a parameter of type any or a union of the other types. (Which, for some reason feels off to me - if every member of a union of argument types can be fulfilled by an overload and there's no better match, why can't the return be the union of those overloads' return values? Question for another time.)

@JsonFreeman
JsonFreeman Apr 13, 2016 Contributor

(Answer for another time ;)

@ahejlsberg
ahejlsberg Apr 13, 2016 Member

@weswigham @JsonFreeman There are actually several issues being debated here:

  • Circularities occurring because we're more eager than we need to when resolving (as opposed to checking) the type of an expression. As Jason points out, that's not a new issue. We could make progress on this issue by making a distinction between resolving and checking an expression, and only evaluate as much as we need to when resolving. Indeed, we could solve the actual cases in the compiler by doing this for type assertion expressions.
  • Inability for symbols to have evolving declared types. This one is much harder. I really don't want to go there unless we absolutely have to.
  • Compute narrowed types by iterating to a fixed point. We only do this in limited form today and, as Wes' example shows, we may need to do more here. I'm not super concerned with crazy examples that walk up a ladder of overloads, but I can indeed imagine real world examples where it occurs without overloading. Although I haven't actually seen any in the wild yet. I will continue to think about this issue.
@JsonFreeman
JsonFreeman Apr 13, 2016 Contributor

Thanks Anders, I think that's a great breakdown. I'll add that (if I'm not mistaken), this separation of resolution and checking is in place for bodies of function expressions. The idea here would be to generalize it for other expressions.

I agree that iterating to a fixed point may be necessary for loops in which the variable is reassigned in the loop body.

@weswigham weswigham commented on the diff Apr 11, 2016
src/compiler/parser.ts
@@ -3643,7 +3643,7 @@ namespace ts {
let elementName: EntityName = parseIdentifierName();
while (parseOptional(SyntaxKind.DotToken)) {
scanJsxIdentifier();
- const node = <QualifiedName>createNode(SyntaxKind.QualifiedName, elementName.pos);
+ const node: QualifiedName = <QualifiedName>createNode(SyntaxKind.QualifiedName, elementName.pos); // !!!
@weswigham
weswigham Apr 11, 2016 Contributor

Same comment as above:

What's the new type annotation and // !!! comment for?

@ahejlsberg
ahejlsberg Apr 11, 2016 Member

Same issue.

@weswigham weswigham and 1 other commented on an outdated diff Apr 11, 2016
src/compiler/checker.ts
@@ -16326,7 +16328,7 @@ namespace ts {
}
if (entityName.parent.kind === SyntaxKind.ExportAssignment) {
- return resolveEntityName(<Identifier>entityName,
+ return resolveEntityName(<Identifier><EntityName>entityName,
@weswigham
weswigham Apr 11, 2016 Contributor

This new cast seems unneeded.

@ahejlsberg
ahejlsberg Apr 11, 2016 Member

Yes, that extra cast was a left over from a work around for the earlier issue with the isDeclarationName user defined type guard.

@ahejlsberg ahejlsberg referenced this pull request Apr 11, 2016
Merged

Non-nullable types #7140

@jods4
jods4 commented Apr 11, 2016

If T is not a union type, the result is T.

Would it be interesting to consider sub-classes narrowing? Just thinking out loud, not sure if it has value:

class Human { };

class Wizard extends Human {
  castSpell() : void;
};

let x: Human;
x = new Wizard();
x.castSpell(); // <- statically ok?

I guess I'll be the one to start the heated discussion, please don't hate me:

Furthermore, in --strictNullChecks mode, control flow based type analysis includes definite assignment analysis for local variables of types that don't permit the value undefined.

Should you do the same check for a variable that permit the value undefined? I am personnally OK with the answer "no", but some people want to use undefined as their "empty" value, maybe even promote some sugar for it (either var x?: number or var x: number?). Under those assumptions, I would find the following code suspicious in a strongly-typed language:

let x?: number;
// lots of fancy logic with a single unassigned code path
return x; // did we intend x to be undefined here, or did we forget one code path?

I won't be championing that as I'm using null for my empty values anyway. But to be fair with the undefined fans I think it has to be asked.

@RyanCavanaugh
Member

@jods4 we were discussing the exact Human / Wizard scenario earlier and have some theories about what might work.

The problem with directly narrowing is cases like this:

interface Options {
  name: string;
  length?: number;
  width?: number;
}
let x: Options = { name: 'a' }; // 1
x = { name: 'other', length: 10 }; // 2
x.width = 32; // 3

On line 2, we can't blindly take the RHS type and overwrite the declared type of x, because it'd break line 3.

It hasn't been implemented yet, but using intersection may solve this problem (in your example, we'd assign the type Human & Wizard to x and allow the method call).

One open question from that discussion was whether or not this code ought to be an error:

let x: { a?: string; b?: number };
let y: { a: string; } = { a: 'foo' };
x = y;
x.b = 30;
@ivogabe
Contributor
ivogabe commented Apr 11, 2016

@jods4 @RyanCavanaugh You can also get issues with generics:

let x: Array<string | number>;
x = ["a", "b"];
x.push(42)

If x would be narrowed to Array<string>, the last line would give an error. I think narrowing only union types after an assignment would support most use cases.

@jods4
jods4 commented Apr 11, 2016

@RyanCavanaugh Structural typing with optional members is indeed more complicated: reading is ok, but you may want to assign missing optional parent members!

I think your & idea would work fine: the initial variable keeps its declared type, yet is extended with additionnal members that are provably there by definite assignment.

The Wizard was "real" inheritance, which seems easy as you can't hide/miss existing members.
Wizard extends Human => Human & Wizard simplifies to just Wizard, right?

I think your x / y example ought to work. x was declared as having an optional member b, so you should be allowed to assign x.b no matter what the underlying object is.
If there should be an error then it would be the assignment x = y, but under current TS structural typing rules it's fine. I think that saying y has the shape { a: string } doesn't exclude the fact that it may have other members, or prevent it from gaining new members in other ways than manipulating y... Types in TS are not closed.
Applying & logic in this case would yield this result.

Note that your code is unsafe for another reason, though:

let x: { a?: string; b?: number };
let y: { a: string; } = { a: 'foo' };
x = y;
x.a = undefined;
y.a.length; // oops!

On what line is this an error?

@ivogabe This is a different case... Array<string> is not a subclass of Array<string | number> (which was my initial suggestion). And if we apply @RyanCavanaugh's extended logic to all types, then we get x: Array<string> & Array<string | number> which would work just fine in your example?

@ivogabe
Contributor
ivogabe commented Apr 11, 2016

@ahejlsberg In my branch, I only tried to figure out the type of an assignment if the variable was of a union type. In this branch, you always try to get the type, and then only narrow if the variable is a union type. For non-union variables, I only check whether it was assigned, not what. That might prevent some issues, for instance issues similar to the one with the // !!! comment. And it might improve the performance of the language service a bit.

@jods4 @RyanCavanaugh Sorry, I thought you were talking about subtypes instead of subclasses. Though in a structural type system, subclasses do not really exist, strictly spoken. I think that intersection types would be a better way to model this, but they can still have issues with generics:

function sum(xs: number[]) { return xs.reduce(...) }
let x: Array<string | number>;
x = [1, 4];
// x: Array<number> & Array<string | number>
x.push("");
sum(x);

This would compile with the proposed behavior. Also error messages would be uglier with this change. I think this would be something to consider after this PR.

@ivogabe
Contributor
ivogabe commented Apr 11, 2016

@ahejlsberg Would the following code show an error that x might not be assigned (eg, might be null)?

let x: string;
x = "";
for (;;) {
    x = x;
    x.substring; // <-- here
}

I'm asking this because of the loop, which causes the narrowing after the assignment to fail (due to recursion). Though I think that that should not break assignment checking, as an assigned value should not be null.

@jods4
jods4 commented Apr 11, 2016

@ivogabe Your example is already broken in TS today, because we have array covariance like in C# and Java... Without any narrowing:

var x: (string | number)[];
var y = [0, 1];
x = y;
x.push("two");
y[2].toPrecision(3); // runtime fail: y[2] is not a number

And the same happens with nullables, because reading is covariant and writing contra-variant but the language just goes for co- all the time, copied from my comment above:

let x: { a?: string; b?: number };
let y: { a: string; } = { a: 'foo' };
x = y;
x.a = undefined;
y.a.length; // oops!

The & idea just extends how the language already works. Not saying we must have it, I was just submitting some random thoughts about the narrowing description by @ahejlsberg.

@ahejlsberg
Member

@ivogabe If you look at the code in checkIdentifier starting here you'll see that if the initial type of a variable is its declared type, and if the variable is not of a "narrowable" type (i.e. not a union type but for example a primitive type), then we don't compute the narrowed type. This is definitely an important optimization.

There is no error in your example:

let x: string;
x = "";
for (;;) {
    x = x;
    x.substring; // Ok
}

Since the declared type of x is string and the narrowed type of x is string in the first code path into the loop, we never even examine the back branch because there is nothing it could do to further affect the type of x. Specifically, when there are multiple code paths leading to a given location, if one of the code paths already yields the declared type (the least specific type the variable could have), then no other code path could affect the result because we're going to union the types together in the end.

@DanielRosenwasser DanielRosenwasser commented on the diff Apr 11, 2016
src/compiler/binder.ts
- currentReachabilityState = ifTrueState;
+ function createFlowAssignment(antecedent: FlowNode, node: Expression | VariableDeclaration | BindingElement): FlowNode {
+ return <FlowAssignment>{
@DanielRosenwasser
DanielRosenwasser Apr 11, 2016 Member

Could you simply use these types as return types instead of using a type assertion?

@DanielRosenwasser DanielRosenwasser commented on the diff Apr 11, 2016
src/compiler/binder.ts
}
}
- function bindReturnOrThrow(n: ReturnStatement | ThrowStatement): void {
- // bind expression (don't affect reachability)
- bind(n.expression);
- if (n.kind === SyntaxKind.ReturnStatement) {
+ function isTopLevelLogicalExpression(node: Node): boolean {
@DanielRosenwasser
DanielRosenwasser Apr 11, 2016 Member

What exactly makes something top-level? I can pretty much figure if out if I dig into this, but can you document the intent?

@ahejlsberg
ahejlsberg Apr 12, 2016 Member

A logical expression (an && or || expression) is top-level if it has no effect on control flow beyond itself. Examples include logical expressions used as statement expressions and logical expressions passed as arguments.

@birbilis
birbilis Sep 23, 2016

is it just for logical expressions or for other expressions too? cause I was wondering (language agnostically) what happens if something always raises an exception but isn't a throw statement (like dividing by zero), should one be clever enough to handle it? And should they also resolve constant values first or other statements that are calculatable at checking time, then check for such a case? Or would it be too far fetched, behaving like an AI coprogrammer?

@DanielRosenwasser DanielRosenwasser commented on an outdated diff Apr 12, 2016
src/compiler/checker.ts
- type: esSymbolType,
- flags: TypeFlags.ESSymbol
- },
- "undefined": {
- type: undefinedType,
- flags: TypeFlags.ContainsUndefinedOrNull
- }
+ const enum TypeFacts {
+ None = 0,
+ TypeofEQString = 1 << 0, // typeof x === "string"
+ TypeofEQNumber = 1 << 1, // typeof x === "number"
+ TypeofEQBoolean = 1 << 2, // typeof x === "boolean"
+ TypeofEQSymbol = 1 << 3, // typeof x === "symbol"
+ TypeofEQObject = 1 << 4, // typeof x === "object"
+ TypeofEQFunction = 1 << 5, // typeof x === "function"
+ TypeofEQHostObject = 1 << 6, // typeof x === "xxx"
@DanielRosenwasser
DanielRosenwasser Apr 12, 2016 Member

Extra space after =

@DanielRosenwasser DanielRosenwasser commented on the diff Apr 12, 2016
src/compiler/checker.ts
- type: numberType,
- flags: TypeFlags.NumberLike
- },
- "boolean": {
- type: booleanType,
- flags: TypeFlags.Boolean
- },
- "symbol": {
- type: esSymbolType,
- flags: TypeFlags.ESSymbol
- },
- "undefined": {
- type: undefinedType,
- flags: TypeFlags.ContainsUndefinedOrNull
- }
+ const enum TypeFacts {
@DanielRosenwasser
DanielRosenwasser Apr 12, 2016 Member

TypeFacts is an odd name for this. Maybe TypeNarrowFlags?

@weswigham
weswigham Apr 12, 2016 Contributor

I dunno, TypeFacts seems right to me - it's a flag which aggregates the facts (is truthy, is not null, etc) a program has proven about a type at a given location.

@DanielRosenwasser
DanielRosenwasser Apr 12, 2016 Member

Maybe, though, when I think ____Facts, I either think of SyntaxFacts (which we also used to have) or this.

@JsonFreeman JsonFreeman commented on the diff Apr 12, 2016
src/compiler/checker.ts
+ const enum TypeFacts {
+ None = 0,
+ TypeofEQString = 1 << 0, // typeof x === "string"
+ TypeofEQNumber = 1 << 1, // typeof x === "number"
+ TypeofEQBoolean = 1 << 2, // typeof x === "boolean"
+ TypeofEQSymbol = 1 << 3, // typeof x === "symbol"
+ TypeofEQObject = 1 << 4, // typeof x === "object"
+ TypeofEQFunction = 1 << 5, // typeof x === "function"
+ TypeofEQHostObject = 1 << 6, // typeof x === "xxx"
+ TypeofNEString = 1 << 7, // typeof x !== "string"
+ TypeofNENumber = 1 << 8, // typeof x !== "number"
+ TypeofNEBoolean = 1 << 9, // typeof x !== "boolean"
+ TypeofNESymbol = 1 << 10, // typeof x !== "symbol"
+ TypeofNEObject = 1 << 11, // typeof x !== "object"
+ TypeofNEFunction = 1 << 12, // typeof x !== "function"
+ TypeofNEHostObject = 1 << 13, // typeof x !== "xxx"
@JsonFreeman
JsonFreeman Apr 12, 2016 Contributor

What is HostObject?

@ahejlsberg
ahejlsberg Apr 12, 2016 Member

Objects defined by the ECMAScript standard are called native objects. Other objects, such as DOM objects, are called host objects. From a TS point of view, anything for which typeof returns a value other than string, number, boolean, symbol, object, function, or undefined is considered a host object.

@JsonFreeman
JsonFreeman Apr 12, 2016 Contributor

Oh yes, like the DOMElements

ahejlsberg added some commits Apr 12, 2016
@ahejlsberg ahejlsberg A few cosmetic changes 7c7a1c0
@ahejlsberg ahejlsberg Merge branch 'master' into controlFlowTypes
df62fa0
@sandersn sandersn commented on an outdated diff Apr 12, 2016
src/compiler/binder.ts
@@ -134,6 +129,9 @@ namespace ts {
let Symbol: { new (flags: SymbolFlags, name: string): Symbol };
let classifiableNames: Map<string>;
+ const unreachableFlow: FlowNode = { kind: FlowKind.Unreachable };
+ const reportedUncreachableFlow: FlowNode = { kind: FlowKind.Unreachable };
@sandersn
sandersn Apr 12, 2016 Member

typo: reportedUnreachableFlow

@ivogabe
Contributor
ivogabe commented Apr 12, 2016

@ahejlsberg Thanks for the explanations. I have played a bit with it and it works pretty good. Though I found two issues. First, the assignment checks report multiple errors for the same issue. In this example, the compiler reports an error that x is not assigned and that substring does not exist on the empty type. I would say that even though x will be undefined there, it would be better to type it as the initial type there, since you already got the real error.

    let x: string;
    x.substring;
//  ~            Used before assigned
//    ~~~~~~~~~  `substring` does not exist on `{}`

In unreachable code, you will get a lot of not-assigned errors:

let x = "";
while (true);
x = ""; // Unreachable
x; // Used before assigned

I think that you shouldn't get the error there. Also, I think that if the third line would be absent, it should still not give an error that x is not assigned since you already have the unreachable code error. Instead, it can fall back to the initial type.

A more serious issue is that the back flow of a loop only works if the condition is a type guard. In the first example, y is narrowed to string, so number is missing. The second example works correctly.

let y: string | number | boolean;
y = "";
while (true) {
    y; // string
    y = 1;
}
let y: string | number | boolean;
y = "";
while (typeof x === "string") {
    y; // string | number
    y = 1;
}
@ahejlsberg
Member

@ivogabe The problem with the missing back flow is caused by an overly aggressive optimization in the finishFlow function in the binder. Good catch. I'll have a fix for it shortly.

ahejlsberg added some commits Apr 12, 2016
@ahejlsberg ahejlsberg Fix finishFlow function and rename to finishFlowLabel 586ac55
@ahejlsberg ahejlsberg Adding regression test cd88f1e
@ahejlsberg ahejlsberg Accepting new baselines
472ab7c
@aleksey-bykov

might be relevant
#4040

@ahejlsberg ahejlsberg Improving error reporting as suggested in code review
b689c07
@ahejlsberg
Member

@ivogabe Improved the error reporting as you suggested. It does indeed reduce the noise.

@mhegazy mhegazy commented on an outdated diff Apr 15, 2016
src/compiler/binder.ts
- bind(n.incrementor);
-
- bind(n.statement);
-
- // for statement is considered infinite when it condition is either omitted or is true keyword
- // - for(..;;..)
- // - for(..;true;..)
- const isInfiniteLoop = (!n.condition || n.condition.kind === SyntaxKind.TrueKeyword);
- const postForState = isInfiniteLoop ? Reachability.Unreachable : preForState;
- popImplicitLabel(postForLabel, postForState);
+ function isNarrowingBinaryExpression(expr: BinaryExpression) {
+ switch (expr.operatorToken.kind) {
+ case SyntaxKind.EqualsEqualsToken:
+ case SyntaxKind.ExclamationEqualsToken:
+ case SyntaxKind.EqualsEqualsEqualsToken:
+ case SyntaxKind.ExclamationEqualsEqualsToken:
@mhegazy
mhegazy Apr 15, 2016 Contributor

CommaToken?

ahejlsberg added some commits Apr 16, 2016
@ahejlsberg ahejlsberg Improved handing of evolving types in iteration statements 921efec
@ahejlsberg ahejlsberg Adding tests 9aea708
@ahejlsberg ahejlsberg Accepting new baselines
10889a0
@ahejlsberg
Member
ahejlsberg commented Apr 16, 2016 edited

Latest commit implement what I think is a reasonable compromise for the "evolving types" issue discussed here. Now, when computing the narrowed type of a variable reference leads to resolving call signatures (for example because the variable is assigned the result of a function call along the control flow path), we don't cache the result of those call signature resolutions. Thus, in the subsequent actual type check of the call, we'll re-check with the further evolved type of the variable. Effectively, this handles evolving types as long as the types settle out after the first iteration. It does not handle cycles or ladders of overloads by exhaustively iterating to a fixed point, but I don't really consider those to be real world scenarios anyway.

Here's an example that is now handled correctly:

declare function foo(s: string | number): number;

function f() {
    let x: string | number | boolean;
    x = "";
    while (cond) {
        x; // string | number
        x = foo(x);
        x; // number
    }
    x; // string | number
}

Determination of the type of the first reference to x within the loop proceeds as follows: First we visit the code path leading to the loop. That produces type string for x, which we cache. Next we examine the loop-back code path. That requires us to resolve the call to foo(x). This in turn requires us to determine the type of x, which leads us back to the top of the loop. Here we see string cached from the first code path and an in-process marker for the loop-back path, so we consider the type of x to be string for now. This is used to resolve foo(x) to type number, and the result of the resolution of the loop-back path becomes number, which we cache. Now, the subsequent actual type check of foo(x) will see string | number as the type of x.

@JsonFreeman JsonFreeman commented on the diff Apr 16, 2016
src/compiler/checker.ts
- }
- else if (node.kind === SyntaxKind.TaggedTemplateExpression) {
- links.resolvedSignature = resolveTaggedTemplateExpression(<TaggedTemplateExpression>node, candidatesOutArray);
- }
- else if (node.kind === SyntaxKind.Decorator) {
- links.resolvedSignature = resolveDecorator(<Decorator>node, candidatesOutArray);
- }
- else {
- Debug.fail("Branch in 'getResolvedSignature' should be unreachable.");
- }
- }
- return links.resolvedSignature;
+ function getResolvedOrAnySignature(node: CallLikeExpression) {
+ // If we're already in the process of resolving the given signature, don't resolve again as
+ // that could cause infinite recursion. Instead, return anySignature.
+ return getNodeLinks(node).resolvedSignature === anySignature ? anySignature : getResolvedSignature(node);
@JsonFreeman
JsonFreeman Apr 16, 2016 edited Contributor

Here, we are using anySignature to signal that we are in the middle of a flow analysis. Why not use inFlowCheck? I think I am confused about the difference.

@ahejlsberg
ahejlsberg Apr 16, 2016 Member

No, we're using anySignature to signal that we're in the process of resolving the call signature (for any reason, flow analysis or not). When obtaining contextual types we never want to kick off resolution if we're already in the process of resolving, so we return anySignature in circular scenarios. Effectively there are two circular scenarios interacting here, flow analysis and contextual typing, and only the flow analysis scenario can kick off resolution when resolution is already ongoing.

@JsonFreeman
JsonFreeman Apr 16, 2016 Contributor

Oh ok, got it

@JsonFreeman JsonFreeman and 1 other commented on an outdated diff Apr 16, 2016
src/compiler/checker.ts
@@ -11079,26 +11098,23 @@ namespace ts {
// However, it is possible that either candidatesOutArray was not passed in the first time,
// or that a different candidatesOutArray was passed in. Therefore, we need to redo the work
// to correctly fill the candidatesOutArray.
- if (!links.resolvedSignature || candidatesOutArray) {
- links.resolvedSignature = anySignature;
+ const cached = links.resolvedSignature;
+ if (cached && cached !== anySignature && !candidatesOutArray) {
+ return cached;
+ }
+ links.resolvedSignature = anySignature;
+ const result = resolveSignature(node, candidatesOutArray);
+ // If signature resolution originated in control flow type analysis (for example to compute the
+ // assigned type in a flow assignment) we don't cache the result as it may be based on temporary
+ // types from the control flow analysis.
+ links.resolvedSignature = inFlowCheck ? cached : result;
@JsonFreeman
JsonFreeman Apr 16, 2016 Contributor

Why does assigning cached mean we are not caching, but assigning result means we are caching?

@ahejlsberg
ahejlsberg Apr 16, 2016 Member

The cached variable contains what was already cached, and in flow check scenarios we restore that value instead of recording the result.

@JsonFreeman
JsonFreeman Apr 17, 2016 Contributor

I see, so if the call has never been processed yet, the value of cached will be undefined at this point. We will restore the undefined if we are in a flow analysis. So if we revisit the call later, it will look unprocessed.

One thing that occurs to me is the following. Suppose one of the call arguments is a lambda. In that case, we will type the lambda according to the first visit in the flow analysis. Any lambda parameters we type will stick, and they will permanently reflect the typing of the first pass. To piggy back on your example:

declare function foo<T, U>(s: T, func: (x: T) => U): U;

function f() {
    let x: string | number | boolean;
    x = "";
    while (cond) {
        x; // string | number
        x = foo(x, y => y.length); // Should be an error, string | number has no property length
        x; // number
    }
}

It is essentially the same issue that we see with overloads, where a lambda parameter is typed according to the first overload that binds it. In this case, instead of different overloads, it's different iterations of the loop that pass an argument of a different type.

@JsonFreeman
Contributor

@ahejlsberg I think this is the right compromise.

@saneyuki saneyuki referenced this pull request in karen-irc/karen Apr 18, 2016
Merged

chore(TypeScript): Enable 'strictNullChecks' option #604

@thebanjomatic

I've been playing around with this branch the past couple of days and I've noticed that any doesn't narrow on control-flow assignments. Is this expected behavior?

For example:

let a: any;
a = "10";
a; // a: any ... expected a: string
a.toFixed(2); // no compile error!

We are using noImplicitAny in our codebase, and try to avoid explicit any as much as humanly possible, but it seems like a generally good thing to continue to catch errors when using any (implicitly or explicitly).

@ahejlsberg
Member

@thebanjomatic That is the expected behavior currently. There are simple cases (such as your example) where we know the exact type of a value assigned to an any, but typically we only know parts of the truth. For example:

declare function foo(): { x: number };

let a: any;
a = foo();
let y = a.y;  // Can't error here

It would be a breaking change from the current behavior to give an error above. That said, we're thinking about ways we can do better with any and control flow analysis, but it probably won't be in the initial PR.

ahejlsberg added some commits Apr 18, 2016
@ahejlsberg ahejlsberg Improve expression type caching to ensure consistent results b83dc88
@ahejlsberg ahejlsberg Adding tests 538e22a
@ahejlsberg ahejlsberg Accepting new baselines
b5104cb
@ahejlsberg ahejlsberg Only evaluate assigned type when declared type is a union type 87f55fa
@ahejlsberg ahejlsberg Accepting new baselines
9defdde
@ahejlsberg ahejlsberg typeof x === "function" type guards include Function interface
d28a4fe
@ahejlsberg ahejlsberg Variables from different source files default to their declared type d735b7a
@ahejlsberg ahejlsberg Variables from different module declarations default to their declare…
…d type
c8bf6d8
@ahejlsberg ahejlsberg Support comma operator in type guards ea96dfd
@ahejlsberg ahejlsberg Adding test 33e359f
@ahejlsberg ahejlsberg Accepting new baselines
bab8ef4
@ahejlsberg ahejlsberg Removing unused logic
e9a7d3d
@ahejlsberg ahejlsberg Improve consistency of instanceof and user defined type guards a0101c0
@ahejlsberg ahejlsberg Fix incorrect user defined type guard function in compiler 729dfce
@ahejlsberg ahejlsberg Add regression test 3045cf5
@ahejlsberg ahejlsberg Accepting new baselines
06928b6
@ahejlsberg ahejlsberg Support assignments in truthiness type guards 8a0bc3b
@ahejlsberg ahejlsberg Adding test
d2b89be
ahejlsberg added some commits Apr 21, 2016
@ahejlsberg ahejlsberg Removing unused logic
ab4b039
@ahejlsberg ahejlsberg Correct issue with exported variables in code flow analysis e12b2a7
@ahejlsberg ahejlsberg Update fourslash test
4fb31ac
@ahejlsberg ahejlsberg Only narrow to {} in getNarrowedType when types are completely unrelated f06d3f6
@ahejlsberg ahejlsberg Revert previous change 42e3fc4
@ahejlsberg ahejlsberg Accepting new baselines
0dee5ad
@ahejlsberg ahejlsberg merged commit 5ed6f30 into master Apr 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@basarat basarat added a commit to alm-tools/alm that referenced this pull request Apr 22, 2016
@basarat basarat update ts to latest for Microsoft/TypeScript#8010 🌹 c9448ed
@dokidokivisual dokidokivisual added a commit to karen-irc/karen that referenced this pull request May 4, 2016
@dokidokivisual dokidokivisual Auto merge of #604 - saneyuki:nullable, r=saneyuki
chore(TypeScript): Enable 'strictNullChecks' option

This tries to enable [`--strictNullChecks` option](Microsoft/TypeScript#7140) of TypeScript compiler.

- [Non-nullable types by ahejlsberg · Pull Request #7140 · Microsoft/TypeScript](Microsoft/TypeScript#7140)
  - [Non-strict type checking · Issue #7489 · Microsoft/TypeScript](Microsoft/TypeScript#7489)
  - [[Request for feedback] Nullable types, `null` and `undefined` · Issue #7426 · Microsoft/TypeScript](Microsoft/TypeScript#7426)
- [Control flow based type analysis by ahejlsberg · Pull Request #8010 · Microsoft/TypeScript](Microsoft/TypeScript#8010)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/karen-irc/karen/604)
<!-- Reviewable:end -->
25ed88b
@kristian-puccio

Just trying out typescript now and working out for to get type check actions in redux. Looks like this feature could be very helpful for this?

reactjs/redux#992

@mhegazy mhegazy deleted the controlFlowTypes branch May 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment