During resolution, record the target of break/continue statements.

During resolution, record the target of break/continue statements.

This adds a new "target" field to BreakStatement and ContinueStatement
which points to the AST for the statement that the break/continue is
breaking from or continuing to.  For labeled break and continue
statements, this will always be the statement identified by the label
(not the LabeledStatement itself but its child).  For unlabeled break
and continue statements, this will be the innermost enclosing loop (or
switch statement, for break statements), provided that it is not in an
enclosing function.

R=brianwilkerson@google.com

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

[Paul Berry, 2014-11-24 22:30:46]

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

[Brian Wilkerson, 2014-11-24 23:20:53]

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

https://codereview.chromium.org/725473005/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/resolver.dart:11030: * target of an unlabeled
"break" statement, or null if there is no such
"target" --> "the target"
"null" --> "`null`"

(both here and below)

https://codereview.chromium.org/725473005/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/resolver.dart:11033: Statement
_unlabeledBreakTarget;
I'm not thrilled with this implementation for two reasons. (That isn't to say
that this is wrong, but I'd be interested to talk more about it.)

First, ScopedVisitor was designed to maintain the scope information so that (a)
all of that knowledge would be in one place and (b) it could be used by any
number of subclasses. While we don't currently have any subclasses that depend
on the label scope being constructed correctly, we can't add them in the future
without duplicating this implementation.

Second, it goes against the general design of the scope objects being a stack
that can be used to push and pop scope information by adding a second stack for
unlabeled statements.

