noFallthroughCasesInSwitch complains about last case

[domoritz]

I was a bit surprised that the last case also requires a break with noFallthroughCasesInSwitch in 1.8. Is that intentional?

switch (x % 2) {
    case 0:
        console.log("even");
         break;
    case 1: // Error: Fallthrough case in switch.
        console.log("odd");
    // no break here
}

[RyanCavanaugh]

On the one hand, it's not technically a fall-through.

On the other hand, it's a maintenance trap for the next person who adds a case label.

❓ 😕 ❓

[domoritz]

This actually is consistent with what tslint does.

@RyanCavanaugh
One could argue that if the next person adds another case, ts will throw the error. But there are certainly more important things to do so the design makes sense.

[fongandrew]

This is also triggering on the default case at the end. Presumably, no one is adding cases after default, so that shouldn't count as a fall-through or a maintenance issue, right?

[mhegazy]

For consistency, the default label is not treated like any of the other case labels.

[aleung]

@domoritz Tslint no-switch-case-fall-through rule doesn't require a break in last branch.

[domoritz]

@aleung You're right. I mixed up two things. Tslint does not require a break for the last case.

[evmar]

As an experiment we attempted enabling this flag for the TypeScript code at Google (~1k source files across multiple teams).

It caught one bug, found one case where the fallthrough was intentional (there was a comment indicating as such), and triggered on ten other cases where the code was already correct and didn't include the final break.

With a 1 in 10 useful triggering rate, it's the sort of thing we might not be able to use, or need to put into some tool that runs offline in some sort of advisory role, which is kind of a bummer. Does the additional information about this being inconsistent with tslint influence your decision any? @mhegazy

(I think the "maintenance trap" argument Ryan made isn't quite right, because if next person who adds a case label doesn't add the appropriate break, their error will be caught by exactly this compiler check.)

[RyanCavanaugh]

I'll bring it up for discussion.

As for your hit rate -- finding 1 bug at the cost of adding 9 mechanical break; statements (should take 2 minutes tops?) sounds like an insanely good ROI. If I could find bugs at that cost, I'd do it at the exclusion of all other activity.

[alexeagle]

Did you guys roll out this check to any MS codebase before finalizing the implementation? You could have a much higher ROI if you detected only the cases where fallthrough has another case to fall into.

I think it's important to define the goals of static analysis in the compiler. Could I join in for the discussion?

(I spent a year making static analysis happen in Google's Java compiler. See http://static.googleusercontent.com/media/research.google.com/en//pubs/archive/43322.pdf and we have a paper coming soon specifically about http://ErrorProne.info which I started)