Allow `rethrow` to end a switch case. Allow braces around switch cases.

Allow `rethrow` to end a switch case. Allow braces around switch cases.

Add `rethrow` as one of the statements that are allowed to end a switch case
without a warning.
Change wording to not claim that `throw` is a statement.
Also allow wrapping case statements in a block statement without causing
more warnings. Currently, a `case 4: { break; }` is required to warn because
the last statement is a block statement, not a break statement.

Fixes issue #27650
BUG= http://dartbug.com/27650
R=eernst@google.com, floitsch@google.com, rnystrom@google.com

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

[Lasse Reichstein Nielsen, 2016-10-24 09:22:42]

lrn@google.com changed reviewers:
+ eernst@google.com, rnystrom@google.com

[floitsch, 2016-10-24 11:31:37]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2016-10-24 11:31:38]

LGTM.

[eernst, 2016-10-24 12:38:02]

I'm not quite sure how we specify the required control flow properties, but I
came up with one possible approach. We would need to take another iteration in
any case.

https://codereview.chromium.org/2447613003/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2447613003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:5976: It is a static warning $s$ is not a
\BREAK{}, \CONTINUE{}, \REHTROW{} or \RETURN{} statement or an expression
statment where the expression is a \THROW{} expression.
I'm not sure it's sufficiently helpful to mention the non-empty block statement,
that just makes all the other cases stick out even more.

What we want here is a flow graph; in that flow graph, we may have a node N that
represents the end of the `case`, and we would then focus on the immediate
predecessors N_1..N_m of N, looking recursively into composite statements. The
composite statement might be an `if`, it might be a nested `switch` statement,
etc., and we would then recursively consider the "then" and `else` branches of
that `if` respectively the cases of that `switch`. And then we need to take into
account the possibility that the nested statements may or may not leave the
right thing (with \BREAK{} and \CONTINUE{}, it might look safe unless they will
break/continue with one of those nested constructs).

I wonder if we should start introducing a notion of local flow analysis in order
to be able to say these things in a way that makes sense?

One approach that we could use would be to define for every statement whether it
is statically known to 'never complete normally'. \BREAK{}, \CONTINUE{},
\RETURN{}, \RETHROW{} have this property, and composite statements could
propagate it.

A similar concept would be needed for expressions which may be known to 'never
complete with a value', and \THROW{} expressions would have this property.

This would also allow us to handle things like
```
switch (e) {
case 1: // Not OK: `case 3` lets the nested `switch` complete normally.
  switch (e2) {
  case 2: // OK
    x = e3 ? throw 3 : throw 4;
  case 3: // OK: fall-through
    x = 4;
  }
case 5:
  x = 6;
}
```

So I guess the obvious choices are the following: do we want to enhance
completion to define 'will never complete normally/with-a-value', and then use
that concept here, or do we want to solve this problem later?

[Bob Nystrom, 2016-10-24 16:48:00]

https://codereview.chromium.org/2447613003/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2447613003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:5976: It is a static warning $s$ is not a
\BREAK{}, \CONTINUE{}, \REHTROW{} or \RETURN{} statement or an expression
statment where the expression is a \THROW{} expression.
On 2016/10/24 12:38:02, eernst wrote:
> I'm not sure it's sufficiently helpful to mention the non-empty block
statement,
> that just makes all the other cases stick out even more.
> 
> What we want here is a flow graph; in that flow graph, we may have a node N
that
> represents the end of the `case`, and we would then focus on the immediate
> predecessors N_1..N_m of N, looking recursively into composite statements. The
> composite statement might be an `if`, it might be a nested `switch` statement,
> etc., and we would then recursively consider the "then" and `else` branches of
> that `if` respectively the cases of that `switch`. And then we need to take
into
> account the possibility that the nested statements may or may not leave the
> right thing (with \BREAK{} and \CONTINUE{}, it might look safe unless they
will
> break/continue with one of those nested constructs).
> 
> I wonder if we should start introducing a notion of local flow analysis in
order
> to be able to say these things in a way that makes sense?
> 
> One approach that we could use would be to define for every statement whether
it
> is statically known to 'never complete normally'. \BREAK{}, \CONTINUE{},
> \RETURN{}, \RETHROW{} have this property, and composite statements could
> propagate it.
> 
> A similar concept would be needed for expressions which may be known to 'never
> complete with a value', and \THROW{} expressions would have this property.
> 
> This would also allow us to handle things like
> ```
> switch (e) {
> case 1: // Not OK: `case 3` lets the nested `switch` complete normally.
>   switch (e2) {
>   case 2: // OK
>     x = e3 ? throw 3 : throw 4;
>   case 3: // OK: fall-through
>     x = 4;
>   }
> case 5:
>   x = 6;
> }
> ```
> 
> So I guess the obvious choices are the following: do we want to enhance
> completion to define 'will never complete normally/with-a-value', and then use
> that concept here, or do we want to solve this problem later?

