hegel icon indicating copy to clipboard operation
hegel copied to clipboard

Switches - disallow fall through

Open WickyNilliams opened this issue 4 years ago • 6 comments

Hey,

I'm not sure if this kind of things is outside of the scope of the language (is it "only" supposed to add types to JS, or is it an opportunity to improve other areas too?), but I thought I would suggest.

First, I love that you force exhaustive switches! Something that feels sorely lacking in TS.

I am wondering if you could also error if a case doesn't contain a break/return statement. This is a common source of errors in switch statements, and the fall-through behaviour is generally considered a bad design decision in JS. Disallowing fall through will prevent bugs, especially as it's likely switch will see more common use if it's used regularly for type exhaustion.

WickyNilliams avatar May 20 '20 13:05 WickyNilliams

Hi @WickyNilliams Yes, first of all, we currently want to have the next switch features:

  1. Prevent lost cases in return (in development):
function test(a: number): string {
    switch (a) {
       case 1: return "";
    }
  // if "a" will have not "1" value it will return undefined
}
  1. Show lost cases (try):
function test(userType: "Admin" | "Moderator" | "User") {
    // Error: This switch case statement is not exhaustive. Here is an example of a case that is not matched: 'Moderator'
    switch(userType) {
        case "Admin": return 2;
        case "User": return 3;
    }
}
  1. Inference type inside case block, which implicitly can show lost break - statement. (try)
type Node = 
 | { type: "Declaration", value: number }
 | { type: "Expression", value: () => unknown }
 | { type: "Comment", value: string }

function test(node: Node) {
    switch(node.type) {
        case "Declaration":
          return node.value + 4;
        case "Comment":
          // Do something, but lost "return", "break" or "throw"
        case "Expression":
          // Error: The type "(() => unknown) | string" is not callable.
          return node.value();
    }
}

Thank you for the question ^_^.

JSMonk avatar May 20 '20 15:05 JSMonk

Awesome! I think it's a great idea to be a little stricter where it makes sense. Using the switch to narrow types is very nice.

So, if I'm reading this right, when development is complete:

  1. all switches must be exhaustive or have a default case
  2. all cases must have a break or a return

Is that correct?

WickyNilliams avatar May 20 '20 15:05 WickyNilliams

The first one is absolutely correct. But the second one is not, because we don't add explicitly restriction for it, but, type refinement via previous case can help you to find the missed break/return ahead of time.

JSMonk avatar May 20 '20 15:05 JSMonk

I just realised I missed an important condition:

  1. all non-empty cases must have a break or a return

Would this be considered?

It is almost always a bug to not have a return/break in cases with a body. eslint has this on by default in its recommended configuration, so people are used to tools telling them this. C# has this exact rule, so there's precedent in other languages.

WickyNilliams avatar May 20 '20 16:05 WickyNilliams

Hmm. Okay, I will think about adding the rule in Hegel.

JSMonk avatar May 20 '20 17:05 JSMonk

Of course it is for your consideration. It's just a suggestion, but one which I think corrects a bad design decision in JS and will certainly prevent bugs. I made this exact mistake today :)

WickyNilliams avatar May 20 '20 17:05 WickyNilliams