TypeScript does not expect variable change in forEach

[pallada-92]

TypeScript Version: 2.4.1

Code

// strictNullChecks should be on
let a: number | null = null;
[1].forEach(() => { a = 1; });
if (a !== null) {
    console.log(a + 1);
}

Expected behavior:
Output '2'.

Actual behavior:
error TS2365: Operator '+' cannot be applied to types 'never' and '1'.

Actually, after the forEach loop the inferred type of 'a' remains to be 'null', although it may change during the loop.

[kitsonk]

Yeah, that isn't good. It is likely related to #11498 which is the opposite problem, where the guards get reset in the callback scope, which occurs here. In this case, CFA should reset the guards in the outer scope, or allow some sort of annotation that the forEach is an in-turn callback.

[ahejlsberg]

This is working as intended. In general, the compiler can't be certain that a callback is actually called and therefore it can't assume that the effects of a callback have happened. Indeed, for an empty array the callback wouldn't be called. The type system would effectively have to make a distinction between empty and non-empty arrays which isn't something we track (nor do I think it is feasible to do so).

[WuTheFWasThat]

but IMO the compiler doesn't need to be certain that a callback is called. if anything, it should be sure that it wasn't called, before deciding that a is never null

[AHelper]

Just ran into this myself, although without null checks but instead with enums. I have the same kind of view as WuTheFWasThat.

@ahejlsberg: How can the compiler safely assume the implementation of any function to assert that the function will not execute the callback before returning? That's a dangerous (and as exemplified, potentially incorrect) assumption to make. The safe approach is to assume that all callbacks/closures will be called unless proven otherwise.

A bit more though on this:

// Assuming strict null checks
function test1() {
  let a: number | null = null;
  (() => a = 1)();
  // Closure called, type of a is inferred to be number
}
function test2() {
  let a: number | null = null;
  const c = () => a = 1;
  // Can be certain c is never called, type unchanged (null)
}
function test3() {
  let a: number | null = null;
  const c = () => a = 1;
  c();
  // c is called, type of a is inferred to be number, differs from current implementation
}
let globalC: () => void;
function test4() {
  let a: number | string | null = null;
  const c = () => a = 1;
  globalC = c;
  someFunctionWhichMayCallGlobalCSynchronously();
  // c is not completely under control of function, may be called
  // type should be number | null (union of types with and without execution), differs from current implementation
}

[RyanCavanaugh]

The safe approach is to assume that all callbacks/closures will be called unless proven otherwise.

Neither approach is more "safe" than the other, it's simply a matter of which cases get incorrectly identified. You can find thousands of confused people on Stack Overflow writing code like this:

let s;
http.get('some url', (err, data) => s = data);
console.log(s); // Why is s undefined?!?!?!

This is all discussed at length at #11498 already.

[WuTheFWasThat]

I agree that usability is an issue with either behavior. But it seems like the compiler should never infer the never type for values which quite simply can exist - that breaks the symmetry between the two approaches, right?

Another way to see it - in your example with http, the user would be confused in javascript as well, and at run time, s is in fact undefined. But in pallada's example, someone with perfect understanding of javascript could write that analagous code, port it to typescript, and then run into a compiler error

Edit: #11498 would address this issue if it were fixed. but since that proposal could be involved to implement, I think this behavior might be easier to fix by relaxing to assume callbacks are ran. although maybe that has problems too, that I'm not seeing. and this looks like a dup of #12176; there wasn't much discussion there though

[RyanCavanaugh]

never is irrelevant. The example is isomorphic to this, which doesn't involve never:

let a: number | string = "hello";
[1].forEach(() => { a = 1; });
if (a === 1) { // should be OK but isn't
    console.log(a + 1);
}

[IEvangelist]

As it pertains to the flow control analysis, I agree with others on this. The compiler should be capable of determining whether or not a function was invoked, i.e.; the callback to array.forEach vs http.get are far different and well known (as was already highlighted above).

With the invocation to forEach we know that it is executed immediately, but only conditionally. If we were to do the following the compiler couldn't possibly know or make an inference as the array size is now known:

let a: number | string = "hello";
[].forEach(() => a = 1);

// What do you expect 'a' to be?

I'm on the fence and can see both perspectives.

[WuTheFWasThat]

@RyanCavanaugh If it isn't to do with never, I think this is how i would generalize my statement: The typescript compiler should always infer a type which is a supertype of the real runtime type, and never infer a type which is a subset of the real type. Is that reasonable?

[RyanCavanaugh]

I think that's a correct goal.

For this issue specifically, any behavioral change would be a breaking change, so if we do change the behavior it would have to be in the context of doing it "correctly" (distinguishing conditional / unconditional and immediate / deferred) so that the only people getting new errors would be those with nonworking code.

[WuTheFWasThat]

got it, thanks for the explanation. looking forward to #11498 being resolved!