Throw FallThroughError when we fall through a case in a switch statement. Fixes

gen.dart

Index: gen.dart
===================================================================
--- gen.dart	(revision 858)
+++ gen.dart	(working copy)
@@ -1072,15 +1072,20 @@
     meth.generator.writeDefinition(writer, null);
   }
 
-  void visitReturnStatement(ReturnStatement node) {
+  /**
+   * Returns true indicating that code will return after executing this

[jimhug, 2011/10/28 14:33:34]

Returns true indicating that normal control-flow is interrupted by this
statement.  This could be either a return, throw, break or continue.  The only
thing this guarantees is that any statement in a block that follows a statement
returning true can never be executed.

+   * statement.
+   */
+  bool visitReturnStatement(ReturnStatement node) {
     if (node.value == null) {
       writer.writeln('return;');
     } else {
       writer.writeln('return ${visitValue(node.value).code};');
     }
+    return true;
   }
 
-  void visitThrowStatement(ThrowStatement node) {
+  bool visitThrowStatement(ThrowStatement node) {
     // Dart allows throwing anything, just like JS
     if (node.value != null) {
       var value = visitValue(node.value);
@@ -1096,6 +1101,7 @@
         writer.writeln('throw ${rethrow.code};');
       }
     }
+    return true;
   }
 
   void visitAssertStatement(AssertStatement node) {

[jimhug, 2011/10/28 14:33:34]

For type safety, you need to update all of the other visit*Statement nodes to
have a return type of bool and to return false.

@@ -1115,31 +1121,35 @@
     }
   }
 
-  void visitBreakStatement(BreakStatement node) {
+  bool visitBreakStatement(BreakStatement node) {
     // TODO(jimhug): Lots of flow error checking here and below.
     if (node.label == null) {
       writer.writeln('break;');
     } else {
       writer.writeln('break ${node.label.name};');
     }
+    return true;
   }
 
-  void visitContinueStatement(ContinueStatement node) {
+  bool visitContinueStatement(ContinueStatement node) {
     if (node.label == null) {
       writer.writeln('continue;');
     } else {
       writer.writeln('continue ${node.label.name};');
     }
+    return true;
   }
 
-  void visitIfStatement(IfStatement node) {
+  bool visitIfStatement(IfStatement node) {
     var test = visitBool(node.test);
+    var return1, return2;

[jimhug, 2011/10/28 14:33:34]

I'd suggest the name "exit" here instead of return to capture the control-flow
exiting - but not-necessarily a return.

Also, you need to properly initialize the return2 - or you might be doing an &&
with null and who knows what that would result in.

I'd rewrite this to remove the return2, and make the other changes noted below.

[Emily Fortuna, 2011/10/28 17:23:23]

I tested this out before I wrote this, and if(null) behaves like false, and if
(null && true) == false, if (3) == true, and if (0) == false but I wasn't
certain that this was actually how Dart was intended to be used versus being
sneaky and taking advantage of the less implemented/spec'd out corners of dart.
I'll fix my code. :-)

On 2011/10/28 14:33:34, jimhug wrote:

     writer.write('if (${test.code}) ');
-    node.trueBranch.visit(this);
+    return1 = node.trueBranch.visit(this);
     if (node.falseBranch != null) {
       writer.write('else ');
-      node.falseBranch.visit(this);
+      return2 = node.falseBranch.visit(this);

[jimhug, 2011/10/28 14:33:34]

if (node.falseBranch.visit(this) && exit1) return true;

     }
+    return (return1 && return2);

[jimhug, 2011/10/28 14:33:34]

return false;

   }
 
   void visitWhileStatement(WhileStatement node) {
@@ -1328,10 +1338,11 @@
     writer.exitBlock('}');
   }
 
-  void visitSwitchStatement(SwitchStatement node) {
+  bool visitSwitchStatement(SwitchStatement node) {
     // TODO(jimhug): Lots of negative tests to check for.
     var test = visitValue(node.test);
     writer.enterBlock('switch (${test.code}) {');
+    bool allCasesReturn = true;

[jimhug, 2011/10/28 14:33:34]

I don't think that you can use this effectively here - see your note below about
the difference between return and break.  For now, visitSwitchStatement needs to
return false - because unless every path actually returns or throws then there
is no guarantee that this will interrupt normal control flow.

     for (var case_ in node.cases) {
       if (case_.label != null) {
         world.error('unimplemented: labeled case statement', case_.span);
@@ -1351,26 +1362,36 @@
         }
       }
       writer.enterBlock('');
+      bool caseReturns;

[jimhug, 2011/10/28 14:33:34]

This needs to be initialized to false.

       for (var stmt in case_.statements) {
-        stmt.visit(this);
+        caseReturns = stmt.visit(this);

[jimhug, 2011/10/28 14:33:34]

This should have the same checking for previous state that is needed in
visitBlockStatement to catch:

case 1:
  foo();
  break;
  return 42;

       }
-      if (case_ != node.cases[node.cases.length - 1]) {
+      if (case_ != node.cases[node.cases.length - 1] && !caseReturns) {
         var span = case_.statements[case_.statements.length - 1].span;
+        //TODO(efortuna): This error message isn't 100% correct because 
+        // constructors are not being built correctly. Check back when I've
+        // fixed constructors. 
+        writer.writeln('\$throw(new FallThroughError("${span.file.filename}",' +
+            ' ${span.file.getLine(span.start)}))');
+        allCasesReturn = false;

[jimhug, 2011/10/28 14:33:34]

See above on allCasesReturn - but this shouldn't be set here.

       }
       writer.exitBlock('');
       _popBlock();
     }
     writer.exitBlock('}');
+    return allCasesReturn;

[jimhug, 2011/10/28 14:33:34]

return false;

   }
 
-  void visitBlockStatement(BlockStatement node) {
+  bool visitBlockStatement(BlockStatement node) {
     _pushBlock();
     writer.enterBlock('{');
+    var returns;

[jimhug, 2011/10/28 14:33:34]

Needs to be initialized.

     for (var stmt in node.body) {
-      stmt.visit(this);
+      returns = stmt.visit(this);

[Emily Fortuna, 2011/10/27 22:11:24]

I should add a warning here if we have a return statement here, followed by
other code. However, to do this, our system of returning just a true on both
return and break would have to provide more information. I was initially
thinking of making "visit" return some "ReturnType" enum/int, but this would be
a fairly substantial change, so I wanted to get your thoughts first. 

[jimhug, 2011/10/28 14:33:34]

I think that your warning should work here just fine - this exactly matches the
current guarantee.  For now, I think this is the right level to do this at.  In
the future I think that we will need to return some sort of FlowState object
from all of these visits - but let's see if we can put that off a little bit
longer.

On 2011/10/27 22:11:24, efortuna wrote:

     }
     writer.exitBlock('}');
     _popBlock();
+    return returns;
   }
 
   void visitLabeledStatement(LabeledStatement node) {