Aggressive "used before assigned" with strictNullChecks

[kitsonk]

TypeScript Version: 2.0.0 (beta)

Code

// @strictNullChecks: true

function on(event: string, callback: () => void): { destroy(): void; } {
    return {
        destroy() {
            /* nothing */
        }
    };
}

const handle = on('example', () => {
    handle.destroy(); // error TS2454: Variable 'handle' is used before being assigned.
});

Expected behavior:

Compile without errors.

Actual behavior:

Error that handle is being used before it is assigned.

It seems to be overly aggressive behaviour which means using block scoped variables in a callback is causing usability issues. Shouldn't flow control assumed that functions would be resolved after the function returns a value?

Seems to be a little bit related to #9382, but in this case, the type is resolvable (and handle.destroy() provides insight. I also noticed that in VSCode, while other compiler errors are displaying, this one isn't getting flagged in the IDE (that may still just be my setup though).

[RyanCavanaugh]

This error is at least as correct as any other check involving a closure. Consider if you had written this:

const arr = [1, 2, 3].map(x => x + arr.length);

[kitsonk]

I see your point. TypeScript often though errs on the side of usability though. It would come down to a judgement of is your example a common error, or is it usually intentional and safe?

Another example of where it could be considered overly aggressive:

const handle = setInterval(() => clearInterval(handle), 1000);

Slightly non-sensical, but could easily be something more complex that for example fires n number of times before clearing.

We had the pattern I showed above quite a lot for self removing event handlers under certain conditions. When we (@agubler) tried to migrate strictNullChecks there is no reasonable workaround except trying to trick the compiler by doing something like:

let handle: { destroy?: () => void; } = {};
handle = on('event', () => handle.remove());

Which just seems a bit silly. You can't even fix it like this:

const handle = on('event', () => handle && handle.remove());

The compiler gets hung up on that it is used before it is assigned. I would be fine if it contextually inferred Handle | undefined and we could guard against that, but that currently doesn't appear to be an option.

[RyanCavanaugh]

I see what you mean and agree, this should not be an error, especially given the lack of good workarounds.

The type inside the function should be Handle (not Handle | undefined). It's an early TDZ error if the callback executes immediately so the undefined value couldn't be observed anyway.

[kitsonk]

Wow, you are right and to be honest, I had sort of understood the concept of the temporal dead zone but hadn't actually run into it. But yeah, if the callback invokes immediately you can't guard yourself against it, because any reference to the variable causes Identifier 'a' has already been declared error at runtime. Experience something new every day.

I guess there is no way to statically identify that without some additional notation that a closure being passed as an argument is not immediately invoked.

[Arnavion]

Heh, this is basically the dual of that other issue where narrowed types widen inside a closure because TS doesn't know the closure is immediately invoked.

[sledorze]

I think it should be an error and the user to write defensive code.
Using some syntactic sugar would help however.. (Feature not implemented yet - not been accepted yet as a next javascript feature so..)

const handle = on('example', () => {
handle?.destroy();
});

The type check path to solve this would involve tracking effects, I'm not sure this is within the scope of typescript.

[kitsonk]

@sledorze your safe navigation was essentially proposed by Ryan in #16. Eventually it was considered out of scope. Even if were delivered, there is no safe emit that gets around the temporal dead zone issue if the callback were to be invoked and then if there is no temporal dead zone issue, then there is no need to guard against that safe operation.

Heh, this is basically the dual of that other issue where narrowed types widen inside a closure because TS doesn't know the closure is immediately invoked.

Another valid point. Though addressing this wouldn't necessarily change the behaviour of the type widening. I can't find it now though, but I thought Anders mentioned that the team were considering some change in behaviour there too, because some of the behaviours were surprising.

The root cause is that callbacks are of two main classes of immediate or not and there is no way the compiler can statically infer that and there is no way of asserting that either. Any syntactic solution would be challenging, because it may very well conflict and be confusing. Especially because instead of describing the shape of the parameter, you would be describing what is going to happen with that parameter.

[sledorze]

@kitsonk what do you mean by 'safe emit' ? (Trying to understand the meaning not to disagree)

[Artazor]

@kitsonk

The root cause is that callbacks are of two main classes of immediate or not and there is no way the compiler can statically infer that and there is no way of asserting that either. Any syntactic solution would be challenging, because it may very well conflict and be confusing. Especially because instead of describing the shape of the parameter, you would be describing what is going to happen with that parameter.

Just want to speculate on this:

Imagine the type constructor deferred. It could be used only with functional types, or types constrained to be functions.

assignability rules:

var f: deferred () => void;
var g: () => void;
f = g; // OK
g = f; // Error

and there is only one inference and type checking rule (coupled with control flow analysis):

• if defered function f is lexically called inside other function g, then g should be inferred as deferred, if g already has non-deferred signature then CompileTime error.

All functions calling their callbacks asynchronously should simply mark their parameters as deferred. e.g:

declare function setTimeout(fn: deferred () => void, ms: number): number;

