www.digitalmars.com         C & C++   DMDScript  

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

reply d-bugmail puremagic.com writes:
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 puremagic.com
        ReportedBy: chadjoan gmail.com



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: -------
Nov 20 2009
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536




Created an attachment (id=505)
Complete patch.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536




Created an attachment (id=506)
Patch for all source files only (excludes spec/doc changes).

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536




Created an attachment (id=507)
Patch for dmd compiler source files only. diff of ${dmd}/src/dmd

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536




Created an attachment (id=508)
Patch for phobos source files only. diff of ${dmd}/src/phobos

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536




Created an attachment (id=509)
Patch for druntime source files only. diff of ${dmd}/src/druntime

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536




Created an attachment (id=510)
Patch for dmd docs only.  diff of ${dmd}/html

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536




Created an attachment (id=511)
Test cases that should compile with a patched compiler.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536




Created an attachment (id=512)
Things that should not compile with the patched compiler.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536


Chad Joan <chadjoan gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------

              patch|                            |


-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536


Chad Joan <chadjoan gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------

        description|only (excludes spec/doc     |only (excludes spec/doc
                   |changes).                   |changes). diff of
                   |                            |${dmd}/src


-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 20 2009
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536


bearophile_hugs eml.cc changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bearophile_hugs eml.cc



See also bug 4349

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 19 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536




Is it possible to close this bug now?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 27 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536


Chad Joan <chadjoan gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |WONTFIX



Yes, it can be closed.  Here ya go.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 27 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla digitalmars.com



13:47:13 PDT ---

 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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 27 2012
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=3536






 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! -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 29 2012