Control flow analysis for array construction #11432

ahejlsberg · 2016-10-06T21:20:53Z

This PR introduces control flow analysis for array construction patterns that originate in an empty array literal (x = []) followed by some number of x.push(value), x.unshift(value) or x[n] = value operations.

A let, const, or var variable declared with no type annotation and an initial value of [] is considered an implicit any[] variable. When an implicit any variable (see #11263) or implicit any[] variable is assigned an empty array literal [], each following x.push(value), x.unshift(value) or x[n] = value operation evolves the type of the variable in accordance with what elements are added to it.

function f1() {
    let x = [];
    x[0] = 5;
    x[1] = "hello";
    x[2] = true;
    return x;  // (string | number | boolean)[]
}

function f2() {
    let x = [];
    x.push(5);
    x.push("hello");
    x.push(true);
    return x;  // (string | number | boolean)[]
}

function f3() {
    let x = null;
    if (cond()) {
        x = [];
        while (cond()) {
            x.push("hello");
        }
    }
    return x;  // string[] | null
}

An array type is evolved only by operations on the variable in which the evolving array type originated. When an evolving array type variable is referenced in an expression, its type is "fixed" and cannot be further evolved.

function f4() {
    let x = [];       // x has evolving array type
    x.push(5);
    let y = x;        // y has fixed array type
    x.push("hello");  // Ok
    y.push("hello");  // Error
}

Similar to implicit any variables, control flow analysis is unable to determine the actual types of implicit any[] variables when they are referenced in nested functions. In such cases, the variables will appear to have type any[] in the nested functions and errors will be reported for the references if --noImplicitAny is enabled.

function f3() {
    let x = [];  // Error: Variable 'x' implicitly has type 'any[]' in some locations where its type cannot be determined.
    x.push(5);
    function g() {
        x;    // Error: Variable 'x' implicitly has an 'any[]' type.
    }
}

zenmumbler · 2016-10-11T21:08:44Z

Will this affect a problem (#11082) I submitted earlier? This sounds like its different enough from my problem but wanted to check if there is any overlap, thanks!

sandersn · 2016-10-11T21:51:44Z

src/compiler/core.ts

            }
        }
        return result;
    }

+    // Maps from T to T and avoids allocation of all elements map to themselves


typo: of → if

ahejlsberg · 2016-10-12T01:22:06Z

@zenmumbler No, this PR doesn't affect the issue in #11082.

ahejlsberg · 2016-10-12T17:55:04Z

@mhegazy Want to take a look before I merge this?

sandersn

Besides a few small changes, I also want to make sure we're OK with less localised errors in a series of functions with no return type annotations. In the example below, the error moves from the line with 'oops' on it to the last line.

function f() {
  let x = [];
  x.push(12);
  x.push('oops');
  return x;
}
function g() {
  let a = g();
  a.push('oops 2'); // shouldn't be allowed
  return a; // g now has type (string | number)[]
}
let sum = 0;
for (const n of g()) {
  sum += n; // error, can't add (string | number) to number
}

Right now, I think I'm on the side of fewer annotations, but I think we should know about the pitfalls of the new behaviour.

sandersn · 2016-10-11T22:30:26Z

tests/baselines/reference/implicitAnyWidenToAny.errors.txt



-==== tests/cases/compiler/implicitAnyWidenToAny.ts (2 errors) ====
+==== tests/cases/compiler/implicitAnyWidenToAny.ts (1 errors) ====
    // these should be errors
    var x = null;                        // error at "x"
    var x1 = undefined;		             // error at "x1"
    var widenArray = [null, undefined];  // error at "widenArray"
        ~~~~~~~~~~
 !!! error TS7005: Variable 'widenArray' implicitly has an 'any[]' type.
    var emptyArray = [];		         // error at "emptyArray"


Delete comment and consider deleting the entire line.

Yes, will fix.

sandersn · 2016-10-12T17:20:04Z

tests/cases/compiler/controlFlowArrays.ts

+
+declare function cond(): boolean;
+
+function f1() {


It would be nice to have meaningful names for these test cases

I think we're fine with the short names.

sandersn · 2016-10-12T17:29:37Z

tests/cases/compiler/controlFlowArrays.ts

+    x[1] = "hello";
+    x[2] = true;
+    return x;  // (string | number | boolean)[]
+}


what about a test case that case that does x[0] = 5; x[0] = "hello"; x[0] = true? It would still return (string | number | boolean)[], right?

