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

gen.dart

 // Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * Top level generator object for writing code and keeping track of
  * dependencies.
  *
  * Should have two compilation models, but only one implemented so far.
  *
@@ skipping 1054 matching lines @@
   void visitFunctionDefinition(FunctionDefinition node) {
     var name = world.toJsIdentifier(node.name.name);
 
     var meth = _makeLambdaMethod(name, node);
 
     // TODO(jimhug): Pass js name into writeDefinition?
     var funcValue = _scope.create(name, meth.functionType, method.definition);
     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);
       // Ensure that we generate a toString() method for things that we throw
       value.invoke(parent, 'toString', node, Arguments.EMPTY);
       writer.writeln('\$throw(${value.code});');
     } else {
       var rethrow = _scope.getRethrow();
       if (rethrow == null) {
         world.error('rethrow outside of catch', node.span);
       } else {
         // Use a normal throw instead of $throw so we don't capture a new stack
         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.

     // 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) {
       var err = world.corelib.types['AssertError'];
       err.markUsed();
       parent.genMethod(err.getConstructor(''));
       parent.genMethod(err.members['toString']);
       var span = node.test.span;
 
       var line = span.file.getLine(span.start);
       var column = span.file.getColumn(line, span.start);
       writer.writeln('\$assert(${test.code}, "${_escapeString(span.text)}",' +
         ' "${span.file.filename}", ${line + 1}, ${column + 1});');
     }
   }
 
-  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) {
     var test = visitBool(node.test);
     writer.write('while (${test.code}) ');
     _pushBlock(/*reentrant:*/true);
     node.body.visit(this);
     _popBlock();
   }
 
@@ skipping 168 matching lines @@
       writer.nextBlock('} finally {');
       _pushBlock();
       visitStatementsInBlock(node.finallyBlock);
       _popBlock();
     }
 
     // Close the try-catch-finally
     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);
       }
       _pushBlock();
       for (int i=0; i < case_.cases.length; i++) {
         var expr = case_.cases[i];
         if (expr == null) {
           // Default can only be the last case.
           if (i < case_.cases.length - 1) {
             world.error('default clause must be the last case', case_.span);
           }
           writer.writeln('default:');
         } else {
           var value = visitValue(expr);
           writer.writeln('case ${value.code}:');
         }
       }
       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) {
     writer.writeln('${node.name.name}:');
     node.body.visit(this);
   }
 
   void visitExpressionStatement(ExpressionStatement node) {
     if (node.body is VarExpression || node.body is ThisExpression) {
       // TODO(jmesserly): this is a "warning" but not a "type warning",
@@ skipping 758 matching lines @@
 
   static removeTrailingNulls(List<Value> argsCode) {
     // We simplify calls with null defaults by relying on JS and our
     // choice to make undefined === null for Dart generated code. This helps
     // and ensures correct defaults values for native calls.
     while (argsCode.length > 0 && argsCode.last() == 'null') {
       argsCode.removeLast();
     }
   }
 }