Do not error on dead, mandated statements at end of switch cases

Do not error on dead, mandated statements at end of switch cases

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

Committed: https://github.com/dart-lang/sdk/commit/6e58ff0575e57a68e73a2fc2438c9e713810cb25

[srawlins, 2016-06-14 18:32:17]

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

[Paul Berry, 2016-06-14 18:39:27]

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

https://codereview.chromium.org/2064203002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:2073: if (nextStatement is
BreakStatement ||
IMHO this is overly permissive.  The important case we want to allow is a
mandated "break".  If someone puts some other statement at the end of a switch
case (a return, continue, return, or throw), then I think we should go ahead and
report a hint, because in those other cases it's highly likely that the user has
made a mistake.

https://codereview.chromium.org/2064203002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/non_hint_code_test.dart (right):

https://codereview.chromium.org/2064203002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/non_hint_code_test.dart:203: break;
Please also add tests for the other kind of statements you're checking for
(continue, return, throw).

(Assuming you take my previous suggestion and make those statements generate
hints, then the additional tests should of course go in hint_code_test.dart).

[Brian Wilkerson, 2016-06-14 18:59:41]

lgtm

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

https://codereview.chromium.org/2064203002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:2073: if (nextStatement is
BreakStatement ||
I could buy that; we're essentially leaving a single idiom for getting around a
warning in the spec.

How does this deal with something like

switch(intValue) {
  case 0:
    return false;
    break;
}

It would be nice if we could still report the break statement as being dead
code, but maybe that's too much for now.

[srawlins, 2016-06-14 21:09:55]

Thanks much for reviewing.

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

https://codereview.chromium.org/2064203002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:2073: if (nextStatement is
BreakStatement ||
On 2016/06/14 18:39:27, Paul Berry wrote:
> IMHO this is overly permissive.  The important case we want to allow is a
> mandated "break".  If someone puts some other statement at the end of a switch
> case (a return, continue, return, or throw), then I think we should go ahead
and
> report a hint, because in those other cases it's highly likely that the user
has
> made a mistake.

Ah, good point. I had not thought this through all the way.

https://codereview.chromium.org/2064203002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:2073: if (nextStatement is
BreakStatement ||
On 2016/06/14 18:59:41, Brian Wilkerson wrote:
> I could buy that; we're essentially leaving a single idiom for getting around
a
> warning in the spec.
> 
> How does this deal with something like
> 
> switch(intValue) {
>   case 0:
>     return false;
>     break;
> }
> 
> It would be nice if we could still report the break statement as being dead
> code, but maybe that's too much for now.

Yeah... I think at this point we just won't be smart enough to catch simple
baddies like this. But imho, this is very ok: the DEAD_CODE warnings are for
_non-obvious_ dead code paths, and our brains can be used for the obvious ones
:P.

It _will_ still catch:

return false;
int a = 1;
break;

https://codereview.chromium.org/2064203002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/non_hint_code_test.dart (right):

https://codereview.chromium.org/2064203002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/non_hint_code_test.dart:203: break;
On 2016/06/14 18:39:27, Paul Berry wrote:
> Please also add tests for the other kind of statements you're checking for
> (continue, return, throw).
> 
> (Assuming you take my previous suggestion and make those statements generate
> hints, then the additional tests should of course go in hint_code_test.dart).

Acknowledged.

[srawlins, 2016-06-15 14:44:37]

Added more tests.

[Brian Wilkerson, 2016-06-15 14:45:58]

lgtm

[Paul Berry, 2016-06-15 15:45:18]

lgtm, thanks!

[srawlins, 2016-06-15 16:24:36]

Description was changed from

==========
Do not error on dead, mandated statements at end of switch cases

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

to

==========
Do not error on dead, mandated statements at end of switch cases

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

Committed:
https://github.com/dart-lang/sdk/commit/6e58ff0575e57a68e73a2fc2438c9e713810cb25
==========

[srawlins, 2016-06-15 16:24:37]

Committed patchset #3 (id:40001) manually as
6e58ff0575e57a68e73a2fc2438c9e713810cb25 (presubmit successful).