Yeah, I'm not crazy about how blocks are handled here either. It addresses this
case:

switch (e) {
  case 1: {
    return;
  }
}

But doesn't extend to even (admittedly contrived):

switch (e) {
  case 1: {
    {
      return;
    }
  }
}

I think we may want to consider having a concept of "definitely exits" and do
some flow analysis like Erik suggests to define that for each of the statement
and expression forms. I think this would dovetail nicely with a user-defined
bottom. It would be neat if you defined:

bottom exit() { /* abort process... */ }

Then:

switch (e) {
  case 1:
    exit();
}

And it would know this is OK.

[Lasse Reichstein Nielsen, 2016-10-24 18:38:25]

https://codereview.chromium.org/2447613003/diff/1/docs/language/dartLangSpec.tex
File docs/language/dartLangSpec.tex (right):

https://codereview.chromium.org/2447613003/diff/1/docs/language/dartLangSpec....
docs/language/dartLangSpec.tex:5976: It is a static warning $s$ is not a
\BREAK{}, \CONTINUE{}, \REHTROW{} or \RETURN{} statement or an expression
statment where the expression is a \THROW{} expression.
I don't want to solve the general problem right now.

That's a larger problem and needs to be addressed in more places in the
specification, and something we need to think about.

This CL addresses two shortcomings of the current spec - it missed the rethrow
statement for no good reason, and it doesn't handle the common case of wrapping
the statements of a case in a block.
It's not dealing with the recursive case of wrapping a block in a block because
that's just a baby step towards a real analysis, and we should either do it
right or not at all. 

Users coming from languages like C++ will wrap the statements in a block because
they think it's necessary if you make declarations inside the case. It's not
necessary in Dart, but I don't think it should be wrong either, and currently it
is (gives a warning).

We could argue the you shouldn't add a block, and then we shouldn't special case
it and instead just allow it to be a warning. That's a different option, I just
think this is better, yet still simple.


For bottom, it would be a Dart 2 feature - Dart 1 can't trust return types
anyway.

[Bob Nystrom, 2016-10-24 19:00:20]

SGTM. LGTM.

[eernst, 2016-10-25 08:09:04]

OK, LGTM.

[Lasse Reichstein Nielsen, 2016-10-25 11:05:13]

Description was changed from

==========
Allow `rethrow` to end a switch case. Allow braces around switch cases.

Add `rethrow` as one of the statements that are allowed to end a switch case
without a warning.
Change wording to not claim that `throw` is a statement.
Also allow wrapping case statements in a block statement without causing
more warnings. Currently, a `case 4: { break; }` is required to warn because
the last statement is a block statement, not a break statement.

Fixes issue #27650
BUG= http://dartbug.com/27650
==========

to

==========
Allow `rethrow` to end a switch case. Allow braces around switch cases.

Add `rethrow` as one of the statements that are allowed to end a switch case
without a warning.
Change wording to not claim that `throw` is a statement.
Also allow wrapping case statements in a block statement without causing
more warnings. Currently, a `case 4: { break; }` is required to warn because
the last statement is a block statement, not a break statement.

Fixes issue #27650
BUG= http://dartbug.com/27650
R=eernst@google.com, floitsch@google.com, rnystrom@google.com

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

[Lasse Reichstein Nielsen, 2016-10-25 11:05:14]

Committed patchset #1 (id:1) manually as
6e6ff496ed6bc0afd2af6c3d89590802cdecb7a9 (presubmit successful).