522774 - Static analysis: Error when falling though switch cases without an explicit "fallthrough" annotation

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I've coded this bug all the time

  switch(foo) {
  case FOO:
     doSomething();
     // oops! forgot to |break;|

  case BAR:
     doSomethingElse();  // CRASH!  bad fallthrough
  }

It would be great to require an explicit annotation for falling through switch cases.  Perhaps something like

  __attribute__(((user "fallthrough"))) inline static fallthrough() { }
  #define FALLTHROUGH()  fallthrough()

  switch (foo)
  case FOO:
     doSomething();
     // if i wanted to fallthrough, i would write
     // FALLTHROUGH();
   
  case BAR:
  ...
  }

The code above would trigger an error, because the fallthrough wasn't explicitly allowed.  Uncommenting "// FALLTHROUGH();" would suppress the error.

This is a pretty straightforward treehydra-CFG analysis.

Comment 1

[Ehren Metcalfe]

Attachment: script

Here's a standalone script that's pretty much airtight. The only 'false positive' is |case '\0'| here: http://mxr.mozilla.org/mozilla-central/source/js/src/jskwgen.cpp#213 but this is kind of a fallthrough in disguise.

Also, whenever a case falls through to more than one case eg

case 0:
  x++
  /* Fallthrough */
case 1:
case 2:
case 3:
  x--;

the script will issue multiple warnings, but I suppose that's not too much of an issue. 

If we were willing to go forward and add this to a static checking build, would adding the 'fallthrough function' to nscore.h be sufficient?

Comment 2

[Ehren Metcalfe]

Attachment: list of fallthrough sites

Comment 3

[Tiziana Sellitto [:tiziana]]

I'm going through old "good first bugs" that have been inactive for a while. 
Chris, is this something you can help Ehren with? Do you think this bug could become a mentored bug?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This area isn't being worked on and I'm not available to mentor in it.

Comment 5

[Joshua Cranmer [:jcranmer]]

Our static analysis is built off of Clang these days, which has a warning for this sort of stuff (-Wimplicit-fallthrough) and an attribute to mark when it is explicitly desired ([[clang::fallthrough]]). We don't need a static analysis for this anymore, then.