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

Uninitialized variables work around strictNullChecks #13884

Closed
rraval opened this issue Feb 5, 2017 · 17 comments
Closed

Uninitialized variables work around strictNullChecks #13884

rraval opened this issue Feb 5, 2017 · 17 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@rraval
Copy link

rraval commented Feb 5, 2017

TypeScript Version: 2.2.0

--strictNullChecks must be used.

Code

interface Foo {
    name: () => string;
}

let foo: Foo;
setTimeout((() => foo.name()), 0);

Expected behavior:

Error at let foo: Foo line since the compiler is implicitly doing let foo: Foo = undefined;.

If I were to write the = undefined initializer myself, the compiler properly errors with:

Type 'undefined' is not assignable to type 'Foo'.

Actual behavior:

The code compiles and errors at runtime:

TypeError: foo is undefined
@rraval
Copy link
Author

rraval commented Feb 22, 2017

Ping! This still feels broken to me.

@patrickhulce
Copy link

I've been bitten by this too, this seems broken that let foo: Foo; can be an allowed statement with --strictNullChecks.

It's not even that TS is unaware that it's potentially undefined as the below snippet does error.

interface Foo {
    name: () => string;
}

let foo: Foo;
foo.name(); // foo.ts(6,1): error TS2454: Variable 'foo' is used before being assigned.

@gcnew
Copy link
Contributor

gcnew commented Feb 22, 2017

Have you tried const?

@patrickhulce
Copy link

Using const errors with foo.ts(5,7): error TS1155: 'const' declarations must be initialized which is nice, but in a case where a variable may or may not be set later (necessitating let), I would like the type checker to prevent me from assuming it's certainly defined later.

@gcnew
Copy link
Contributor

gcnew commented Feb 22, 2017

You can add | undefined to such variables. I agree that it's a shortcomming of the compiler that it's unable to track whether a variable is initialized inside closures, but that's because you can't know whether the closure is immediately invoked or stored for later use.

@patrickhulce
Copy link

Ah, my example was a bit unclear. I'm fine with TS being unable to know when a variable has been initialized in a closure. I'm advocating for the same expected behavior as the OP, no error at line 6 but an error at line 5 Type 'undefined' is not assignable to type 'Foo' which does not require tracking of initialization beyond the variables declaration statement.

@gcnew
Copy link
Contributor

gcnew commented Feb 22, 2017

Well, that's kind of unpractical for deferred initializations such as in if/else.

@patrickhulce
Copy link

Sure, but I'm not enabling --strictNullChecks and using TypeScript for the practicality and free-wheeling spirit of initializing variables however I please 😉 . When I've declared a variable to be of a particular type, I've explicitly expressed the desire for undefined to not be considered a member of that type, and I assign undefined to that variable somewhere in my program (regardless of if I potentially re-assign to a valid type elsewhere, such as in your if/else example), I expect a reasonable compiler to provide me with an error.

I'm sure there are cases where this necessitates additional ! operators throughout the code, but that's to be expected if indeed a particular Object "is possibly 'undefined'".

@rraval
Copy link
Author

rraval commented Feb 23, 2017

@gcnew

Well, that's kind of unpractical for deferred initializations such as in if/else.

Coming from a CoffeeScript background, this seems like a non-sequitur to me. I consider deferred initialization to be a bit of a code smell and tend to rely on immediately invoked function expressions for complicated initialization:

var x = (function() {
    // can even use complicated control flow
    switch (y) {
        case 1: return 'foo';
        case 2: return 'bar';
        default: return 'unknown';
    }
})();

CoffeeScript has syntax sugar for IIFE's with do notation that makes this even more idiomatic.

The point is, instead of forcing the compiler to reason about variable initialization (which isn't complete because it can't deal with closures), just do complicated initialization via IIFE's. Syntax sugar is nice but it's workable without.

@gcnew
Copy link
Contributor

gcnew commented Feb 23, 2017

@rraval But then, how do you express self referential initializations?

let self = {
    x: 4,
    doSomething: () => self.x + self.x
}

It can be made a special case, however I think the current situation strikes a good compromise:

  • const variables are always initialized. As a bonus they are assigned just once, which is a good thing
  • you can have a convention to always initialize let variables or add | undefined to the lazily inited ones and enforce it by a lint rule

@rraval
Copy link
Author

rraval commented Feb 23, 2017

@gcnew I'm not sure I follow. The following code seems perfectly reasonable to me:

