Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Narrow const variables in function declarations #8976

Closed
mhegazy opened this issue Jun 5, 2016 · 7 comments
Closed

Narrow const variables in function declarations #8976

mhegazy opened this issue Jun 5, 2016 · 7 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2016

With #8849, control flow analysis happens thorough function expression and lambda boundaries for const variables.

it should also be applied to function declarations, class expressions, and class declarations, etc..

so today this works:

const x: number | number[] = []

var cb = function () {
    x.push(1)
}

but not this:

const x: number | number[] = []

function cb() {
    x.push(1)
}
@mhegazy mhegazy added the Bug A bug in TypeScript label Jun 5, 2016
@ahejlsberg
Copy link
Member

ahejlsberg commented Jun 6, 2016

Declared functions are hoisted so we can't actually be certain the initialization has occurred.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 6, 2016

True, but it is likely to run into a runtime error if the hoisted function is called:

cb();

const x = [];

function cb() {
  x.push(1); // Uncaught ReferenceError: x is not defined
}

In theory, static analysis could have flagged the issue, but I can't see any additional problems it would introduce.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 6, 2016

Also var cb = function () is hoisted too, just as undefined though (and TypeScript 1.8 does not catch it (not sure about @next)):

cb(); // TypeError: cb is not a function

const x = [];

var cb = function() {
  x.push(1);
}

@ahejlsberg
Copy link
Member

@kitsonk No, var cb = function() isn't hoisted and in your second example the checker reports an error on the cb(); call because cb is used before being assigned.

My point about hoisting is this: Since a hoisted function is defined (i.e. non-undefined) before any code executes in a particular file, it doesn't make sense to consider where in the control flow a function is declared. The behavior should be the same no matter where in a file a function declaration is placed.

Part of the problem here is that we're debating a fairly meaningless example:

const x: number | number[] = [];  // Why this annotation when x can't be reassigned?
function cb() {
    x.push(1);  // Error
}

Here's something more meaningful:

const x: number[] = [];
function cb() {
    x.push(1);  // Ok
}

Since cb is hoisted it could be called from anywhere, including before the const declaration. So, technically we should consider x to be possibly undefined and require a type guard for x on entry to every declared function. However, that ends up being annoyingly strict, so instead we trust that x has been initialized on entry to every declared function--which means, we consider x to have its declared type upon entry to cb. Therefore, no error above. Now, if you change the declared type to be number | number[] you're telling us to consider that to be the starting type of x. So now a type guard is required.

In the example with a function expression

const x: number | number[] = [];
var cb = function() {
    x.push(1);  // Ok
}

we know from control flow analysis that x for sure has been initialized before cb could be called, so we use the narrowed type of x. That seems reasonable, but we can't apply that same analysis do a declared function.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 7, 2016

No, var cb = function() isn't hoisted and in your second example the checker reports an error on the cb(); call because cb is used before being assigned.

Not to argue (especially with you), but that is why I linked to the Playground. Because it is a var, the checker doesn't seem to catch it, and likely rightly so. There is no design time error. If it is changed to let, it properly reports that it is used before declared. Which is the whole point of why, before we had let and const we used to put all of our vars at the top of the function scope to avoid those sort of silly pitfalls.

But I understand the rest of your point in that because functions and their values are hoisted, it makes reliable flow control analysis impossible, where as in variable assignment, the value is not accessible until that point in the code.

@ahejlsberg
Copy link
Member

Ah, sorry for the confusion. What I meant was that the initialization of cb isn't hoisted (but you already knew that) and the control flow analyzer (in --strictNullChecks mode) reports an error on the cb(); call. But that is of course an @next feature only.

@mhegazy mhegazy added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Bug A bug in TypeScript labels Jun 7, 2016
@mhegazy
Copy link
Contributor Author

mhegazy commented Jun 7, 2016

one thing to note, with closing this we chose to ignore the cases of block scoped function declarations (i.e. ES6 in strict mode function declarations):

"use strict";

let x: number | number[];

if (isArray(x)) {
    function foo() {
         x.push(1) // Error
    }
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants