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

gen.dart

Index: gen.dart
===================================================================
--- gen.dart	(revision 910)
+++ gen.dart	(working copy)
@@ -865,7 +865,7 @@
   }
 
   /** Visits [body] without creating a new block for a [BlockStatement]. */
-  void visitStatementsInBlock(Statement body) {
+  bool visitStatementsInBlock(Statement body) {
     if (body is BlockStatement) {
       for (var stmt in body.body) {
         stmt.visit(this);
@@ -873,6 +873,7 @@
     } else {
       if (body != null) body.visit(this);
     }
+    return false;
   }
 
   _pushBlock([bool reentrant = false]) {
@@ -922,12 +923,13 @@
 
   // ******************* Statements *******************
 
-  void visitDietStatement(DietStatement node) {
+  bool visitDietStatement(DietStatement node) {
     var parser = new Parser(node.span.file, /*diet:*/false, node.span.start);
     visitStatementsInBlock(parser.block());
+    return false;
   }
 
-  void visitVariableDefinition(VariableDefinition node) {
+  bool visitVariableDefinition(VariableDefinition node) {
     var isFinal = false;
     // TODO(jimhug): Clean this up and share modifier parsing somewhere.
     if (node.modifiers != null && node.modifiers[0].kind == TokenKind.FINAL) {
@@ -960,9 +962,11 @@
       }
     }
     writer.writeln(';');
+    return false;
+
   }
 
-  void visitFunctionDefinition(FunctionDefinition node) {
+  bool visitFunctionDefinition(FunctionDefinition node) {
     var name = world.toJsIdentifier(node.name.name);
 
     var meth = _makeLambdaMethod(name, node);
@@ -970,9 +974,14 @@
     // TODO(jimhug): Pass js name into writeDefinition?
     var funcValue = _scope.create(name, meth.functionType, method.definition);
     meth.generator.writeDefinition(writer, null);
+    return false;
   }
 
-  void visitReturnStatement(ReturnStatement node) {
+  /**
+   * Returns true indicating that normal control-flow is interrupted by 
+   * this statement. (This could be a return, break, throw, or continue.)
+   */
+  bool visitReturnStatement(ReturnStatement node) {
     if (node.value == null) {
       writer.writeln('return;');
     } else {
@@ -981,9 +990,10 @@
       }
       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);
@@ -999,9 +1009,10 @@
         writer.writeln('throw ${rethrow.code};');
       }
     }
+    return true;
   }
 
-  void visitAssertStatement(AssertStatement node) {
+  bool visitAssertStatement(AssertStatement node) {
     // be sure to walk test for static checking even is asserts disabled
     var test = visitValue(node.test); // TODO(jimhug): check bool or callable.
     if (options.enableAsserts) {
@@ -1016,53 +1027,62 @@
       writer.writeln('\$assert(${test.code}, "${_escapeString(span.text)}",' +
         ' "${span.file.filename}", ${line + 1}, ${column + 1});');
     }
+    return false;
   }
 
-  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 exit1;
     writer.write('if (${test.code}) ');
-    node.trueBranch.visit(this);
+    exit1 = node.trueBranch.visit(this);

[jimhug, 2011/10/28 20:02:44]

I'd move the var decl to this line as I really like declaring and initializing
variables in one line.

     if (node.falseBranch != null) {
       writer.write('else ');
-      node.falseBranch.visit(this);
+      if (node.falseBranch.visit(this) && exit1) {
+        return true;
+      }
     }
+    return false;
   }
 
-  void visitWhileStatement(WhileStatement node) {
+  bool visitWhileStatement(WhileStatement node) {
     var test = visitBool(node.test);
     writer.write('while (${test.code}) ');
     _pushBlock(/*reentrant:*/true);
     node.body.visit(this);
     _popBlock();
+    return false;
   }
 
-  void visitDoStatement(DoStatement node) {
+  bool visitDoStatement(DoStatement node) {
     writer.write('do ');
     _pushBlock(/*reentrant:*/true);
     node.body.visit(this);
     _popBlock();
     var test = visitBool(node.test);
     writer.writeln('while (${test.code})');
+    return false;
   }
 
-  void visitForStatement(ForStatement node) {
+  bool visitForStatement(ForStatement node) {
     _pushBlock();
     writer.write('for (');
     if (node.init != null) node.init.visit(this);
@@ -1086,9 +1106,10 @@
     node.body.visit(this);
     _popBlock();
     _popBlock();
+    return false;
   }
 
-  void visitForInStatement(ForInStatement node) {
+  bool visitForInStatement(ForInStatement node) {
     // TODO(jimhug): visitValue and other cleanups here.
     var itemType = method.resolveType(node.item.type);
     var itemName = node.item.name.name;
@@ -1132,9 +1153,10 @@
     visitStatementsInBlock(node.body);
     writer.exitBlock('}');
     _popBlock();
+    return false;
   }
 
-  void visitTryStatement(TryStatement node) {
+  bool visitTryStatement(TryStatement node) {
     writer.enterBlock('try {');
     _pushBlock();
     visitStatementsInBlock(node.body);
@@ -1229,9 +1251,10 @@
 
     // Close the try-catch-finally
     writer.exitBlock('}');

[jimhug, 2011/10/28 20:02:44]

Add a TODO: This could be more precise by combining all the different paths
here.  -i.e. if there is a finally block with a return at the end then this can
return true, similarly if all blocks have a return at the end then the same
holds.

+    return false;
   }
 
-  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}) {');
@@ -1254,34 +1277,56 @@
         }
       }
       writer.enterBlock('');
-      for (var stmt in case_.statements) {
-        stmt.visit(this);
+      bool caseExits = false;
+      for (int i = 0; i < case_.statements.length; i++) {
+        var stmt = case_.statements[i];
+        caseExits = stmt.visit(this);
+        if (stmt != case_.statements[case_.statements.length - 1] && caseExits) {
+          world.warning('unreachable code', case_.statements[i + 1].span);

[jimhug, 2011/10/28 20:02:44]

Don't worry about it in this checkin - but for a future TODO - this will
generate a lot of errors if I have:
  case 0:
    return 1;
    a;
    b;
    c;

As written, this will give me three errors - and I'd prefer to just get the
error on a.

+        }
       }
-      if (case_ != node.cases[node.cases.length - 1]) {
+      if (case_ != node.cases[node.cases.length - 1] && !caseExits) {
         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 for correct
+        // output when constructors are fixed. 
+        writer.writeln('\$throw(new FallThroughError("${span.file.filename}",' +
+            ' ${span.file.getLine(span.start)}))');
       }
       writer.exitBlock('');
       _popBlock();
     }
     writer.exitBlock('}');
+    // TODO(efortuna): When we are passing more information back about
+    // control flow by returning something other than bool, return true for the
+    // cases where every branch of the switch statement ends with a return 
+    // statement.

[jimhug, 2011/10/28 20:02:44]

Nice TODO.

+    return false;
   }
-
-  void visitBlockStatement(BlockStatement node) {
+  
+  bool visitBlockStatement(BlockStatement node) {
     _pushBlock();
     writer.enterBlock('{');
-    for (var stmt in node.body) {
-      stmt.visit(this);
+    var exits = false;
+    for (int i = 0; i < node.body.length; i++) {
+      var stmt = node.body[i];
+      exits = stmt.visit(this);
+      if (exits && stmt != node.body[node.body.length - 1]) {

[jimhug, 2011/10/28 20:02:44]

TODO: Again, not for this checkin - but the same statement as above applies here
for getting only one error.  Now that I think about it, this code should
probably be factored out to be shared there - as well as by
visitStatementsInBlock as well - where you are currently missing the error
message.

+        world.warning('unreachable code', node.body[i + 1].span);
+      }
     }
     writer.exitBlock('}');
     _popBlock();
+    return exits;
   }
 
-  void visitLabeledStatement(LabeledStatement node) {
+  bool visitLabeledStatement(LabeledStatement node) {
     writer.writeln('${node.name.name}:');
     node.body.visit(this);
+    return false;
   }
 
-  void visitExpressionStatement(ExpressionStatement node) {
+  bool visitExpressionStatement(ExpressionStatement node) {
     if (node.body is VarExpression || node.body is ThisExpression) {
       // TODO(jmesserly): this is a "warning" but not a "type warning",
       // Is that okay? We have a similar issue around unreachable code warnings.
@@ -1289,10 +1334,12 @@
     }
     var value = visitVoid(node.body);
     writer.writeln('${value.code};');
+    return false;
   }
 
-  void visitEmptyStatement(EmptyStatement node) {
+  bool visitEmptyStatement(EmptyStatement node) {
     writer.writeln(';');
+    return false;
   }
 
   _checkNonStatic(Node node) {
@@ -1783,7 +1830,7 @@
     return m.invoke(parent, node, null, _makeArgs(node.arguments));
   }
 
-  void visitListExpression(ListExpression node) {
+  visitListExpression(ListExpression node) {
     // TODO(jimhug): Use node.type or other type inference here.
     var argsCode = [];
     var argValues = [];
@@ -1811,7 +1858,7 @@
     return new Value(world.listType, code);
   }
 
-  void visitMapExpression(MapExpression node) {
+  visitMapExpression(MapExpression node) {
     // Ensure that Map factory is intialized - pseudo-call needed members.
     var mapImplType = parent.useMapFactory();