let self = {
    x: 4,
    doSomething: () => self.x + self.x
};

self would have type {x: Number, doSomething: () => Number}, which the () => self.x + self.x expression captures and everything should typecheck cleanly.

Here's a thornier example that currently breaks even with const:

function indirection(func: () => Number): Number {
    return func();
}

const foo: {a: Number, b: Number} = {
    a: 1,
    b: indirection(() => foo.a),
};

This results in a runtime TypeError because foo is undefined when the callback for b is invoked.

Here's how I would expect things to work. I might've gotten things horribly wrong so please correct me if so:

  1. In strictNullChecks, given a declaration let x: T or const x: T where undefined ∉ T, x must be immediately initialized.
  2. In the initializer for x, function definitions that capture x are split into 2 types:
    1. Simple definitions where the function cannot be passed to anything else will have x: T in their body. This means that the doSomething: () => self.x + self.x from your example has self: {x: Number, doSomething: () => Number}.
    2. If the function //can// be passed to something else, the compiler can no longer reason about when the function gets called. Thus, for the body of the function, x: T | undefined. For my example, b: indirection(() => foo.a), inside the function foo: {a: Number, b: Number} | undefined, at which point the compiler can complain about foo.a because it no longer typechecks.

Point 1 can be made palatable with sugar for IIFE's for complicated initialization.

Point 2.1 should capture the bulk of self referencing function definitions and things just work. This is because the compiler knows that those functions cannot be invoked until after the variable has been initialized.

Point 2.2 is necessary to ensure correctness. The current version of the compiler breaks with a runtime TypeError, which seems to fly in the face of TypeScript's primary goal.

@rraval
Copy link
Author

rraval commented Mar 7, 2017

Ping!

We might as well close this issue if it's just going to languish like this.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 7, 2017
@RyanCavanaugh
Copy link
Member

@gcnew 's comments already addressed why this is the intended behavior. See #10641, #9382, #10815

@farzher
Copy link

farzher commented Feb 21, 2018

let num_or_undefined:number
function runtimeerror() { num_or_undefined.toString() }
runtimeerror()

How is this not a compile error in strict mode? How can i make it error?

let num_or_undefined:number|undefined works as intended, but i have to remember to put |undefined everywhere.

I feel like maybe line 1 should be an error?
Something like Type 'undefined' is not assignable to type 'number'.
(it would be better if it could just be automatically considered 'possibly undefined' though)

(typescript's not making me feel very type safe)

@patrickhulce
Copy link

@bterlson we discussed this at Edge summit way back in September, any chance you followed up? :)

@agopshi
Copy link

agopshi commented Apr 9, 2018

@RyanCavanaugh Why was this closed? Those fixes don't cover this particular issue.

I think it's especially important to revisit this issue now that --strictPropertyInitialization is available. With --strictPropertyInitialization, this code correctly emits a compile error:

Link

class Foo {
  private x: number; // Compile-time error!
  foo() {
    // `x` isn't initialized!
    // But it's OK, compile-time error above.
    return this.x.toString();
  }
}

But this code compiles successfully:

Link

function fooFactory() {
  let x: number;
  return {
    foo() {
      // `x` isn't initialized!
      // Run-time error!
      return x.toString();
    }
  };
}

Shouldn't there be a --strictLocalInitialization mode to catch errors like this? I imagine it would work similarly to --strictPropertyInitialization: if a local variable is not synchronously initialized before it's used, a compile error would be emitted. To resolve it, one would need to:

A) Initialize the variable in a way that the TypeScript compiler understands (e.g. immediately, or before referencing it in a function somewhere).

-OR-

B) Use a definite assignment assertion. (It seems that this issue would come up naturally when discussing definition assignment assertions for local variables.)

-OR-

C) Explicitly mark the variable as potentially undefined.


Expanding upon option (B), this code was given as an example in the linked-to blog:

let x!: number[];
initialize();
x.push(4);

function initialize() {
    x = [0, 1, 2, 3];
}

I think that this is a misleading example. That code - in an ideal world where the compiler performs flow analysis - should not need the definite assignment assertion.

However, this code should (and currently does not):

let x: number[]; // Doesn't require definite assignment assertion.
function foo() {
    x.toString();
}
foo(); // No compile-time error, but throws an exception.

Thoughts?

@RyanCavanaugh
Copy link
Member

@agopshi you can log a new suggestion for that

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

6 participants