Throw FallThroughError when we fall through a case in a switch statement. Fixes (issue 8343037)
efor...@google.com
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.
Description:
Throw FallThroughError when we fall through a case in a switch statement.
Fixes
two tests.
Please review this at http://codereview.chromium.org/8343037/
SVN Base: http://dart.googlecode.com/svn/experimental/frog/
Affected files:
M frogsh
M gen.dart
M lib/corelib.dart
M tests/frog/frog.status
M tree.dart
Index: frogsh
===================================================================
--- frogsh (revision 858)
+++ frogsh (working copy)
@@ -283,6 +283,11 @@
TypeError.prototype.toString = function() {
return ("Failed type check: type " + this.srcType + " is not assignable
to type ") + ("" + this.dstType + " of " + this.dstName + " in " + this.url
+ " at line ") + ("" + this.line + ", column " + this.column + ".");
}
+// ********** Code for FallThroughError **************
+function FallThroughError() {}
+FallThroughError.prototype.toString = function() {
+ return ("Switch case fall-through in " + this.url + " at line " +
this.line + ".");
+}
// ********** Code for Object **************
Object.prototype.get$dynamic = function() {
return this;
@@ -7272,6 +7277,7 @@
else {
this.writer.writeln(('return ' + this.visitValue(node.value).code
+ ';'));
}
+ return true;
}
MethodGenerator.prototype.visitThrowStatement = function(node) {
if (node.value != null) {
@@ -7288,6 +7294,7 @@
this.writer.writeln(('throw ' + rethrow.code + ';'));
}
}
+ return true;
}
MethodGenerator.prototype.visitAssertStatement = function(node) {
var test = this.visitValue(node.test);
@@ -7309,6 +7316,7 @@
else {
this.writer.writeln(('break ' + node.label.name + ';'));
}
+ return true;
}
MethodGenerator.prototype.visitContinueStatement = function(node) {
if (node.label == null) {
@@ -7317,15 +7325,18 @@
else {
this.writer.writeln(('continue ' + node.label.name + ';'));
}
+ return true;
}
MethodGenerator.prototype.visitIfStatement = function(node) {
var test = this.visitBool(node.test);
+ var return1, return2;
this.writer.write(('if (' + test.code + ') '));
- node.trueBranch.visit(this);
+ return1 = node.trueBranch.visit(this);
if (node.falseBranch != null) {
this.writer.write('else ');
- node.falseBranch.visit(this);
+ return2 = node.falseBranch.visit(this);
}
+ return (return1 && return2);
}
MethodGenerator.prototype.visitWhileStatement = function(node) {
var test = this.visitBool(node.test);
@@ -7482,6 +7493,7 @@
MethodGenerator.prototype.visitSwitchStatement = function(node) {
var test = this.visitValue(node.test);
this.writer.enterBlock(('switch (' + test.code + ') {'));
+ var allCasesReturn = true;
var $list = node.cases;
for (var $i = 0;$i < $list.length; $i++) {
var case_ = $list.$index($i);
@@ -7493,6 +7505,9 @@
i < case_.cases.length; i++) {
var expr = case_.cases.$index(i);
if (expr == null) {
+ if (i < case_.cases.length - 1) {
+ world.error('default clause must be the last case',
case_.get$span());
+ }
this.writer.writeln('default:');
}
else {
@@ -7501,26 +7516,35 @@
}
}
this.writer.enterBlock('');
+ var caseReturns;
var $list0 = case_.statements;
for (var $i0 = case_.statements.iterator(); $i0.hasNext(); ) {
var stmt = $i0.next();
- stmt.visit(this);
+ caseReturns = stmt.visit(this);
}
+ if ($ne(case_, node.cases.$index(node.cases.length - 1))
&& !caseReturns) {
+ var span = case_.statements.$index(case_.statements.length -
1).get$span();
+ this.writer.writeln(('\$throw(new FallThroughError("' +
span.file.filename + '",') + (' ' + span.file.getLine(span.start) + '))'));
+ allCasesReturn = false;
+ }
this.writer.exitBlock('');
this._popBlock();
}
this.writer.exitBlock('}');
+ return allCasesReturn;
}
MethodGenerator.prototype.visitBlockStatement = function(node) {
this._pushBlock(false);
this.writer.enterBlock('{');
+ var returns;
var $list = node.body;
for (var $i = 0;$i < $list.length; $i++) {
var stmt = $list.$index($i);
- stmt.visit(this);
+ returns = stmt.visit(this);
}
this.writer.exitBlock('}');
this._popBlock();
+ return returns;
}
MethodGenerator.prototype.visitLabeledStatement = function(node) {
this.writer.writeln(('' + node.name.name + ':'));
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
+ * 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) {
@@ -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;
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);
}
+ return (return1 && return2);
}
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;
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;
for (var stmt in case_.statements) {
- stmt.visit(this);
+ caseReturns = stmt.visit(this);
}
- 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;
}
writer.exitBlock('');
_popBlock();
}
writer.exitBlock('}');
+ return allCasesReturn;
}
- void visitBlockStatement(BlockStatement node) {
+ bool visitBlockStatement(BlockStatement node) {
_pushBlock();
writer.enterBlock('{');
+ var returns;
for (var stmt in node.body) {
- stmt.visit(this);
+ returns = stmt.visit(this);
}
writer.exitBlock('}');
_popBlock();
+ return returns;
}
void visitLabeledStatement(LabeledStatement node) {
Index: lib/corelib.dart
===================================================================
--- lib/corelib.dart (revision 858)
+++ lib/corelib.dart (working copy)
@@ -92,7 +92,14 @@
final String dstName;
}
class FallThroughError {
- const FallThroughError() : super();
+ final String url;
+ final int line;
+
+ const FallThroughError(this.url, this.line);
+
+ String toString() {
+ return "Switch case fall-through in $url at line $line.";
+ }
}
// Dart core library.
Index: tests/frog/frog.status
===================================================================
--- tests/frog/frog.status (revision 858)
+++ tests/frog/frog.status (working copy)
@@ -44,7 +44,6 @@
ConstructorRedirect5NegativeTest: Fail # error detected, but code
unreachable
ConstructorSetterNegativeTest: Fail # we were passing this test for the
wrong reason
CTConst2Test: Fail # newly added on 10/17
-EmptyBlockCaseTest: Fail
EmptyBodyMemberNegativeTest: Fail
ExampleConstructorTest: Fail
ExecuteFinally6Test: Fail
@@ -172,7 +171,6 @@
SuperNegativeTest: Fail
SuperOperatorTest: Fail
SuperTest: Fail
-SwitchFallthruTest: Fail
SwitchLabelTest: Fail
TryCatch9NegativeTest: Fail
TypeDartcTest: Fail
Index: tree.dart
===================================================================
--- tree.dart (revision 858)
+++ tree.dart (working copy)
@@ -11,7 +11,7 @@
Node(this.span) {}
/** Classic double-dispatch visitor for implementing passes. */
- abstract void visit(TreeVisitor visitor);
+ abstract visit(TreeVisitor visitor);
/** A multiline string showing the node and its children. */
String toDebugString() {
jim...@google.com
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.
efor...@google.com
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. :-)
Jim Hugunin
efor...@google.com
jim...@google.com
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.