Semantic checks on switch cases and fall-through errors.

Semantic checks on switch cases and fall-through errors.

R=paulberry@google.com

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

[ahe, 2017-06-06 18:17:37]

ahe@google.com changed reviewers:
+ johnniwinther@google.com, paulberry@google.com

[Paul Berry, 2017-06-06 18:32:26]

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2558: while (lastNode is
Block || lastNode is LabeledStatement) {
This isn't consistent with the spec.  The spec says we only unwrap a block once.
 Here's the relevant spec text:

Let $s$ be the last statement of the statement sequence $s_k$.
If $s$ is a non-empty block statement, let $s$ instead be the last statement of
the block statement.
It is a static warning $s$ is not a \BREAK{}, \CONTINUE{}, \RETHROW{} or
\RETURN{} statement or an expression statement where the expression is a
\THROW{} expression.

[ahe, 2017-06-06 18:40:05]

Thank you, Paul. Good catch!

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2558: while (lastNode is
Block || lastNode is LabeledStatement) {
On 2017/06/06 18:32:26, Paul Berry wrote:
> This isn't consistent with the spec.  The spec says we only unwrap a block
once.
>  Here's the relevant spec text:
> 
> Let $s$ be the last statement of the statement sequence $s_k$.
> If $s$ is a non-empty block statement, let $s$ instead be the last statement
of
> the block statement.
> It is a static warning $s$ is not a \BREAK{}, \CONTINUE{}, \RETHROW{} or
> \RETURN{} statement or an expression statement where the expression is a
> \THROW{} expression.

Then I should replace lines 2558-2569 with lines 2559-2565, agreed?

[Paul Berry, 2017-06-06 18:55:37]

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2558: while (lastNode is
Block || lastNode is LabeledStatement) {
On 2017/06/06 18:40:05, ahe wrote:
> On 2017/06/06 18:32:26, Paul Berry wrote:
> > This isn't consistent with the spec.  The spec says we only unwrap a block
> once.
> >  Here's the relevant spec text:
> > 
> > Let $s$ be the last statement of the statement sequence $s_k$.
> > If $s$ is a non-empty block statement, let $s$ instead be the last statement
> of
> > the block statement.
> > It is a static warning $s$ is not a \BREAK{}, \CONTINUE{}, \RETHROW{} or
> > \RETURN{} statement or an expression statement where the expression is a
> > \THROW{} expression.
> 
> Then I should replace lines 2558-2569 with lines 2559-2565, agreed?

I think we still need the LabeledStatement logic.  Can labeled statements nest? 
If so, then I think we need something like:

while (lastNode is LabeledStatement) {
  LabeledStatement statement = lastNode;
  lastNode = statement.body;
}
if (lastNode is Block && lastNode.statements.isNotEmpty) {
  Block block = lastNode;
  lastNode = lastNode.statements.last;
}
while (lastNode is LabeledStatement) {
  LabeledStatement statement = lastNode;
  lastNode = statement.body;
}

(If labeled statements can't nest, then those "while"s above can change to
"if"s)

[ahe, 2017-06-06 19:02:16]

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2558: while (lastNode is
Block || lastNode is LabeledStatement) {
On 2017/06/06 18:55:37, Paul Berry wrote:
> On 2017/06/06 18:40:05, ahe wrote:
> > On 2017/06/06 18:32:26, Paul Berry wrote:
> > > This isn't consistent with the spec.  The spec says we only unwrap a block
> > once.
> > >  Here's the relevant spec text:
> > > 
> > > Let $s$ be the last statement of the statement sequence $s_k$.
> > > If $s$ is a non-empty block statement, let $s$ instead be the last
statement
> > of
> > > the block statement.
> > > It is a static warning $s$ is not a \BREAK{}, \CONTINUE{}, \RETHROW{} or
> > > \RETURN{} statement or an expression statement where the expression is a
> > > \THROW{} expression.
> > 
> > Then I should replace lines 2558-2569 with lines 2559-2565, agreed?
> 
> I think we still need the LabeledStatement logic.  Can labeled statements
nest? 
> If so, then I think we need something like:
> 
> while (lastNode is LabeledStatement) {
>   LabeledStatement statement = lastNode;
>   lastNode = statement.body;
> }
> if (lastNode is Block && lastNode.statements.isNotEmpty) {
>   Block block = lastNode;
>   lastNode = lastNode.statements.last;
> }
> while (lastNode is LabeledStatement) {
>   LabeledStatement statement = lastNode;
>   lastNode = statement.body;
> }
> 
> (If labeled statements can't nest, then those "while"s above can change to
> "if"s)

That's what I thought at first. However, that doesn't match the specification
you quoted.

[Paul Berry, 2017-06-06 19:18:09]

lgtm assuming you make the change you proposed in message 4

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2926533004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2558: while (lastNode is
Block || lastNode is LabeledStatement) {
On 2017/06/06 19:02:15, ahe wrote:
> On 2017/06/06 18:55:37, Paul Berry wrote:
> > On 2017/06/06 18:40:05, ahe wrote:
> > > On 2017/06/06 18:32:26, Paul Berry wrote:
> > > > This isn't consistent with the spec.  The spec says we only unwrap a
block
> > > once.
> > > >  Here's the relevant spec text:
> > > > 
> > > > Let $s$ be the last statement of the statement sequence $s_k$.
> > > > If $s$ is a non-empty block statement, let $s$ instead be the last
> statement
> > > of
> > > > the block statement.
> > > > It is a static warning $s$ is not a \BREAK{}, \CONTINUE{}, \RETHROW{} or
> > > > \RETURN{} statement or an expression statement where the expression is a
> > > > \THROW{} expression.
> > > 
> > > Then I should replace lines 2558-2569 with lines 2559-2565, agreed?
> > 
> > I think we still need the LabeledStatement logic.  Can labeled statements
> nest? 
> > If so, then I think we need something like:
> > 
> > while (lastNode is LabeledStatement) {
> >   LabeledStatement statement = lastNode;
> >   lastNode = statement.body;
> > }
> > if (lastNode is Block && lastNode.statements.isNotEmpty) {
> >   Block block = lastNode;
> >   lastNode = lastNode.statements.last;
> > }
> > while (lastNode is LabeledStatement) {
> >   LabeledStatement statement = lastNode;
> >   lastNode = statement.body;
> > }
> > 
> > (If labeled statements can't nest, then those "while"s above can change to
> > "if"s)
> 
> That's what I thought at first. However, that doesn't match the specification
> you quoted.

Fair enough.  I've checked and analyzer's interpretation matches yours.

[ahe, 2017-06-07 09:07:25]

The solution was a little bit different than we discussed, and I have added
comments that explain why. I'm going to submit this now if the tests pass, but
please do take another look.

[ahe, 2017-06-07 09:26:39]

Description was changed from

==========
Semantic checks on switch cases and fall-through errors.
==========

to

==========
Semantic checks on switch cases and fall-through errors.

R=paulberry@google.com

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

[ahe, 2017-06-07 09:26:40]

Committed patchset #2 (id:20001) manually as
c4a8b146e004e95311b20288b9bcf0a5e16b1d5f (presubmit successful).

[Paul Berry, 2017-06-07 14:14:24]

lgtm