Bug 3536 – [patch] Make switch case error at unintentional fallthrough. (allow intentional fallthrough)

Created attachment 504 Contains patch and test cases. [patch] Make switch case error at unintentional fallthrough. (allow intentional fallthrough) This patch implements the following: <qoute> Case statements and default statements come in two flavors: 'fall through' and 'unconditional branch'. The 'unconditional branch' variety are denoted with a normal colon (:) like C and C++ case statements. However, unlike C and C++, the block contained by these must end in one of these unconditional branch statements: break, return, continue, goto, goto case, goto default, or throw. The reasons for this are to prevent the common mistake of forgetting to place break at the end of a case statement and to encourage the catching of such mistakes while porting C and C++ code to D. The 'fall through' variety are denoted with the !: token. These behave like the traditional C and C++ case statements and allow for case statements to 'fall through' to subsequent case values. </qoute> For example: switch (i) { case 1!: // Intent to use fall through behavior. x = 3; case 2!: // It's OK to decide to not actually fall through. x = 4; break; case 3,4,5: x = 5; break; case 6: // Error: You either forgot a break; or need to use !: instead of : case 7: // Fine, ends with goto case. goto case 1; case 8: break; x = 6; // Error: break; must be the last statement for case 8. } The complete patch is given as switchcase.patch. The patch is from dmd 2.036. There are two test cases in the attached file: pass.d and fail.d. pass.d should compile while fail.d is full of things that shouldn't compile. Patches of finer granularity are also given: switchcase-html.patch // Patches the entire html directory (mostly the language spec). switchcase-src.patch // Patches the entire src directory. switchcase-src-dmd.patch // Patches src/dmd (only the things needed to make dmd accept the changes). switchcase-src-phobos.patch // Patches src/phobos switchcase-src-druntime.patch // Patches src/druntime These should make it easier to play with the patch. For example you could patch dmd and not phobos, then see what kind of error messages it gives when run on phobos' pre-patch D code. As far as I could tell, phobos passed all unit tests after the changes. This was difficult due to http://d.puremagic.com/issues/show_bug.cgi?id=3140 If you don't care about combing your code for fallthrough bugs, you can use a sed expression to update most code to the new switch-case: sed -r 's/((case|default)\s*?[_a-zA-Z0-9]*?):/\1!:/' input.d > output.d I realize that I could have made swapped !: and : to make existing code survive unmodified. I decided against that since : is what people will natural use (due to experience from C-like languages) and they may not realize !: exists until it is too late. This makes the intuitive default option become the safe one, while making the harder to find/use feature be the unsafe option (fallthrough). You'll probably notice that I haven't added assert to the list of unconditional branches. That's because it's conditional. assert(1) won't divert execution, and it is trivial to write asserts that may or may not end the program depending on some runtime value. The common pattern is much more restricted: assert(0). If the compiler bothers to dig into the assert's parameter and finds a literal 0 or false there, then it can safely assume that execution is not supposed to reach that point. assert(0) also raises the question: what about assert(true ? 0 : 1); ? what about assert(zero!()); ? Should the compiler be expected to figure those out, or expected to figure out that the zero it's looking at isn't one of them? If you specify that constant-folded zeros are not allowed, then dmd's implementation is fairly straightforward: int ExpStatement::isUnconditionalBranch() { AssertExp *assertExp = dynamic_cast<AssertExp*>(this->exp); if ( assertExp ) { return assertExp->e1->isBool(FALSE); } return FALSE; } Other minor known issues: The case range syntax suggests the existence of these: case 50: .. case 54!: case 55!: .. case 59: however those don't really make sense. The patch is written such that those are treated as invalid D code and are not recognized by the spec or by dmd. There are some cases in which an erraneous case statement can cause the compiler to emit extra garbage error messages: T bar(T)(T baz) { return baz; } ... switch(i) { case '1': case '2': // Error: The last statement in a case or default statement must be... int j = .bar!(int)(i); // Error: function expected before (), not __error of type int break; ... } The first error is expected. The second is garbage.

Created attachment 505 Complete patch.

Created attachment 506 Patch for all source files only (excludes spec/doc changes). diff of ${dmd}/src

Created attachment 507 Patch for dmd compiler source files only. diff of ${dmd}/src/dmd

Created attachment 508 Patch for phobos source files only. diff of ${dmd}/src/phobos

Created attachment 509 Patch for druntime source files only. diff of ${dmd}/src/druntime

Created attachment 510 Patch for dmd docs only. diff of ${dmd}/html

Created attachment 511 Test cases that should compile with a patched compiler.

Created attachment 512 Things that should not compile with the patched compiler.

See also bug 4349

Is it possible to close this bug now?

Yes, it can be closed. Here ya go.

(In reply to comment #11) > Yes, it can be closed. Here ya go. I do appreciate the effort you put into this (especially since you've gone the distance with test cases and documentation), and am sorry that it hasn't caught interest. One of the most difficult parts of my job with D is to say no, and sometimes I feel like Doctor No. I know how hard it is on contributors like you.

(In reply to comment #12) > (In reply to comment #11) > > Yes, it can be closed. Here ya go. > > I do appreciate the effort you put into this (especially since you've gone the > distance with test cases and documentation), and am sorry that it hasn't caught > interest. > > One of the most difficult parts of my job with D is to say no, and sometimes I > feel like Doctor No. I know how hard it is on contributors like you. Hey, thanks for the acknowledgment!