Control flow analysis for implicit any variables #11263

ahejlsberg · 2016-09-29T23:07:48Z

This PR introduces control flow analysis for let and var variables that have no type annotation and either no initial value or an initial value of null or undefined.

function f(cond: boolean) {
    let x;
    if (cond) {
        x = "hello";
        x;  // string
    }
    else {
        x = 123;
        x;  // number
    }
    return x;  // string | number
}

function g(cond: boolean) {
    let y = null;
    if (cond) {
        y = "hello";
    }
    return y;  // string | null
}

In the example above, x and y implicitly have declared types of any but control flow analysis can determine their actual types at every reference. Therefore, no errors are reported even when the example is compiled with --noImplicitAny.

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 f(cond: boolean) {
    let x;  // Error: Variable 'x' implicitly has type 'any' in some locations where its type cannot be determined.
    if (cond) {
        x = "hello";
    }
    else {
        x = 123;
    }
    return x;  // type string | number
    function bar() {
        const y = x;  // Error: Variable 'x' implicitly has an 'any' type.
    }
}

In time it is possible that control flow analysis will be able to more accurately determine types of references to outer variables from nested functions in some cases, but given that nested functions are first class objects that can be arbitrarily passed around and called, it is effectively impossible to analyze all such scenarios.

…ializer

…ne type

sandersn

A couple of questions each about the change and the tests, plus a couple of ideas for the tests.

sandersn · 2016-09-30T16:35:31Z

src/compiler/checker.ts

@@ -3050,6 +3051,11 @@ namespace ts {
            return undefined;
        }