Yes, you'd get the same type. We don't look at the value of the index expression, so nothing gained by an additional test.

sandersn · 2016-10-12T17:41:28Z

src/compiler/binder.ts

@@ -1200,6 +1215,12 @@ namespace ts {
            else {
                forEachChild(node, bind);
            }
+            if (node.expression.kind === SyntaxKind.PropertyAccessExpression) {
+                const propertyAccess = <PropertyAccessExpression>node.expression;
+                if (isNarrowableOperand(propertyAccess.expression) && propertyAccess.name.text === "push") {


what about unshift? I used unshift briefly when writing spread types. :)

Yup, we should support unshift, uncommon as it may be.

sandersn · 2016-10-12T17:45:16Z

src/compiler/checker.ts

@@ -8391,6 +8404,11 @@ namespace ts {
                getAssignedType(<Expression>node);
        }

+        function isEmptyArrayAssignment(node: VariableDeclaration | BindingElement | Expression) {
+            return node.kind === SyntaxKind.VariableDeclaration && (<VariableDeclaration>node).initializer && isEmptyArrayLiteral((<VariableDeclaration>node).initializer) ||


can you break these lines up a bit more? They are really hard to read on github.

sandersn · 2016-10-12T17:49:50Z

src/compiler/checker.ts

@@ -8469,21 +8495,115 @@ namespace ts {
            return incomplete ? { flags: 0, type } : type;
        }

+        // An evolving array type tracks the element types that have so far been seen in an


comment should be jsdoc formatted

sandersn · 2016-10-12T17:51:22Z

src/compiler/types.ts

@@ -2748,6 +2756,8 @@ namespace ts {
    export interface AnonymousType extends ObjectType {
        target?: AnonymousType;  // Instantiation target
        mapper?: TypeMapper;     // Instantiation mapper
+        elementType?: Type;      // Element expressions of evolving array type


why isn't AutoArrayType a new subtype of AnonymousType? I think it would make it easier to track the property that auto array types don't escape the dynamic scope of getFlowTypeOfReference.

I don't follow. autoArrayType is the declared type of an auto-inferred any[] and it's already used outside of getFlowTypeOfReference.

It's separate from autoArrayType, whose type is actually TypeReference.

I'm talking about the comment at the top of the new code that says

Evolving array types are ultimately converted into manifest array types
and never escape the getFlowTypeOfReference function."

I was suggesting something like:

export interface AutoArrayType extends AnonymousType { elementType?: Type; finalArrayType?: Type; }

And then having the new code that returns AnonymousType today return AutoArrayType instead.

Oh, I see. The issue with doing it that way is that we don't have a TypeFlags.EvolvingArrayType that would indicate an evolving array type (because we're out of flag bits). Instead, we distinguish by looking for the elementType property on AnonymousType, so it has to be part of AnonymousType.

Ok, that makes sense.

sandersn · 2016-10-12T18:00:14Z

src/compiler/checker.ts

+                getUnionType(sameMap(types, finalizeEvolvingArrayType), subtypeReduction);
+        }
+
+        // Return true if the given node is 'x' in an 'x.push(value)' operation.


should support unshift here too

sandersn · 2016-10-12T18:05:17Z

src/compiler/checker.ts

            visitedFlowCount = visitedFlowStart;
-            if (reference.parent.kind === SyntaxKind.NonNullExpression && getTypeWithFacts(result, TypeFacts.NEUndefinedOrNull).flags & TypeFlags.Never) {
+            // When the reference is 'x' in an 'x.push(value)' or 'x[n] = value' operation, we give type
+            // 'any[]' to 'x' instead of using the type determed by control flow analysis such that new


typo:determined

ahejlsberg · 2016-10-13T20:57:14Z

With the latest commits, when an evolving array has no x.push(value), x.unshift(value) or x[n] = value operations in any control flow path leading to the current location, its type is implicitly any[]. Thus, any reference to such an empty evolving array will produce an error with --noImplicitAny:

function f1() {
    let a = [];
    if (cond()) {
        a.push("hello");
    }
    a;  // string[]
}

function f2() {
    let a = [];
    a;  // any[], error with --noImplicitAny
}

Note that references to the length property are always allowed since they don't depend on the element type of the array:

function f3() {
    let a = [];
    while (a.length < 10) {
        a.push("test");
    }
    return a;  // string[]
}

ahejlsberg · 2016-10-13T20:57:47Z

@mhegazy Latest commits should solve the issue we discussed yesterday.

# Conflicts: # src/compiler/checker.ts

ahejlsberg added 6 commits October 6, 2016 11:04

Use control flow analysis for variables initialized with []

9ca2569

Accept new baselines

b5c1055

Introduce sameMap function

9c4190b

Accept new baselines

144f4ff

Add tests

a9c2b23

Accept new baselines

c0c2271

msftclas added the cla-not-required label Oct 6, 2016

ahejlsberg added 6 commits October 6, 2016 14:36

Merge branch 'master' into controlFlowArrays

b8d5226

Fix some minor issues

12906a7

Add additional test

2f5af2e

Fix issue in control flow analysis of nested loops

71b9b33

Add test

d202b1c

Accept new baselines

89826a5

DanielRosenwasser mentioned this pull request Oct 10, 2016

Design Meeting Notes, 10/10/2016 #11505

Closed

ahejlsberg added 4 commits October 11, 2016 10:14

Support parentheses and comma operator with evolving arrays

e6b588a

Fix minor issue

612ed1e

Add test

e9858de

Accept new baselines

c9e2f95

sandersn reviewed Oct 11, 2016

View reviewed changes

Fix typo

38278ee

sandersn requested changes Oct 12, 2016

View reviewed changes

ahejlsberg added 3 commits October 13, 2016 09:43

Support 'unshift' and fix typo

1dedca7

Fix test

620b3f9

Add additional tests

a27a68f

sandersn approved these changes Oct 13, 2016

View reviewed changes

ahejlsberg added 2 commits October 13, 2016 13:28

Treat reference to empty evolving array as an implicit any[]

bf301e9

Update tests

bfa4197

Accept new baselines

79ed3a7

Merge branch 'master' into controlFlowArrays

c876d92

# Conflicts: # src/compiler/checker.ts

ahejlsberg merged commit b5d1e4c into master Oct 14, 2016

ahejlsberg deleted the controlFlowArrays branch October 14, 2016 23:28

zhuravlik mentioned this pull request Jan 31, 2017

Control flow analysis for implicit any and array initialization works only when --noImplicitAny is on #13779

Closed

ghost mentioned this pull request Jun 2, 2017

Infer variable type form return type #16218

Closed

ahejlsberg mentioned this pull request Aug 16, 2017

Some code written in strict mode is invalid in non-strict mode #17774

Closed

ahejlsberg mentioned this pull request Sep 5, 2017

Error on excessively large control flow graphs #18246

Merged

ahejlsberg mentioned this pull request Oct 23, 2017

implicitAny doesn't work for 2.5.2 #19416

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control flow analysis for array construction #11432

Control flow analysis for array construction #11432

ahejlsberg commented Oct 6, 2016 •

edited

Loading

zenmumbler commented Oct 11, 2016

sandersn Oct 11, 2016

ahejlsberg commented Oct 12, 2016

ahejlsberg commented Oct 12, 2016

sandersn left a comment

sandersn Oct 11, 2016

ahejlsberg Oct 13, 2016

sandersn Oct 12, 2016

ahejlsberg Oct 13, 2016

sandersn Oct 12, 2016

ahejlsberg Oct 13, 2016

sandersn Oct 12, 2016

ahejlsberg Oct 13, 2016

sandersn Oct 12, 2016

ahejlsberg Oct 13, 2016

sandersn Oct 12, 2016

sandersn Oct 12, 2016 •

edited

Loading

ahejlsberg Oct 13, 2016

sandersn Oct 13, 2016

ahejlsberg Oct 13, 2016

sandersn Oct 13, 2016

sandersn Oct 12, 2016

sandersn Oct 12, 2016

ahejlsberg commented Oct 13, 2016

ahejlsberg commented Oct 13, 2016

Control flow analysis for array construction #11432

Control flow analysis for array construction #11432

Conversation

ahejlsberg commented Oct 6, 2016 • edited Loading

zenmumbler commented Oct 11, 2016

Choose a reason for hiding this comment

ahejlsberg commented Oct 12, 2016

ahejlsberg commented Oct 12, 2016

sandersn left a 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

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

Choose a reason for hiding this comment

sandersn Oct 12, 2016 • edited Loading

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

Choose a reason for hiding this comment

ahejlsberg commented Oct 13, 2016

ahejlsberg commented Oct 13, 2016

ahejlsberg commented Oct 6, 2016 •

edited

Loading

sandersn Oct 12, 2016 •

edited

Loading