Variable isn't narrowed within a capturing closure

[felixge]

TypeScript 3.7.2
Playground link

Compiler Options:

{
  "compilerOptions": {
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "useDefineForClassFields": false,
    "alwaysStrict": true,
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    "downlevelIteration": false,
    "noEmitHelpers": false,
    "noLib": false,
    "noStrictGenericChecks": false,
    "noUnusedLocals": false,
    "noUnusedParameters": false,
    "esModuleInterop": true,
    "preserveConstEnums": false,
    "removeComments": false,
    "skipLibCheck": false,
    "checkJs": false,
    "allowJs": false,
    "experimentalDecorators": false,
    "emitDecoratorMetadata": false,
    "target": "ES2017",
    "module": "ESNext"
  }
}

Input:

let i: number | undefined;
i = 0;
let j:number = i+1; // works
(k: number) => k === i+1; // error: Object i is possibly undefined

Output:

"use strict";
let i;
i = 0;
let j = i + 1; // works
(k) => k === i + 1; // error: Object i is possibly undefined

Expected behavior:

The compiler should not complain about the last i+1 because it's clearly a number type after assigning 0 to it.

I suspect the closure uses the type of i from the let statement and ignores it being narrowed down later on. Is this expected behavior?

[j-oliveras]

Duplicate of #9998.

Typescript warns you that the i can be undefined when the function is called:

let i: number | undefined;
i = 0;
let j:number = i+1; // works
const func = (k: number) => k === i + 1; // error: Object i is possibly undefined
i = undefined;
func(3);

Playground.

[fatcerberus]

The more I see this issue come up, the more I wonder if it’s possible to do better. The analyzer can’t look into function calls, but would it be possible to notice that the variable is not accessed after the closure is created, so the last narrowing can be maintained? Seems like it should be easy enough to determine that syntactically, since the function is already inline.

If the function modifies the variable itself, that’s trickier, but that problem seems similar to loop analysis on the surface.

[felixge]

Jordi Oliveras Rovira (@j-oliveras) which section of the linked issue are you referring to? I'm okay with closing this as a dupe if it's covered by the other issue, but I couldn't find my case on first glance.

Anyway, I understand your point. I was hoping the compiler would realize that i doesn't get modified after the assignment of 0 as Bruce Pascoe (@fatcerberus) suggests. But if that's not in the cards, I can easily deal with it.

[fatcerberus]

This is kind of the inverse of #9998, but not a direct dupe I don’t think. #9998 is about what happens with local narrowings when you call a function (answer: nothing, the compiler is optimistic and assumes no side effects); the issue here is about what happens to narrowings inside a function that closes over the narrowed variable (answer: narrowings are discarded at function boundaries, with the exception of IIFEs).

This particular issue has come up many times in the past though.

[jcalz]

I don't think there's anything better than the current behavior in this case.

I assume the suggestion isn't: "uncallable functions should have no type checking inside their implementation" (the arrow function in the example code is uncallable because it's neither immediately invoked nor assigned to anything)

So, assuming the function is meant to be callable, what better could be done here? i isn't const, so as pointed out by Jordi Oliveras Rovira (@j-oliveras), one could do i = undefined and call the function for a nice runtime error. Is the suggestion: "we should suppress a warning if the situation warned about doesn't happen to occur in the actual code"? Like this:

function foo(i: number | undefined) {
    i + 1; // no error! foo() is only called safely
}
foo(1);
foo(2);
// foo(undefined); // commented out

and then if you uncomment foo(undefined), an error then appears inside the foo() implementation? That's some spooky action at a distance. You'd only ever get warnings if you wrote tests I guess.

Since that doesn't make much sense, I'm guessing there's some other suggestion being made here, but I'm not seeing it. IIFEs are already inlined because it's one of the few places we can say that the unsafe situation is known to be impossible and not just fortuitously avoided. What better behavior are we hoping for here?

[fatcerberus]

So, assuming the function is meant to be callable, what better could be done here? i isn't const, so as pointed out by Jordi Oliveras Rovira (@j-oliveras), one could do i = undefined and call the function for a nice runtime error.

If i is a local variable and the outer function returns before i is reassigned (which normal CFA can already tell us), and it's not modified inside the closure, then we already know, a priori--just by looking at the AST--that the variable can't possibly be modified further. If it's modified inside the closure, then the same challenges apply as for loop analysis I would think.

[felixge]

assuming the function is meant to be callable

Yes, it's meant to be callable. I just omitted that from my example for the sake of brevity.

[fatcerberus]

function foo(i: number | undefined) {
    i + 1; // no error! foo() is only called safely
}
foo(1);
foo(2);
// foo(undefined); // commented out

I would expect this to error regardless of my proposed behavior, because undefined is potentially being added to a number. Also, there's no closure involved in that example.

[jcalz]

If Bruce Pascoe (@fatcerberus) is accurately describing the problem, can we change the example code to something that demonstrates the relevant issue? Like, maybe this:

function foo() {
    let i: number | undefined;
    i = 0;
    return (k: number) => k === i + 1; // error that i is possibly undefined 
}
console.log(foo()(1)); // true
console.log(foo()(0)); // false

[RyanCavanaugh]

If I had to draw the most realistic picture of a warning on a closed-over variable that really shouldn't exist, it'd look like this:

function makeAdder(n?: number) {
  n = n ?? 0;
  return (m: number) => n + m;
}

It seems like the rule is "If a closure is lexically after all assignments to a bare identifier, then the last narrowing can apply".

You can break this, though:

function adderWrapper(n?: number) {
  function setValue(v?: number) {
    n = v;
  }
  n = n ?? 0;
  return {
    setValue,
    add: (m: number) => m + n
  }
}
const k = adderWrapper(0);
k.setValue(undefined);
k.add(3); // NaN

So the rule would need to be amended to "All assignments to a bare identifier occur lexically and CFA-before the closure expression", though possibly that could be broken too.

[fatcerberus]

Ryan Cavanaugh (@RyanCavanaugh) nailed what I was shooting for. From my outsider perspective, it definitely seems like this could be enabled using a combination of existing lexical and CFA data, though it would be interesting to explore the edge cases, if any indeed exist.