Of course deferred it is idempotent. However, I'd allow deferred types only in field declarations and formal parameters of functions, and prohibit as standalone types (like parameters for generics, the destination of aliases, etc.). It could be done via transposing the deferred modifier to the name/field position i.e. instead of writing

var a: deferred () => void;
var someFn: (cb: deferred () => void);
var someOther: (cbs: { ok: deferred () => void, fail: deferred (e) => void })

we could write:

var deferred a: () => void;
var someFn: (deferred cb: () => void);
var someOther: (cbs: { deferred ok: () => void, deferred fail: (e) => void })

And the later case I'd make the only way to express "defered-ness" (my intuition says that it shoud be so). But under the hood it is essentialy a type modifier.

Are there any contradictions?

[Artazor]

Need to add a rule

• Top level functions and class member functions by default should not be inferred as deferred (should raise an error at compile time)

function f(deferred cb:(e, res) => void) {
     function inner() { // inferred as `deferred () => void` (culprit is cb)
            return cb(null, 123);
     }
     setTimeout(inner, 0); // ok;
     inner(); // Error: f is top-level function, can't make it deferred.
}

NB! deferred modifier encodes the obligation to run the function not earlier than in the next event loop cycle. This is much stronger property than the obligation not to run something within the current function (which could not be encoded in such a simple manner). Being defined in this way deferred perfectly composes and is traceable.

[Artazor]

This will help to express the asynchronous semantics of Promises/A+ as well.
Ping @benjamingr, @spion

[yortus]

@Artazor interesting idea, I hope it gets some further investigation. Couple of additional points:

• The deferred annotation would enable better use before assignment checks that would address this issue. However the narrowing inside function expressions problem (see Improve control flow analysis in function expressions #8849, Discriminated union types lose their narrowed type inside nested function scopes #9258) would not benefit. That problem would benefit from an analogous immediate annotation, e.g. for things like Array#map, Array#filter, etc.
• The placement of the deferred annotation might be better kept within the type annotation so it is simply erased with the rest of the type annotations. Making it a modifier in a name/field position (eg var deferred a: ...) looks like expression-level syntax that could clash with some future ECMA addition.

[RyanCavanaugh]

This is good conversation and everyone's on the right track. That said, the way the compiler is currently written, it is likely impossible to add this kind of functionality to the language without a massive architectural change.

The problem is that reachability and control flow analysis is done before typechecking. In other words, the way things are today, in this code example below, we have some limitations.

let g;
f(() => g());
g = () => {};
h();

The type of f cannot inform whether or not we believe g to be initialized, because by the time we resolve the type of f, we've already made up our minds about the initialization state of g. Additionally, even if f has the return type never, we still couldn't detect that the last two lines are unreachable (because again, this comes after the typechecking phase).

There's no straightforward fix here because a type-aware reachability analysis would also potentially modify the types of the program, meaning the reachability analysis would also have to be rechecked, leading to an unbounded (though probably finite?) number of analysis cycles. We're just not architected for that at the moment.

[kitsonk]

@sledorze

what do you mean by 'safe emit' ? (Trying to understand the meaning not to disagree)

We are dealing with the temporal dead zone here. So emitting any sort of "safety check" on the variable is meaningless. For example, if the callback is immediately invoked, using Ryan's example:

const arr = [1, 2, 3].map(x => {
  return x + (arr && arr.length); // ReferenceError: arr is not defined
});

const arr = [1, 2, 3].map(x => {
  return x + (arr ? arr.length : undefined); // ReferenceError: arr is not defined
});

const arr = [1, 2, 3].map(x => {
  let value = 0;
  if (arr) { // ReferenceError: arr is not defined
    value = arr.length;
  }
  return x + value;
});

Of course if it is the callback is not immediately invoked, there is no need to check the value. Welcome to the world of the TDZ! Something I knew about but I didn't really understand until this (because for some reason in the 8 years of my life spent on JavaScript, I was "lucky" to have never written TDZ code, which I am still shocked at).

[novemberborn]

Perhaps strictNullChecks shouldn't concern itself with TDZ violations. They strike me as a different kind of error, at least in most situations. A strictDeadZoneChecks could be introduced instead.

[RyanCavanaugh]

@kitsonk TDZ only occurs for let / const, so it's unsurprising you hadn't encountered it before. A var in its would-be TDZ is just undefined (which I'm sure you have seen 😉)

[kitsonk]

@RyanCavanaugh ah, yeah, that might work for us now. We banned via linting var but obviously we can hint those rules off for this.

[unional]

In this example, replacing const with let and the error goes away:

class foo {

  boo(callback: () => void): Function { return () => { }; }

  foo() {
    const fn = this.boo(() => {
      fn();
    });
  }
}

class foo {

  boo(callback: () => void): Function { return () => { }; }

  foo() {
    let fn = this.boo(() => {
      fn();
    });
  }
}

i.e. same as this comment: #9382 (comment)

[mhegazy]

This should be fixed by #10815