In analyzer, catch use of continue and break in inappropriate places

In analyzer, catch use of continue and break in inappropriate places
(outside of a loop or in the case of break, with no referenced label)

See http://code.google.com/p/dart/issues/detail?id=1658
and http://code.google.com/p/dart/issues/detail?id=2441

Committed: https://code.google.com/p/dart/source/detail?r=8467

[Brian Wilkerson, 2012-06-08 18:49:05]

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/DartParser.java:229: private
boolean inBreakableStatement = false;
Looking at the spec, it seems that there is a difference between a "breakable"
statement and a "continuable" statement. I think we need to maintain both pieces
of state.

Both flags need to be reset to false while processing a nested function.

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/DartParser.java:3699: private
DartStatement parseBreakableStatement() {
This is fine for both "breakable" and "continuable" statement, but I think we
also need to change parseStatement() to mark a statement as "breakable" if there
is a label before the statement.

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/javatests/...
File compiler/javatests/com/google/dart/compiler/parser/ClassesInterfaces.dart
(right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/parser/ClassesInterfaces.dart:1: //
Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
nit: copyright year

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/javatests/...
File compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java (right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java:545: "    C:
continue C;", // bad
Perhaps add a nested use, such as

  L2: while (true) { int f() { break L2; }; } // bad

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/co19/co19-com...
File tests/co19/co19-compiler.status (right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/co19/co19-com...
tests/co19/co19-compiler.status:173: #Language/03_Overview/2_Privacy_A01_t19:
Fail # co19 issue: break w/o loop
Is there an issue number?

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/co19/co19-com...
tests/co19/co19-compiler.status:243: Language/11/Statements/11_Labels_A03_t05:
Fail # Issue 2441
"11/Statements"?

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/language/labe...
File tests/language/label_test.dart (right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/language/labe...
tests/language/label_test.dart:1: // Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
nit: copyright year

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/language/labe...
tests/language/label_test.dart:224: while (true) {
Looking at the spec, I don't see where the original version of this code is
disallowed. (That is, I don't see why we need the while loop wrapped around it.)

[zundel, 2012-06-08 20:15:08]

I hope this is clearer.  Your suggested test case revealed something I hadn't
handled.

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/java/com/g...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/DartParser.java:229: private
boolean inBreakableStatement = false;
On 2012/06/08 18:49:05, Brian Wilkerson wrote:
> Looking at the spec, it seems that there is a difference between a "breakable"
> statement and a "continuable" statement. I think we need to maintain both
pieces
> of state.
> 
> Both flags need to be reset to false while processing a nested function.

I realized this halfway through the patch.  I had two different state variables
at first, but there is already handling for labels not matching up, so I'm
letting all breaks with labels on them fall through without comment and allow
the resolver to catch the problem.  This is better, I think, because a break
with no label is still not valid, so setting the 'isBreakable' flag when you see
a label will falsely ignore problems in some cases.

I did some renaming and added a comment above.

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/java/com/g...
compiler/java/com/google/dart/compiler/parser/DartParser.java:3699: private
DartStatement parseBreakableStatement() {
On 2012/06/08 18:49:05, Brian Wilkerson wrote:
> This is fine for both "breakable" and "continuable" statement, but I think we
> also need to change parseStatement() to mark a statement as "breakable" if
there
> is a label before the statement.

Renamed, this just records the fact that your in a loop or case stmt and it is
up to the break and continue parsers to react accordingly.

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/javatests/...
File compiler/javatests/com/google/dart/compiler/parser/ClassesInterfaces.dart
(right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/parser/ClassesInterfaces.dart:1: //
Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
On 2012/06/08 18:49:05, Brian Wilkerson wrote:
> nit: copyright year

Done.

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/javatests/...
File compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java (right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java:545: "    C:
continue C;", // bad
On 2012/06/08 18:49:05, Brian Wilkerson wrote:
> Perhaps add a nested use, such as
> 
>   L2: while (true) { int f() { break L2; }; } // bad

that case should be taken care of by the resolver.  but the unlabeled 'break'
should be caught in the parser, I think.

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/co19/co19-com...
File tests/co19/co19-compiler.status (right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/co19/co19-com...
tests/co19/co19-compiler.status:173: #Language/03_Overview/2_Privacy_A01_t19:
Fail # co19 issue: break w/o loop
On 2012/06/08 18:49:05, Brian Wilkerson wrote:
> Is there an issue number?
No, PBKAC

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/co19/co19-com...
tests/co19/co19-compiler.status:243: Language/11/Statements/11_Labels_A03_t05:
Fail # Issue 2441
On 2012/06/08 18:49:05, Brian Wilkerson wrote:
> "11/Statements"?

removed

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/language/labe...
File tests/language/label_test.dart (right):

https://chromiumcodereview.appspot.com/10534065/diff/2001/tests/language/labe...
tests/language/label_test.dart:224: while (true) {
On 2012/06/08 18:49:05, Brian Wilkerson wrote:
> Looking at the spec, I don't see where the original version of this code is
> disallowed. (That is, I don't see why we need the while loop wrapped around
it.)

You are right. I had coded it incorrectly at first then when I found more
instances of this I went back and carefully re-read the spec and found out I was
wrong.

Reverted.

[Brian Wilkerson, 2012-06-08 20:22:52]

LGTM

[scheglov, 2012-06-09 19:15:53]

LGTM

https://chromiumcodereview.appspot.com/10534065/diff/8002/compiler/javatests/...
File compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java (right):

https://chromiumcodereview.appspot.com/10534065/diff/8002/compiler/javatests/...
compiler/javatests/com/google/dart/compiler/parser/SyntaxTest.java:536:
parseUnit("phony_lone_super_expression1.dart",
super expression?