ExitDetector: Examine continue when determining a 'do'

ExitDetector: Examine continue when determining a 'do'

BUG=https://github.com/dart-lang/sdk/issues/26786
R=brianwilkerson@google.com, paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/8a710c8589170a7524890009a1839fa9c0ebcbb0

[srawlins, 2016-06-28 19:02:20]

srawlins@google.com changed reviewers:
+ brianwilkerson@google.com, keertip@google.com, paulberry@google.com

[srawlins, 2016-06-28 19:02:22]

The ExitDetector gets longer...

no performance concerns though...

[Paul Berry, 2016-06-28 19:17:41]

https://codereview.chromium.org/2102223002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2102223002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:3850: bool outerContinueValue =
_enclosingBlockContainsContinue;
I don't think this method should be modified.  Switch statements only affect the
target of "break" statements, not "continue" statements.  So for instance I
think with your code as written, we would incorrectly classify the following
code as exiting:

do {
  switch (x) {
    case 0:
      continue;
  }
  throw 'foo';
} while (false);

(Assuming I'm right about this, we should add a test case to verify that the
above code is handled correctly).

[srawlins, 2016-06-28 20:00:55]

https://codereview.chromium.org/2102223002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2102223002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:3850: bool outerContinueValue =
_enclosingBlockContainsContinue;
On 2016/06/28 19:17:41, Paul Berry wrote:
> I don't think this method should be modified.  Switch statements only affect
the
> target of "break" statements, not "continue" statements.  So for instance I
> think with your code as written, we would incorrectly classify the following
> code as exiting:
> 
> do {
>   switch (x) {
>     case 0:
>       continue;
>   }
>   throw 'foo';
> } while (false);
> 
> (Assuming I'm right about this, we should add a test case to verify that the
> above code is handled correctly).

Done. With testcase.

[Paul Berry, 2016-06-28 20:35:58]

lgtm

[Brian Wilkerson, 2016-06-28 20:47:33]

https://codereview.chromium.org/2102223002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/2102223002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:3616:
_enclosingBlockContainsContinue = true;
This is ignoring the presence or absence of a label. It seems like the condition
should apply to enclosing loops if the label is associated with that loop and
not just be discarded when we exit the current loop, such as:

foo: while (...) {
  while (...) {
    continue foo;
  }
}

but maybe it doesn't matter.

Also, I noticed that you deleted the visitSwitchCase (or Statement) code, but
the spec says:

Let sc be a continue statement. If sc is of the form continue L;, then let sE
be the the innermost labeled do (17.8), for (17.6) or while (17.7) statement or
case clause with label L enclosing sc.

Notice the last line: if the continue has a label it can apply to a case clause.
Not sure how that ought to affect the logic here, but some test cases are in
order.

[srawlins, 2016-06-28 23:10:59]

I added more tests.

Brian: My implementation was weird; I didnt think about how saving the current
value of _enclosingBlockContainsContinue is a process only for when you want to
check the new value, and then restore the old value. So this implementation has
changed.

As far as labels go, I think there is a more sophisticated way to handle this,
but for now, this is fairly accurate. Let me know if you want more tests.

[Brian Wilkerson, 2016-06-29 13:59:31]

lgtm, but it would be good to capture the failing cases for future reference.
You can do that by creating a method that is prefixed by "fail_". For example,
here's a test case illustrating the use of a labeled continue in a switch:

void f(int i) {
  switch (i) {
    zero: case 0:
      return;
    case 1:
      continue zero;
    default:
      return;
  }
  // Cannot reach this point.
}

[Paul Berry, 2016-06-29 14:08:27]

On 2016/06/29 13:59:31, Brian Wilkerson wrote:
> lgtm, but it would be good to capture the failing cases for future reference.
> You can do that by creating a method that is prefixed by "fail_". For example,
> here's a test case illustrating the use of a labeled continue in a switch:
> 
> void f(int i) {
>   switch (i) {
>     zero: case 0:
>       return;
>     case 1:
>       continue zero;
>     default:
>       return;
>   }
>   // Cannot reach this point.
> }

FYI, as of 24daee5bd1235389221dc1c74fe77151f32a0db2, the preferred way to do
this is to keep the test named "test_", but add the annotation "@failingTest"
above it.

[srawlins, 2016-06-30 17:18:01]

Description was changed from

==========
ExitDetector: Examine continue when determining a 'do'

BUG=https://github.com/dart-lang/sdk/issues/26786
==========

to

==========
ExitDetector: Examine continue when determining a 'do'

BUG=https://github.com/dart-lang/sdk/issues/26786
R=brianwilkerson@google.com, paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/8a710c8589170a7524890009a1839fa9c0ebcbb0
==========

[srawlins, 2016-06-30 17:18:03]

Committed patchset #5 (id:80001) manually as
8a710c8589170a7524890009a1839fa9c0ebcbb0 (presubmit successful).