Flow: Typed value turns into untyped after refinement

[mislav]

Summary: when refining the value V of type T1 as type T2 (either using instanceof conditional or an invariant call), V becomes untyped if T2 is a class that is _not a subclass of T1_.

Demonstration:

/* @flow */

class A {}
class B extends A {}

const obj1 = new A
if (obj1 instanceof B) {
  obj1.nonexist()
  //   ^^^ Failed as expected: "Cannot call obj1.nonexist because property nonexist is missing in B."
}

const obj2 = 'string'
if (obj2 instanceof B) {
  //                ^^^ Expected flow to complain since B is not a subtype of string
  obj2.nonexist()
  //   ^^^ Expected flow to fail, but didn't as `obj2` is now untyped
}

What I expected:

1. No untyped values in my code;
2. Flow to report an error on obj2.nonexist() call;
3. (Optionally) Flow to report an error that a string type cannot possibly be instanceof B.

What happened instead:

1. obj2 became untyped after refinement.

Flow version tested in: v0.80.0. This behavior seems to have existed for a long time in Flow and doesn't seem like a recent regression.

Apologies if there are existing reports for this; I scanned open issues for keywords like “refine” and there were many existing ones, but most of them deal with more complex scenarios such as type unions, and none that I saw really describe a scenario similar to this one.

[wchargin]

This behavior is expected and correct, though I’d phrase it differently.

Extracting the essence of your example:

// @flow
class A {}
const x: string = "hello";
if (x instanceof A) {
  x.wat();  // no error (good)
}

Inside the if-statement, x is not “untyped”; its type is not any.
Rather, it has been refined to have type empty. The type empty is a
type with no values, and therefore to say that a value has type
empty implies that the code can never reach that point.

In other words, Flow knows that it is impossible for the code to reach
this branch, and so it refines the type of x accordingly.

Because you can never have a value of the empty type, the empty type
admits all operations, including your .nonexist(). This is the
“principle of explosion”: _ex falso quodlibet_.

Refinements with empty are, in fact, quite useful in practice. Suppose
that I write the following code:

// @flow
type Fruit =
  | {|+type: "APPLE", +color: string|}
  | {|+type: "ORANGE", +weightInPounds: number|};
function processFruit(f: Fruit): void {
  switch (f.type) {
    case "APPLE":
      console.log(`What a nice ${f.color} apple.`);
      break;
    case "ORANGE":
      console.log(`What a hefty ${f.weightInPounds}-lb. orange.`);
      break;
    default:
      throw new Error("unknown fruit: " + (f.type: empty));
  }
}

This code is pretty straightforward, but it has one interesting line:
the error in the default case includes an assertion that f.type is
of type empty. As described above, Flow lets this typecheck because it
can statically verify that any Fruit must match one of the cases
APPLE or ORANGE above. But this is terrifically useful: it means
that if I add an additional fruit to the type definition—

type Fruit =
  | {|+type: "APPLE", +color: string|}
  | {|+type: "ORANGE", +weightInPounds: number|}
  | {|+type: "PEAR", +bruised: boolean|};

—then Flow will complain that my switch statement has not matched all
possible cases (and in particular that f.type cannot be cast to
empty because "PEAR" is not covered). On large codebases, this is
invaluable: it means that you can confidently make a single change to a
type definition, re-run Flow, fix all the errors that Flow tells you
about, and be confident that the resulting code is correct.

In summary, if Flow can prove that some code is unreachable, a relevant
variable _should_ be refined to empty, because this enables the
programmer to _assert_ that the code is unreachable, which in turn
enables Flow to alert the programmer if the code _changes_ to no longer
become unreachable. Flow is protecting your future self against changes
to the code.

---

Your point (3), that if ((x: string) instanceof A); should raise a
type error, is a reasonable suggestion, especially in light of the fact
that the following _does_ raise a type error for this reason:

// @flow
class A {}
class B {}
declare var a: A;
if (a instanceof B);  // error

[mislav]

@wchargin Thank you so much for explaining.

We at GitHub have been doing if (event instanceof CustomEvent) all over the codebase to refine an Event so to access its .detail property, but in certain contexts event wasn't a built-in Event type to start with (it was a custom type that implements the Event "interface"). We didn't know that event was an empty type within all these blocks until we ran flow-coverage-report.

Should we switch to a different approach to refinement?

Re: (3), this behavior of Flow also means that typos in code could silently be tolerated. For example:

class A {}
class Blubber extends A {
  hello(name: string): void;
}
class Flubber {}

const a = new A
if (a instanceof Flubber) {
              // ^ typo; should have been Blubber
  a.hello(12)
       // ^^ this invocation style exists in neither Blubber nor Flubber,
       // but Flow allows it.
}

[wchargin]

Yep, that’s understandable.

Your suggestion (3) sounds like the best solution. In the meantime, as a
workaround, you could explicitly assert the upper bound by guarding
against ((e: Event) instanceof CustomEvent):

// @flow
class CustomEvent extends Event {
  +detail: string;
  // etc.
}

declare var actualEvent: Event;
if (actualEvent instanceof CustomEvent) {
  actualEvent.detail; // OK (good)
  actualEvent.wat; // error (good)
}

declare var notReallyEvent: string;
if (notReallyEvent instanceof CustomEvent /* no type error (unfortunate) */) {
  (notReallyEvent: empty); // whoops
}

if ((notReallyEvent: Event) instanceof CustomEvent /* type error (good!) */) {
  (notReallyEvent: empty); // error (good)
}

(No flow-try link because the Event type isn’t available in that
environment, but this works as a standalone file in Flow v0.80.0.)

As workarounds go, this one isn’t too bad (IMO): it’s explicit and
sensible, and doesn’t require too much typing. As with any workaround,
though, you have to remember to use it consistently, which is
unfortunate.

In your Flubber/Blubber example: agreed that this is bad, noting
that the problem is the lack of type error on the instanceof check,
not the lack of type error on the hello(12) (which can never cause a
problem because it is unreachable).

[mislav]

Thank you! Feel free to close this, unless you want to keep the issue open as a reminder to tweak instanceof behavior, in which case perhaps we could rename the issue.

[wchargin]

You’re quite welcome.

I don’t have ACLs to close (I don’t work on Flow), so I’ll let you or a
Flow maintainer handle this as you see fit.