[Issue 3536] New: [patch] Make switch case error at unintentional fallthrough. (allow intentional fallthrough)

[d-bugmail at puremagic.com]

http://d.puremagic.com/issues/show_bug.cgi?id=3536

           Summary: [patch] Make switch case error at unintentional
                    fallthrough. (allow intentional fallthrough)
           Product: D
           Version: 2.036
          Platform: Other
        OS/Version: Linux
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: DMD
        AssignedTo: nobody at puremagic.com
        ReportedBy: chadjoan at gmail.com


--- Comment #0 from Chad Joan <chadjoan at gmail.com> 2009-11-20 17:22:08 PST ---
Created an attachment (id=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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------