https://codereview.chromium.org/725473005/diff/1/pkg/analyzer/lib/src/generat...
pkg/analyzer/lib/src/generated/resolver.dart:13193: Object
visitSwitchStatement(SwitchStatement node) {
This change looks wrong to me. The purpose of the label scope was to allow the
labels defined on the switch members to be in scope across the entire switch
statement and then to be easily removed when we exited the switch statement. How
do we remove those scopes?

[Paul Berry, 2014-11-25 00:29:11]

On 2014/11/24 23:20:53, Brian Wilkerson wrote:
>
https://codereview.chromium.org/725473005/diff/1/pkg/analyzer/lib/src/generat...
> File pkg/analyzer/lib/src/generated/resolver.dart (right):
> 
>
https://codereview.chromium.org/725473005/diff/1/pkg/analyzer/lib/src/generat...
> pkg/analyzer/lib/src/generated/resolver.dart:11030: * target of an unlabeled
> "break" statement, or null if there is no such
> "target" --> "the target"
> "null" --> "`null`"
> 
> (both here and below)
> 
>
https://codereview.chromium.org/725473005/diff/1/pkg/analyzer/lib/src/generat...
> pkg/analyzer/lib/src/generated/resolver.dart:11033: Statement
> _unlabeledBreakTarget;
> I'm not thrilled with this implementation for two reasons. (That isn't to say
> that this is wrong, but I'd be interested to talk more about it.)

Yes, we definitely should talk about it.  I think the implementation you'd
prefer looks a lot like the implementation I initially tried, but I kept running
into trouble and the CL you see now is the result of patching up that trouble. 
Now that I understand what's going on, I probably could re-do it with a cleaner
design, but I'm not relishing the idea of yet another attempt :)

I'll respond to your comments below outlining the trouble I ran into, and then
we can discuss it in person and decide whether it's worth giving it another try.

> 
> First, ScopedVisitor was designed to maintain the scope information so that
(a)
> all of that knowledge would be in one place and (b) it could be used by any
> number of subclasses. While we don't currently have any subclasses that depend
> on the label scope being constructed correctly, we can't add them in the
future
> without duplicating this implementation.

Actually that's not entirely true.  As the code currently exists in svn,
ResolverVisitor does depend on the label scope being constructed correctly, at
least for non-synthetic labels, because ElementResolver._lookupLabel gets its
starting point from _resolver.labelScope, and _resolver is the ResolverVisitor. 
However, ResolverVisitor doesn't construct correct scopes for synthetic labels,
because its implementation of visitWhileStatement doesn't call
ScopedVisitor.visitWhileStatement.  Worse yet,
ResolverVisitor.visitWhileStatement can't be trivially rewritten in terms of
ScopedVisitor.visitWhileStatement, because it needs to call
_overrideManager.enterScope() in between visiting the condition and the loop
body.  So to fix this, we would have to duplicate the code to maintain the label
scope stack in ResolverVisitor.visitWhileStatement.

The reason this isn't causing a user-visible bug right now is that
ElementResolver._lookupLabel returns null for non-labeled break and continue
statements, so it doesn't matter that it's not finding the correct synthetic
label.

When I wrote the CL, I tried to get while loops working before moving on to
other kinds of loops, so I didn't even notice that while loops are the only kind
of loop that has this problem.  Now that I realize that, it seems like
duplicating some code in ResolverVisitor.visitWhileStatement is probably not
such a big deal.

> 
> Second, it goes against the general design of the scope objects being a stack
> that can be used to push and pop scope information by adding a second stack
for
> unlabeled statements.

Unfortunately, there's a conflict of interest between how we want the scope of
labeled statements to behave compared to how we want the scope of unlabeled
statements to behave.  Consider this (incorrect) code:

  void f() {
    label: for (...) {
      void g() {
        break label;
      }
    }
  }

Currently, we nest the label scope for g() inside the label scope for f(), so
that when we lookup "label" we find the target element.  Only after we find it
do we check to see whether the target is in the same function, by comparing the
labelElement's ExecutableElement ancestor with _resolver.enclosingFunction.  I
think that's a good design, because it means that we can give the user a nice
informative error message ("Cannot reference label 'label' declared in an outer
method").  If instead we built a brand new LabelScope for the function g(), then
the error message would be "Cannot reference undefined label 'label'", which is
a lot more confusing.

However, things aren't so easy when we consider an unlabeled break:

  void f() {
    for (...) {
      void g() {
        break;
      }
    }
  }

In the current implementation, the "for" statement is associated with a
synthetic LabelElement, but that element is just a transient artifact created on
the fly by the ScopedVisitor--it doesn't exist in the element hierarchy.  Which
means there's no way to see what its enclosing function is because it doesn't
have a parent.

When I was writing the CL, the only way I could see to get the correct behavior
for the unlabeled example above was to use a fresh empty label scope for the
inner function.  But that would have meant losing the nice error message for the
labeled example.  However, in retrospect, it seems like there are three better
options:

(a) create the synthetic LabelElements with a parent pointer to the enclosing
function.  (This would work, but it seems a little sloppy to me because normally
an element contains pointers to all its children, but we don't want to do that
here because these LabelElements get created afresh each time a
ScopedVisitor-derived visitor runs).

(b) don't worry about whether the target of the break/continue statement is in
an outer function, because if it is, an error has already been reported by the
parser, so it doesn't matter if the resolver produces some slightly bogus
information.  (This would work, but I don't like it because the code in the
parser that catches this error condition doesn't work properly when doing
incremental parsing, and I was hoping to fix that bug by moving the error check
to the resolver.)

(c) store a pointer to the enclosing function element in the LabelScope, so that
we can use that to determine when a break/continue points to the outer function.

My preference is for option (c) for the reasons stated above :)

> 
>
https://codereview.chromium.org/725473005/diff/1/pkg/analyzer/lib/src/generat...
> pkg/analyzer/lib/src/generated/resolver.dart:13193: Object
> visitSwitchStatement(SwitchStatement node) {
> This change looks wrong to me. The purpose of the label scope was to allow the
> labels defined on the switch members to be in scope across the entire switch
> statement and then to be easily removed when we exited the switch statement.
How
> do we remove those scopes?

You're right.  I shouldn't have removed the try/finally block here.

[Brian Wilkerson, 2014-11-25 16:56:00]

After our discussion, LGTM

[Paul Berry, 2014-11-25 17:05:12]

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