Issue 3536 - [patch] Make switch case error at unintentional fallthrough. (allow intentional fallthrough)
Summary: [patch] Make switch case error at unintentional fallthrough. (allow intention...
Status: RESOLVED WONTFIX
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: Other Linux
: P2 enhancement
Assignee: No Owner
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2009-11-20 17:22 UTC by Chad Joan
Modified: 2012-05-29 22:30 UTC (History)
3 users (show)

See Also:


Attachments
Contains patch and test cases. (47.92 KB, application/zip)
2009-11-20 17:22 UTC, Chad Joan
Details
Complete patch. (85.68 KB, patch)
2009-11-20 17:26 UTC, Chad Joan
Details | Diff
Patch for all source files only (excludes spec/doc changes). diff of ${dmd}/src (75.34 KB, patch)
2009-11-20 17:28 UTC, Chad Joan
Details | Diff
Patch for dmd compiler source files only. diff of ${dmd}/src/dmd (17.05 KB, patch)
2009-11-20 17:32 UTC, Chad Joan
Details | Diff
Patch for phobos source files only. diff of ${dmd}/src/phobos (44.76 KB, patch)
2009-11-20 17:33 UTC, Chad Joan
Details | Diff
Patch for druntime source files only. diff of ${dmd}/src/druntime (13.57 KB, patch)
2009-11-20 17:34 UTC, Chad Joan
Details | Diff
Patch for dmd docs only. diff of ${dmd}/html (10.31 KB, patch)
2009-11-20 17:36 UTC, Chad Joan
Details | Diff
Test cases that should compile with a patched compiler. (1.04 KB, text/plain)
2009-11-20 17:37 UTC, Chad Joan
Details
Things that should not compile with the patched compiler. (1.12 KB, application/octet-stream)
2009-11-20 17:38 UTC, Chad Joan
Details

Note You need to log in before you can comment on or make changes to this issue.
Description Chad Joan 2009-11-20 17:22:08 UTC
Created attachment 504 [details]
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.
Comment 1 Chad Joan 2009-11-20 17:26:44 UTC
Created attachment 505 [details]
Complete patch.
Comment 2 Chad Joan 2009-11-20 17:28:04 UTC
Created attachment 506 [details]
Patch for all source files only (excludes spec/doc changes). diff of ${dmd}/src
Comment 3 Chad Joan 2009-11-20 17:32:17 UTC
Created attachment 507 [details]
Patch for dmd compiler source files only. diff of ${dmd}/src/dmd
Comment 4 Chad Joan 2009-11-20 17:33:50 UTC
Created attachment 508 [details]
Patch for phobos source files only. diff of ${dmd}/src/phobos
Comment 5 Chad Joan 2009-11-20 17:34:31 UTC
Created attachment 509 [details]
Patch for druntime source files only. diff of ${dmd}/src/druntime
Comment 6 Chad Joan 2009-11-20 17:36:03 UTC
Created attachment 510 [details]
Patch for dmd docs only.  diff of ${dmd}/html
Comment 7 Chad Joan 2009-11-20 17:37:23 UTC
Created attachment 511 [details]
Test cases that should compile with a patched compiler.
Comment 8 Chad Joan 2009-11-20 17:38:09 UTC
Created attachment 512 [details]
Things that should not compile with the patched compiler.
Comment 9 bearophile_hugs 2010-06-19 13:07:18 UTC
See also bug 4349
Comment 10 bearophile_hugs 2012-05-27 05:44:10 UTC
Is it possible to close this bug now?
Comment 11 Chad Joan 2012-05-27 06:08:33 UTC
Yes, it can be closed.  Here ya go.
Comment 12 Walter Bright 2012-05-27 13:47:13 UTC
(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.
Comment 13 Chad Joan 2012-05-29 22:30:26 UTC
(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!