+        function isAutoVariableInitializer(initializer: Expression) {
+            const expr = initializer && skipParentheses(initializer);
+            return !expr || expr.kind === SyntaxKind.NullKeyword || expr.kind === SyntaxKind.Identifier && getResolvedSymbol(<Identifier>expr) === undefinedSymbol;


Can this work for [] or {} as well?

Yes, but they each need specialized strategies and I'll cover them in separate PRs.

sandersn · 2016-09-30T16:38:34Z

src/compiler/checker.ts

@@ -8952,7 +8971,7 @@ namespace ts {
                if (isRightSideOfQualifiedNameOrPropertyAccess(location)) {
                    location = location.parent;
                }
-                if (isExpression(location) && !isAssignmentTarget(location)) {
+                if (isPartOfExpression(location) && !isAssignmentTarget(location)) {


What does this change and why? I'm guessing that now we can get the type of a symbol given a child of the symbol's declaration, but I can't guess why.

This is a bug that somehow got introduced during some refactoring for the new emitter. The old isExpression was renamed to isPartOfExpression and a new isExpression was introduced that doesn't look at the context (i.e. doesn't access the parent property).

sandersn · 2016-09-30T16:42:13Z

tests/cases/conformance/statements/for-inStatements/for-inStatements.ts

@@ -14,7 +14,7 @@ for (var x in fn()) { }
 for (var x in /[a-z]/) { }
 for (var x in new Date()) { }

-var c, d, e;
+var c: any, d: any, e: any;


why add any here? Is it to preserve the intent of the tests — does control flow pick a union type for these variables now?

The test just needed some values of type any but they aren't actually meaningful once we start analyzing control flow.

sandersn · 2016-09-30T16:46:09Z

tests/cases/compiler/es5-asyncFunctionTryStatements.ts

@@ -1,10 +1,10 @@
 // @lib: es5,es2015.promise
 // @noEmitHelpers: true
 // @target: ES5
-declare var x, y, z, a, b, c;
+declare var x: any, y: any, z: any, a: any, b: any, c: any;


I thought that auto typing didn't apply to ambient contexts. Why did you have to add any here?

Yeah, you're right about that. But having them doesn't hurt and is a better indication of intent (i.e. we expressly don't want CFA here).

sandersn · 2016-09-30T16:46:54Z

tests/cases/conformance/Symbols/ES5SymbolProperty2.ts

@@ -1,6 +1,6 @@
 //@target: ES5
 module M {
-    var Symbol;
+    var Symbol: any;


I don't understand why auto typing would apply here either

sandersn · 2016-09-30T16:48:35Z

tests/cases/compiler/controlFlowLetVar.ts

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


It would be nice if these functions had names that showed what they were testing.

sandersn · 2016-09-30T16:55:17Z

tests/cases/compiler/controlFlowLetVar.ts

+    if (cond) {
+        x = "hello";
+    }
+    const y = x;


if you have if (cond) .. else then y: string | number, right? Might be nice to have a case for that.

ahejlsberg · 2016-10-03T23:18:17Z

@mhegazy Want to take a look?

* Fix fialing tests after microsoft/TypeScript#11263 * Fix more failures

jods4 · 2016-11-08T23:26:47Z

@ahejlsberg
Reading the descriptions, I have the impression that the following code example won't be a --noImplicitAny error. Am I wrong?

function f(x: number[]): void { ... }

let x = [];  // x: any[]
x.push(3); // x: number[]
f(x); // x is ok according to control flow, so no --noImplicitAny error?

mhegazy · 2016-11-08T23:33:44Z

Am I wrong?

You are correct. this is not flagged as an implicit any error.

jods4 · 2016-11-08T23:44:36Z

@mhegazy this is not safe, though.
Consider that f() might use the x value later (through capture or saving into a global variable) and the code might arbitrarily change x.

function f(x: number[]): void {
  setImmediate(function () { 
    console.log(x.reduce((a, b) => a * b, 1));
  });
}

let x = [];
x.push(3);
f(x);
x.push("I am not a number"); // ok, now x: (string|number)[]

sandersn · 2016-11-09T17:49:18Z

Typescript is already unsafe in this way. You can also write

let sns: (string | number)[];
f(sns);

sandersn · 2016-11-09T18:00:19Z

Never mind, I was completely wrong. It doesn't work.

jods4 · 2016-11-09T23:15:43Z

@sandersn
Yeah, I like that this enables more errors to be caught when migrating (untyped) JS code.
I like that you now can drop more type declarations for local variables as long as you use them correctly.

But I think many people like TS for being (for the most part) statically type-safe and use noImplicitAny to guarantee that no dynamically typed code slips through, i.e. as a way to enforce type safety.
Currently, this PR weakens the guarantees provided by noImplicitAny and allows some unsafe code that was not possible before.

Thus, I would suggest that you limit what is allowed by this feature or (harder) only allow cases that are provably correct (e.g. determine that the dynamic type never changes to an incompatible type).

I can at least imagine the following operations to be potentially unsafe: saving the value in an object, an array or a variable in an outer scope; assigning the value to a setter; passing the value to a function; capturing the value in a closure; calling a member of the value; new-ing the value.

mhegazy · 2016-11-10T00:34:45Z

@jods4, the compiler today does not guard against side effects in multiple places. it is not specific to noImplicitAny checks. for instance:

function f(x: { a: number }) {
    setTimeout(() => {
        x.a.toFixed();
    }, 10);
 }

var x: { kind: "number", a: number } | { kind: "string", a: string };

if (x.kind === "number") { 
    f(x);
}

x.a = "string"; // potentially unsafe, as x has already been captured.

Arrays are just like objects with properties when it comes to narrowing. I do understand your concern, but really this issue is a general design limitation and not specific to this change.

jods4 · 2016-11-10T18:49:38Z

@mhegazy
Yes, I agree.
That's why I said TS is for the most part statically type-safe, which is already a lot 😃

That said, having safety holes doesn't necessarily mean we should add new ones. I guess the question is: do benefits outweight the drawbacks?

Your example is a lot more convoluted than the simple example I gave, which was just:

let x = [];
x.push(3);
f(x);
x.push("NaN");

I guess what annoys me most here is that this is unsafe because I use x in a dynamic way, without declaring my intent, under noImplicitAny. I think the essence of noImplicitAny is to forbid errors from unwanted dynamic code.

My opinion:

This is great for migrating existing JS code and I wouldn't change anything in this context.
I believe people who use noImplicitAny have little use for that feature. Typically those people want maximum safety and try to have everything typed. In this context and with all the inference we already have, you seldom have the need for this shortcut. In my company we build web apps exclusively in this model and I can't say I ever had feature envy for that.

To maximize both safety and value, I would suggest that under noImplicitAny, the dynamic type of locals cannot be changed. I.e.:

let x;  // this is not an error yet.
x = 4;  // so x: number at this point
console.log(x.toFixed());  // sure
x += 5;  // and so on...

let y;  // same thing
y = 4;  // so y: number
y = "Four";  // still an error when --noImplicitAny. Can't use dynamic types without explicit declaration.

ahejlsberg added 13 commits September 20, 2016 13:22

Improve handling of ++ and -- in control flow analysis

38d16bb

Accept new baselines

7df7bb2

Use control flow analysis for let/var with no type annotation or init…

864ee1c

…ializer

Unify anyType and autoType when checking for identical declarations

dda8aac

Update tests

b14b24a

Accept new baselines

c37b790

Merge branch 'master' into controlFlowLetVar

bd19f1d

Control flow typing for variables with null or undefined initializer

0621f7e

Accept new baselines

52d2aa5

Report error on both declaration and reference when CFA can't determi…

4479013

…ne type

Accept new baselines

1c39cdc

Add tests

d8ec857

Accept new baselines

0739611

msftclas added the cla-not-required label Sep 29, 2016

Fix linting issue

6ae5337

sandersn requested changes Sep 30, 2016

View reviewed changes

ahejlsberg added 2 commits October 3, 2016 09:01

Add comments to tests

6c2344e

Update tests and baselines

dd63c46

sandersn approved these changes Oct 4, 2016

View reviewed changes

ahejlsberg mentioned this pull request Oct 6, 2016

Control flow analysis for array construction #11432

Merged

ahejlsberg merged commit 44c475f into master Oct 6, 2016

ahejlsberg deleted the controlFlowLetVar branch October 6, 2016 21:23

mhegazy added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Oct 7, 2016

Fix fialing tests after microsoft/TypeScript#11263

a73da6a

mhegazy added a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Oct 7, 2016

Fix failing tests after microsoft/TypeScript#11263 (#11835)

13dec92

* Fix fialing tests after microsoft/TypeScript#11263 * Fix more failures

ahejlsberg mentioned this pull request Nov 2, 2016

No error when using uninitialized value in noImplicitAny & strictNullChecks mode #12002

Closed

ahejlsberg mentioned this pull request Nov 16, 2016

Compiler should allow assignment of new property in empty object #11839

Closed

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

HerringtonDarkholme mentioned this pull request Sep 22, 2017

Config noImplicitAny seems to be not working since typescript 2.1 #18679

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 implicit any variables #11263

Control flow analysis for implicit any variables #11263

ahejlsberg commented Sep 29, 2016 •

edited

Loading

sandersn left a comment

sandersn Sep 30, 2016

ahejlsberg Sep 30, 2016

sandersn Sep 30, 2016

ahejlsberg Sep 30, 2016

sandersn Sep 30, 2016

ahejlsberg Sep 30, 2016

sandersn Sep 30, 2016

ahejlsberg Sep 30, 2016

sandersn Sep 30, 2016

ahejlsberg Sep 30, 2016

sandersn Sep 30, 2016

sandersn Sep 30, 2016

ahejlsberg commented Oct 3, 2016

jods4 commented Nov 8, 2016

mhegazy commented Nov 8, 2016

jods4 commented Nov 8, 2016

sandersn commented Nov 9, 2016

sandersn commented Nov 9, 2016

jods4 commented Nov 9, 2016

mhegazy commented Nov 10, 2016

jods4 commented Nov 10, 2016

Control flow analysis for implicit any variables #11263

Control flow analysis for implicit any variables #11263

Conversation

ahejlsberg commented Sep 29, 2016 • edited Loading

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

Choose a reason for hiding this comment

ahejlsberg commented Oct 3, 2016

jods4 commented Nov 8, 2016

mhegazy commented Nov 8, 2016

jods4 commented Nov 8, 2016

sandersn commented Nov 9, 2016

sandersn commented Nov 9, 2016

jods4 commented Nov 9, 2016

mhegazy commented Nov 10, 2016

jods4 commented Nov 10, 2016

ahejlsberg commented Sep 29, 2016 •

edited

Loading