Flow: Removing nullable case in declarations

[andreypopp]

I'm writing declaration for a library and found the following case. Consider code like this:

class Reactive {

  constructor(value) {
    this.value = value;
  }

  maybeReact(f) {
    if (this.value !== null) {
      f(this.value)
    }
  }
}

The declaration I wrote looks like this:

declare class Reactive<T> {
  constructor(value: T);
  maybeReact(f: (value: T) => void): void;
}

Usage:

// Error here!
new Value(1).maybeReact(value => console.log(value + 42));

What if instead I could annotate type with some $NonNull<T>:

declare class Reactive<T> {
  constructor(value: T);
  maybeReact(f: (value: $NonNull<T>) => void): void;
}

[andreypopp]

Updated description.

[alexeygolev]

@andreypopp what is the error you're getting?

[andreypopp]

@alexeygolev see the example.

[vkurchatkin]

There is $NonMaybeType in master

[alexeygolev]

@vkurchatkin hm... interesting. I though that any type T is non-null unless it's ?T

[andreypopp]

@alexeygolev T is not a concrete type in this case and can be instantiated to be nullable.

@vkurchatkin awesome, $ prefix means that it is a "magical" type? Any ideas if it will be promoted to public API?

[vkurchatkin]

@alexeygolev not really. I guess you could use something like <T: string | number | boolean | Object>

@andreypopp yep, but I never hesitate to use magic types. Worst case scenario: it's gone and you can revert to using T

Any ideas if it will be promoted to public API?

There are really no public "magic" types, as far as I remember @thejameskyle talked about changing that

[alexeygolev]

@vkurchatkin @andreypopp $NonMaybeType seems like a workaround but I can't really see any other option at the moment

[alexeygolev]

@vkurchatkin @andreypopp wouldn't it be less of a hack if flow treated any type variable like T as a non-nullable? There is a bigger chance that somebody will forget to do null checks if T is nullable by default than a chance that a person will forget to do ?T (or even Nullable). I think nullability should be as explicit as possible. We don't have a luxury of a proper Option/Maybe so we need to capture the null as much as we can. Just my thoughts. Probably missing something

[andreypopp]

@alexeygolev the example I posted above shows that flow checks if instantiated T type is nullable, so it's ok I think.

[alexeygolev]

@andreypopp The thing is this line

new Value(1).maybeReact(value => console.log(value + 42))

should give you an error for not covering the null scenario. What you're trying to achieve is to state that your function can't have null as an argument. Am I correct?

[vkurchatkin]

There is a bigger chance that somebody will forget to do null checks if T is nullable by default

That's the point of Flow, it won't let you forget something like this.

[alexeygolev]

@vkurchatkin fair enough... I guess it's just a matter of preference. I would prefer explicit nullable type argument rather that explicit non-nullable. But I guess the design choice has already been made:)

[samwgoldman]

Here's a small repro of a Bad Thing™ happening with generics and the new $NonMaybeType

/* @flow */

class C<T> {
  x: T;
  constructor(x:T): void {
    this.x = x;
  }
  maybe(cb: (x: $NonMaybeType<T>) => void): void {
    cb(this.x); // should error, `this.x` can be null/void
  }
}

var maybeString: ?string = null;

var c = new C(maybeString);
c.maybe(x => {
  x.length; // will throw, no Flow error
});

The issue here is how we deal with polymorphic definitions. We test the body of a polymorphic definition by instantiating each type parameter with empty and mixed. The logic is that any body that type checks with both should also type check with anything in-between.

However, the $NonMaybeType constraint is irregular because it slices off parts of the type lattice in the middle.

Fixing this is probably non-trivial, but we should definitely look into it because we'd love to make more magic types and eventually get them stable enough to be non-experimental.

Thanks for the interesting issue report!

[andreypopp]

Looks like nullable didn't get into 0.31?

[andreypopp]

Apologize, have issues with PATH.