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

Throw FallThroughError when we fall through a case in a switch statement. Fixes
two tests.
Committed: https://code.google.com/p/dart/source/detail?r=918

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

http://codereview.chromium.org/8343037/diff/2001/gen.dart
File gen.dart (right):

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1390
gen.dart:1390: returns = stmt.visit(this);
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]

This is moving in the right direction!  I just have a few things for you to fix
before this can go in and we can knock down those two troubling test cases.

http://codereview.chromium.org/8343037/diff/2001/gen.dart
File gen.dart (right):

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1076
gen.dart:1076: * Returns true indicating that code will return after executing
this
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.

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1107
gen.dart:1107: void visitAssertStatement(AssertStatement node) {
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.

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1145
gen.dart:1145: var return1, return2;
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.

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1150
gen.dart:1150: return2 = node.falseBranch.visit(this);
if (node.falseBranch.visit(this) && exit1) return true;

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1152
gen.dart:1152: return (return1 && return2);
return false;

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1345
gen.dart:1345: bool allCasesReturn = true;
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.

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1365
gen.dart:1365: bool caseReturns;
This needs to be initialized to false.

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1367
gen.dart:1367: caseReturns = stmt.visit(this);
This should have the same checking for previous state that is needed in
visitBlockStatement to catch:

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

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1376
gen.dart:1376: allCasesReturn = false;
See above on allCasesReturn - but this shouldn't be set here.

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1382
gen.dart:1382: return allCasesReturn;
return false;

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1388
gen.dart:1388: var returns;
Needs to be initialized.

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1390
gen.dart:1390: returns = stmt.visit(this);
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:
> 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.

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

http://codereview.chromium.org/8343037/diff/2001/gen.dart
File gen.dart (right):

http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1145
gen.dart:1145: var return1, return2;
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:
> 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 18:39:08]

PTAL.

On 2011/10/28 17:23:23, efortuna wrote:
> http://codereview.chromium.org/8343037/diff/2001/gen.dart
> File gen.dart (right):
> 
> http://codereview.chromium.org/8343037/diff/2001/gen.dart#newcode1145
> gen.dart:1145: var return1, return2;
> 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:
> > 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.

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

LGTM!

There are some TODOs in here - but those can easily wait for a future CL.  As
far as your fix to the parser for labeled case statements - is this covered by
an existing test case?  If not we should file an issue that we need a test here
for the future.

http://codereview.chromium.org/8343037/diff/5002/frogsh
File frogsh (right):

http://codereview.chromium.org/8343037/diff/5002/frogsh#newcode2567
frogsh:2567: $throw(new FallThroughError("./leg/scanner/scanner.dart", 389))
Please send Peter Ahe and email pointing this change out to him and explaining
the reasons for it.  He is very carefully tracking the performance of this code
and it is vaguely possible that he will see a perf regression.  I think this is
clearly the right choice for correctness now - and more analysis to reduce code
size later - but I'd prefer that Peter not be surprised.

http://codereview.chromium.org/8343037/diff/5002/gen.dart
File gen.dart (right):

http://codereview.chromium.org/8343037/diff/5002/gen.dart#newcode1056
gen.dart:1056: exit1 = node.trueBranch.visit(this);
I'd move the var decl to this line as I really like declaring and initializing
variables in one line.

http://codereview.chromium.org/8343037/diff/5002/gen.dart#newcode1253
gen.dart:1253: writer.exitBlock('}');
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.

http://codereview.chromium.org/8343037/diff/5002/gen.dart#newcode1285
gen.dart:1285: world.warning('unreachable code', case_.statements[i + 1].span);
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.

http://codereview.chromium.org/8343037/diff/5002/gen.dart#newcode1303
gen.dart:1303: // statement.
Nice TODO.

http://codereview.chromium.org/8343037/diff/5002/gen.dart#newcode1314
gen.dart:1314: if (exits && stmt != node.body[node.body.length - 1]) {
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.

http://codereview.chromium.org/8343037/diff/5002/parser.dart
File parser.dart (right):

http://codereview.chromium.org/8343037/diff/5002/parser.dart#newcode632
parser.dart:632: label = identifier();
Whoa!  How was this working before?

http://codereview.chromium.org/8343037/diff/5002/tree.dart
File tree.dart (left):

http://codereview.chromium.org/8343037/diff/5002/tree.dart#oldcode14
tree.dart:14: abstract void visit(TreeVisitor visitor);
Good catch.