Pattern matching via switch should not require default case if all cases are handled

[Gozala]

Here is an example of valid code that flow fails to type check:

/* @flow */

type Message =
  | { tag: "Inc" }
  | { tag: "Dec" }

const update =
  (model:number, message:Message):number => {
    switch (message.tag) {
      case "Inc":
        return model + 1
      case "Dec":
        return model - 1
    }
  }

Flow produces following errors:

scratch.js:8
  8:   (model:number, message:Message):number => {
                                       ^^^^^^ number. This type is incompatible with an implicitly-returned undefined.


Found 1 error

I believe what error is trying to tell is that if non of the cases match update would return undefined, but I would expect flow to infer that all possible cases are handled and there for do not fail to type check this code.

Unfortunately this issue is far worse that it may seem as only way to satisfy type checker is to handle else / default clause, which means type checker won't be able to infer a lot of possible errors. For example consider solution to make flow type check.

/* @flow */

type Message =
  | { tag: "Inc" }
  | { tag: "Dec" }

const update =
  (model:number, message:Message):number => {
    switch (message.tag) {
      case "Inc":
        return model + 1
      case "Dec":
        return model - 1
      default:
        throw Error('Invalid message')
    }
  }

Now above code type checks fine and does pretty much what originally was intended. The problem is that with such code flow no longer will be able to detect unhandled cases. For example consider following evolution of the code above:

/* @flow */

type Message =
  | { tag: "Inc" }
  | { tag: "Dec" }
  | { tag: "Sqrt" }

const update =
  (model:number, message:Message):number => {
    switch (message.tag) {
      case "Inc":
        return model + 1
      case "Dec":
        return model - 1
      case "Srqt":
        return Math.sqrt(model)
      default:
        throw Error('Invalid message')
    }
  }

update(4, {tag: "Sqrt"}) // => Error('Invalid message')

Notice that flow is type checks this code fine even though there is a typo in handling Sqrt messages & error will only appear at runtime (and that's only because our default case throws instead of say returning model back).

[gabelevi]

Thanks for the detailed error report! Support for exhaustive refinements has been a TODO for awhile. I think we're using #451 as the master tracking issue for this. @bhosmer has done a little preliminary work on this kind of stuff, but exhaustive switch statement refinements for tagged unions is still a TODO

[Gozala]

@gabelevi in the meantime are there any recommended patterns one could follow to avoid a pitfalls mentioned ? Another class of issues / regressions we tend to run into is caused by changes like this:

/* @flow */

type Message =
  | { tag: "Increment" }
  | { tag: "Decrement" }

const update =
  (model:number, message:Message):number => {
    switch (message.tag) {
      case "Inc":
        return model + 1
      case "Dec":
        return model - 1
      default:
        throw Error('Invalid message')
    }
  }

Note flow will type check this fine. But you'll encounter runtime error, as changes to update methods were not made. If there was no default cause flow & exhaustive refinements were supported flow would have caught this. But then there are still cases (at least in our code base) where we do need default case & would highly benefit if flow was more strict and disallowed cases that can't occur.

[Gozala]

@bhosmer I wonder if the work you've being doing would also have some solution for the issue mentioned in last comment.

[jhegedus42]

related, possible workaround : http://stackoverflow.com/questions/40338895/sealed-case-classes-in-flow

[jhegedus42]

full code example:
you can try it here : https://flowtype.org/try/#0C4TwDgpgBAKg9gEzjEUC8UDeAoKVgQAewAXAM7ABOAlgHYDmA3LlAMZwC2YANhAQiQBGcOLwCGtZnmoDaAVw6CIlZgF9m2VtzFkysRHABic2q2DU4tPTjwcxAawgAKUhRoMANDJLzFygJQk8EgoNnhQlHxylLRYBMQkwF4CMh7sXLz8JABmYtxkEKosRXjAcPT0vC5BBiiBwcioYXiRwNGxmAB03UlsnDx8EAIAhMCd6QP86sXYRdigkFAAggAiKwD6MADyK1voWCwLECQA5Ksb27snHodErlR09DfSsgpKlLPY8+DQ2wDifwAMgBRTY7PYYTBHU7-IGgy5ba5Qby+d5QOZHfRIJZmCy0FD7c5g3ZQAA+sC2AJBxK2Xy+7CswHwBn2LgMNRCIA8UDEuMsHLgOPMljqUBIWMa6AAfAc8GQAO7UYCsAAWUCcvOFtE6R38zXCrB00DOaxpJzFLHCVoiURiWGRAk1eM6qXwdxITssOru3ImmSG4ty+UKUmthoKUBOsOpCJOJEt1uo2XVZSQLoQUGGaAwnu1Mn8WFa7WZSEYJWtVqLdq6PQMvv6-pGqbg4wbgwQ0wrAHou+HoBq+bRxfJuNxAvgVZQ4PLIyZ7LRp7FcycE3gEBBcnJuEzxQOtcOt2PV9aqx0HR7B+nufFSLnvcR6xl24G8gVO+jPkA

type TodoTy = {
  text:string;
  completed:boolean;
  id:number;
};

class TodoFunctions {
  make(t:string,id:number):TodoTy{
    return {text:t,id:id,completed:false}
  }
  toggle(t:TodoTy):TodoTy {
    return {...t, completed:!t.completed};
  }
}

type ADD_TODO = {
  type:'ADD_TODO',
  text:string,
  id:number
}

type TOGGLE_TODO = {type:'TOGGLE_TODO', id:number }

type TodoActionTy = ADD_TODO | TOGGLE_TODO



const todo = (todo:TodoTy, action:TodoActionTy) : TodoTy => {
  switch (action.type){
      case 'ADD_TODO' :
          return { id:action.id, text:action.text, completed: false};
      case 'TOGGLE_TODO':
        if (todo.id !== action.id) {return todo;}
          return {...todo, completed:!todo.completed};
      //case (action: null): throw 'unknown action'
      default : (action: null)
          return { id:action.id, text:action.text, completed: false};
  }
}

[Gozala]

Thanks @jhegedus42, I'm definitely going to employ this workaround

[Gozala]

Unfortunately it seems that it only covers cases where types in the union are type declarations with sentinel field, but it does not work at all if say classes are joined into a union like in this example

type TodoTy = {
  text:string;
  completed:boolean;
  id:number;
};

class TodoFunctions {
  make(t:string,id:number):TodoTy{
    return {text:t,id:id,completed:false}
  }
  toggle(t:TodoTy):TodoTy {
    return {...t, completed:!t.completed};
  }
}

class ADD_TODO {
  type = 'ADD_TODO'
  text:string
  id:number
}

class TOGGLE_TODO {
  type = 'TOGGLE_TODO'
  id:number
}

type TodoActionTy =
  | ADD_TODO
  | TOGGLE_TODO



const todo = (todo:TodoTy, action:TodoActionTy) : TodoTy => {
  switch (action.type){
      case 'not really a value of .type field' :
          return { id:action.id, text:action.text, completed: false};
      case 'neither is this one':
        if (todo.id !== action.id) {return todo;}
          return {...todo, completed:!todo.completed};
      default : (action: empty)
		throw Error()
  }
}

What I can't even explain is that removing one of the arms of the switch statement seems to trigger flow error 👀

type TodoTy = {
  text:string;
  completed:boolean;
  id:number;
};

class TodoFunctions {
  make(t:string,id:number):TodoTy{
    return {text:t,id:id,completed:false}
  }
  toggle(t:TodoTy):TodoTy {
    return {...t, completed:!t.completed};
  }
}

class ADD_TODO {
  type = 'ADD_TODO'
  text:string
  id:number
}

class TOGGLE_TODO {
  type = 'TOGGLE_TODO'
  id:number
}

type TodoActionTy =
  | ADD_TODO
  | TOGGLE_TODO



const todo = (todo:TodoTy, action:TodoActionTy) : TodoTy => {
  switch (action.type){
      case 'neither is this one':
        if (todo.id !== action.id) {return todo;}
          return {...todo, completed:!todo.completed};
      default : (action: empty)
		throw Error()
  }
}

[gaborsar]

This seems to work:

type A = { type: 'a' }
type B = { type: 'b' }

type AB = A | B

const fn = (v: AB): number => {
  switch (v.type) {
    case 'a':
      return 1
    case 'b':
    default:
      return 2
  }
}

const v1 = fn({ type: 'a' })
const v2 = fn({ type: 'b' })

[Gozala]

Sure but, if you add type `C` into that union and forget to update your `fn` you won't get any errors reported as you'll handle `C` as `B` this is also why requiring default case is problematic

On Wed, Jul 5, 2017 at 11:22 AM Gabor Sar ***@***.***> wrote: This seems to work: type A = { type: 'a' } type B = { type: 'b' } type AB = A | B const fn = (v: AB): number => { switch (v.type) { case 'a': return 1 case 'b': default: return 2 } } const v1 = fn({ type: 'a' })const v2 = fn({ type: 'b' }) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1835 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABS9NjhdYuLp-wJ6QsDJxv-cgmDRj8Gks5sK9RtgaJpZM4IlK6D> .

-- Regards -- Irakli Gozalishvili Web: http://www.jeditoolkit.com/

[gaborsar]

@Gozala I do agree with you! :)

[Gozala]

If anyone is interested this is a solution I have being using to avoid pitfalls outlined in this thread:
https://github.com/Gozala/corrupt