Flow: Flow doesn't detect when a function handles all cases of a mixed type

[whmountains]

With if/else:

const getGreeting = (name: 'John' | 'Rebecca'): string => {
  if (name === 'John') {
    return 'Hi John!'
  } else if (name === 'Rebecca') {
    return 'Hi Rebecca!'
  }
}

With switch:

const getGreeting = (name: 'John' | 'Rebecca'): string => {
  switch (name) {
    case 'John':
      return 'Hi John!'
    case 'Rebecca':
      return 'Hi Rebecca!'
  }
}

[whmountains]

The error I get is "string type is incompatible with an implicitly returned undefined".

[lll000111]

_Hello dog, here is cat._

Flow is right. While you tell Flow that the variable should have only those two values, without context we cannot be sure. Any caller may have a different idea and send something else - and then the return value will be undefined.

Even if, with context, you are dead sure "this can NEVER happen", it is good coding style to recognize that often in the real world somebody messes up and the unthinkable does happen, and you get an invalid parameter from somewhere. That's why you add an else branch or a `default:´ statement, respectively, and throw an error if that happens to indicate the abnormal and unexpected parameter.

You rely on things always working as intended, which is not a good default mind set in programming (or in engineering in general). You need to be(come) waaayyyyyy more paranoid :-)

[tempname11]

@lll000111 I respectfully disagree. I feel like the kind of "defensive programming" you describe goes again the grain of what Flow is about.

Of course, in the real world an any (or a value in uncovered code) might be passed into this function, and then there are no guarantees that the types of any values computed subsequently match what Flow says they should be. If we then start safeguarding everything (i.e. checking for null when the annotation says a value is a { foo: Bar}), then our code will become littered with those checks.

At some point you do need to stop and say: here are some assumptions about the values in this piece of code. What Flow allows is to express (some of) these assumptions in code, and prove some nice correctness properties about it.

In this specific example, Flow needs to prove that the function will, given 'John' or 'Rebecca' as the argument, return a string. It does that, therefore, I'd say the better behavior would be not to show a type error.

@whmountains my favorite option to deal with this (unfortunate?) type error is to use an explicitly-named helper like this:

const impossible = (value: empty): any => {
   // log the error? throw? you decide here
};

const getGreeting = (/*...*/) => {
  /* ... */
  return impossible(name);
}

Because it uses the empty type, it has the benefit of ensuring that all your code paths stay explicitly covered, even when you add another case (like 'Mike') to the type of name. See here

[lll000111]

I feel like the kind of "defensive programming" you describe goes again the grain of what Flow is about.

I think _this_ assumption goes against what Flow is about :-)

Especially in this example. How is Flow supposed to check all possibilities? You are telling it "this is type X". But Flow cannot verify that that claim is actually true, hence the error. Who is preventing a caller form using any other string? Flow should just take your word for it? That would save a lot of internal Flow code and effort.

[whmountains]

@tempname11 Thanks for the tip. I will use it as a stopgap for the moment.

@lll000111 IMHO this is not a question of whether flow should infer types, it's a question of where the error should show up.

Let's say there's another function called main, and that main calls getGreeting:

const main = () => {
  getGreeting('KittenWithHerbs')
}

This is obviously an error, and we are all in agreement that Flow should catch the error. The question is whose fault is it. Wouldn't it make more sense to blame main for passing an illegal value rather than getGreeting for not handling it correctly?

I'm guessing the reasoning for Flow requiring an explicit check is for mixed codebases that are not 100% covered with Flow. In this case, it would be more hygienic to explicitly check the inputs at run time, thus ensuring that you always immediately throw errors on invalid arguments.

[asolove]

It is a reported and in progress fix to make this kind of exhaustive ness checking work in flow. Flow makes many assumptions that aren’t safe in an untyped codebase, so I don’t think that is the reason.

For now, the empty tricky is recommended.

[roryokane]

I think this is a duplicate of #451 "Flow doesn't detect exhaustive type refinement". #1835 has a more similar example to the one in this issue, but it was closed in favor of #451.

[whmountains]

Thanks @roryokane. I'll close this in favor of #451.