Fix several bugs in closure conversion.

Fix several bugs in closure conversion.

Summary:

1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
   correctly forward parameters to converted closure functions when
   they were captured in the converted function's body. This could happen when, for
   example, a closure was introduced into it by async conversion.

   Now, this is fixed by an approach that mirrors the technique in
   'BuildGraphOfFunction'.

2. Previously, local variables declared inside loop bodies were being saved in
   the loop's enclosing context, so closures within the loop would see new
   values initialized to the variable in subsequent iterations.

   Now, this is fixed by creating nested contexts for all loops, regardless of
   whether the loop variables are captured.

3. Previously, arity checks were not being performed on converted closures, so
   they could be called with too few or too many arguments. In the former case, the
   missing arguments would be filled in with garbage on the stack.

   Now, the assembly generation in 'CompileGraph' inserts argument count checks
   for converted closures as well as regular closures.

Test Plan:

Introduced new tests in the closure conversion suite to test each bug:

1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart

With these changes, closure conversion passes all co19 tests in non-checked mode, except those that are not passed without it:

python tools/test.py -m release -c dartk --vm-options "--reify --reify_generic_functions" co19

BUG=
R=dmitryas@google.com

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

[sjindel, 2017-08-22 18:47:52]

sjindel@google.com changed reviewers:
+ dmitryas@google.com

[Dmitry Stefantsov, 2017-08-24 09:41:13]

Nice to see closure conversion becoming more stable!  I have some questions and
comments.  Please, find them below.

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3181: bool get isLoop => false;
Personally, I don't like modifying AST definitions because of just one
transformation.  I believe, changes here should be justified universally.
 I would prefer defining a function `bool isLoop(Statement stmt)` in
`pkg/kernel/lib/transformations/closure/converter.dart` (currently, the only
place that needs to distinguish between loop statements and non-loop
statements).

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/transformati...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/closure/converter.dart:350: Expression
handleLocalFunction(FunctionNode function, [bool nope]) {
Is [nope] used anywhere?  If not, I suggest removing it.  If it is used, I
suggest considering renaming it into something more descriptive, turning it into
a named parameter (to me, it's hard to guess the meaning of boolean parameters
at call sights otherwise), and giving it the default value `false` (so that we
may remove the first line assigning `false` to [nope] in case it wasn't
specified).

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/arity....
File pkg/kernel/testcases/arity.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/arity....
pkg/kernel/testcases/arity.dart:1: /*
I think this file belongs to `pkg/kernel/testcases/closures`.  Maybe, because
it's in the wrong directory, the corresponding `.expect` file wasn't generated.

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/arity....
pkg/kernel/testcases/arity.dart:1: /*
Please, use line comments for the preamble here and in other files.  See
https://www.dartlang.org/guides/language/effective-dart/documentation#dont-us....

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/closur...
File pkg/kernel/testcases/closures/blocks.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/blocks.dart:5: */
Please, write a short description of the test here and at the other new test
cases.  It's hard to guess the intention when the test is more complex than a
few simple idiomatic lines.

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/blocks.dart:48: main() {}
Also, a comment describing why [main] is empty would be great.  Another solution
is to actually put something into [main] :)

[sjindel, 2017-08-24 11:11:43]

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3181: bool get isLoop => false;
On 2017/08/24 09:41:13, Dmitry Stefantsov wrote:
> Personally, I don't like modifying AST definitions because of just one
> transformation.  I believe, changes here should be justified universally.
>  I would prefer defining a function `bool isLoop(Statement stmt)` in
> `pkg/kernel/lib/transformations/closure/converter.dart` (currently, the only
> place that needs to distinguish between loop statements and non-loop
> statements).

I understand that, but I feel like determining whether a statement represents a
loop is a pretty general operation. It doesn't change the binary format anyway
-- it's just a helper function.

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/transformati...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/closure/converter.dart:350: Expression
handleLocalFunction(FunctionNode function, [bool nope]) {
On 2017/08/24 09:41:13, Dmitry Stefantsov wrote:
> Is [nope] used anywhere?  If not, I suggest removing it.  If it is used, I
> suggest considering renaming it into something more descriptive, turning it
into
> a named parameter (to me, it's hard to guess the meaning of boolean parameters
> at call sights otherwise), and giving it the default value `false` (so that we
> may remove the first line assigning `false` to [nope] in case it wasn't
> specified).

This was just some vestigial debugging code.

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/arity....
File pkg/kernel/testcases/arity.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/arity....
pkg/kernel/testcases/arity.dart:1: /*
On 2017/08/24 09:41:13, Dmitry Stefantsov wrote:
> Please, use line comments for the preamble here and in other files.  See
>
https://www.dartlang.org/guides/language/effective-dart/documentation#dont-us....

I think I just copied it from another file (maybe in co19?)

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/closur...
File pkg/kernel/testcases/closures/blocks.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/blocks.dart:5: */
On 2017/08/24 09:41:13, Dmitry Stefantsov wrote:
> Please, write a short description of the test here and at the other new test
> cases.  It's hard to guess the intention when the test is more complex than a
> few simple idiomatic lines.

Done.

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/blocks.dart:48: main() {}
On 2017/08/24 09:41:13, Dmitry Stefantsov wrote:
> Also, a comment describing why [main] is empty would be great.  Another
solution
> is to actually put something into [main] :)

Done.

[Dmitry Stefantsov, 2017-08-24 11:49:57]

Thanks for the update!  I have some more comments :)

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3181: bool get isLoop => false;
On 2017/08/24 11:11:43, sjindel wrote:
> On 2017/08/24 09:41:13, Dmitry Stefantsov wrote:
> > Personally, I don't like modifying AST definitions because of just one
> > transformation.  I believe, changes here should be justified universally.
> >  I would prefer defining a function `bool isLoop(Statement stmt)` in
> > `pkg/kernel/lib/transformations/closure/converter.dart` (currently, the only
> > place that needs to distinguish between loop statements and non-loop
> > statements).
> 
> I understand that, but I feel like determining whether a statement represents
a
> loop is a pretty general operation. It doesn't change the binary format anyway
> -- it's just a helper function.

I agree that it's a general operation and that it doesn't change the binary
representation, but I'd prefer to have it as a part of a bigger update that
brings some kind of systematic categorization to AST nodes.  This file serves as
a reference to Kernel syntax, and auxiliary methods may bring unnecessary
questioning about the design of the syntax.

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/closur...
File pkg/kernel/testcases/closures/blocks.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/blocks.dart:5: */
On 2017/08/24 11:11:43, sjindel wrote:
> On 2017/08/24 09:41:13, Dmitry Stefantsov wrote:
> > Please, write a short description of the test here and at the other new test
> > cases.  It's hard to guess the intention when the test is more complex than
a
> > few simple idiomatic lines.
> 
> Done.

Thanks!  It would be nice to boil down this test case to its essential part
though.  But feel free to live it as it is.

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/arity.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/arity.dart:3: // BSD-style license that can be
found in the LICENSE file.
Please, add a description to this test case.

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/loop2.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/loop2.dart:1: /*
Please, use one-line comments in the preamble and add a description.

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/syncstar.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/syncstar.dart:1: /*
Please, use one-line comments and add a description.

[sjindel, 2017-08-24 14:39:59]

Description was changed from

==========
Fix several bugs in closure conversion.

Summary:

1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
   correctly forward the context parameter to converted closure functions when
   it was captured in the converted function's body. This could happen when, for
   example, a closure was introduced into it by async conversion.

   Now, this is fixed by an approach that mirrors the technique in
   'BuildGraphOfFunction'.

2. Previously, local variables declared inside loop bodies were being saved in
   the loop's enclosing context, so closures within the loop would see new
   values initialized to the variable in subsequent iterations.

   Now, this is fixed by creating nested contexts for all loops, regardless of
   whether the loop variables are captured.

3. Previously, arity checks were not being performed on converted closures, so
   they could be called with too few or too many arguments. In the former case,
the
   missing arguments would be filled in with garbage on the stack.

   Now, the assembly generation in 'CompileGraph' inserts argument count checks
   for converted closures as well as regular closures.

Test Plan:

Introduced new tests in the closure conversion suite to test each bug:

1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart

BUG=
==========

to

==========
Fix several bugs in closure conversion.

Summary:

1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
   correctly forward parameters to converted closure functions when
   they were captured in the converted function's body. This could happen when,
for
   example, a closure was introduced into it by async conversion.

   Now, this is fixed by an approach that mirrors the technique in
   'BuildGraphOfFunction'.

2. Previously, local variables declared inside loop bodies were being saved in
   the loop's enclosing context, so closures within the loop would see new
   values initialized to the variable in subsequent iterations.

   Now, this is fixed by creating nested contexts for all loops, regardless of
   whether the loop variables are captured.

3. Previously, arity checks were not being performed on converted closures, so
   they could be called with too few or too many arguments. In the former case,
the
   missing arguments would be filled in with garbage on the stack.

   Now, the assembly generation in 'CompileGraph' inserts argument count checks
   for converted closures as well as regular closures.

Test Plan:

Introduced new tests in the closure conversion suite to test each bug:

1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart

BUG=
==========

[sjindel, 2017-08-24 14:50:14]

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/3000333002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3181: bool get isLoop => false;
On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> On 2017/08/24 11:11:43, sjindel wrote:
> > On 2017/08/24 09:41:13, Dmitry Stefantsov wrote:
> > > Personally, I don't like modifying AST definitions because of just one
> > > transformation.  I believe, changes here should be justified universally.
> > >  I would prefer defining a function `bool isLoop(Statement stmt)` in
> > > `pkg/kernel/lib/transformations/closure/converter.dart` (currently, the
only
> > > place that needs to distinguish between loop statements and non-loop
> > > statements).
> > 
> > I understand that, but I feel like determining whether a statement
represents
> a
> > loop is a pretty general operation. It doesn't change the binary format
anyway
> > -- it's just a helper function.
> 
> I agree that it's a general operation and that it doesn't change the binary
> representation, but I'd prefer to have it as a part of a bigger update that
> brings some kind of systematic categorization to AST nodes.  This file serves
as
> a reference to Kernel syntax, and auxiliary methods may bring unnecessary
> questioning about the design of the syntax.

Done.

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/arity.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/arity.dart:3: // BSD-style license that can be
found in the LICENSE file.
On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> Please, add a description to this test case.

Done.

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/loop2.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/loop2.dart:1: /*
On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> Please, use one-line comments in the preamble and add a description.

Done, but why does it matter?

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/syncstar.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/syncstar.dart:1: /*
On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> Please, use one-line comments and add a description.

Done.

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/syncstar.dart:1: /*
On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> Please, use one-line comments and add a description.

Done.

[sjindel, 2017-08-24 14:53:54]

sjindel@google.com changed reviewers:
+ karlklose@chromium.org

[Dmitry Stefantsov, 2017-08-24 15:07:29]

LGTM if you respond to the comments.

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/loop2.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/loop2.dart:1: /*
On 2017/08/24 14:50:13, sjindel wrote:
> On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> > Please, use one-line comments in the preamble and add a description.
> 
> Done, but why does it matter?

So that the next reader experiences less distraction by noticing that the same
thing is done differently, and concentrates on more important stuff.

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/syncstar.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/syncstar.dart:1: /*
On 2017/08/24 14:50:14, sjindel wrote:
> On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> > Please, use one-line comments and add a description.
> 
> Done.

Nope :)

https://codereview.chromium.org/3000333002/diff/60001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/3000333002/diff/60001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:3498: LocalVariable* parameter =
LookupParameterDirect(
Good catch!

https://codereview.chromium.org/3000333002/diff/80001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/syncstar.dart (right):

https://codereview.chromium.org/3000333002/diff/80001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/syncstar.dart:1: /*
I think the fix that changes /* */ to // and adds a description is lost
somewhere :)

[sjindel, 2017-08-24 15:13:42]

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/syncstar.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/syncstar.dart:1: /*
On 2017/08/24 15:07:29, Dmitry Stefantsov wrote:
> On 2017/08/24 14:50:14, sjindel wrote:
> > On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> > > Please, use one-line comments and add a description.
> > 
> > Done.
> 
> Nope :)

Oops!

[sjindel, 2017-08-24 15:14:11]

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/loop2.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/loop2.dart:1: /*
On 2017/08/24 15:07:28, Dmitry Stefantsov wrote:
> On 2017/08/24 14:50:13, sjindel wrote:
> > On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> > > Please, use one-line comments in the preamble and add a description.
> > 
> > Done, but why does it matter?
> 
> So that the next reader experiences less distraction by noticing that the same
> thing is done differently, and concentrates on more important stuff.

I think this is Green vs. Orange thinking :P

[Dmitry Stefantsov, 2017-08-24 15:17:29]

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures/loop2.dart (right):

https://codereview.chromium.org/3000333002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures/loop2.dart:1: /*
On 2017/08/24 15:14:11, sjindel wrote:
> On 2017/08/24 15:07:28, Dmitry Stefantsov wrote:
> > On 2017/08/24 14:50:13, sjindel wrote:
> > > On 2017/08/24 11:49:57, Dmitry Stefantsov wrote:
> > > > Please, use one-line comments in the preamble and add a description.
> > > 
> > > Done, but why does it matter?
> > 
> > So that the next reader experiences less distraction by noticing that the
same
> > thing is done differently, and concentrates on more important stuff.
> 
> I think this is Green vs. Orange thinking :P

Or Gold ;)

[sjindel, 2017-08-24 15:20:35]

Description was changed from

==========
Fix several bugs in closure conversion.

Summary:

1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
   correctly forward parameters to converted closure functions when
   they were captured in the converted function's body. This could happen when,
for
   example, a closure was introduced into it by async conversion.

   Now, this is fixed by an approach that mirrors the technique in
   'BuildGraphOfFunction'.

2. Previously, local variables declared inside loop bodies were being saved in
   the loop's enclosing context, so closures within the loop would see new
   values initialized to the variable in subsequent iterations.

   Now, this is fixed by creating nested contexts for all loops, regardless of
   whether the loop variables are captured.

3. Previously, arity checks were not being performed on converted closures, so
   they could be called with too few or too many arguments. In the former case,
the
   missing arguments would be filled in with garbage on the stack.

   Now, the assembly generation in 'CompileGraph' inserts argument count checks
   for converted closures as well as regular closures.

Test Plan:

Introduced new tests in the closure conversion suite to test each bug:

1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart

BUG=
==========

to

==========
Fix several bugs in closure conversion.

Summary:

1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
   correctly forward parameters to converted closure functions when
   they were captured in the converted function's body. This could happen when,
for
   example, a closure was introduced into it by async conversion.

   Now, this is fixed by an approach that mirrors the technique in
   'BuildGraphOfFunction'.

2. Previously, local variables declared inside loop bodies were being saved in
   the loop's enclosing context, so closures within the loop would see new
   values initialized to the variable in subsequent iterations.

   Now, this is fixed by creating nested contexts for all loops, regardless of
   whether the loop variables are captured.

3. Previously, arity checks were not being performed on converted closures, so
   they could be called with too few or too many arguments. In the former case,
the
   missing arguments would be filled in with garbage on the stack.

   Now, the assembly generation in 'CompileGraph' inserts argument count checks
   for converted closures as well as regular closures.

Test Plan:

Introduced new tests in the closure conversion suite to test each bug:

1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart

With these changes, closure conversion passes all co19 tests in non-checked
mode, except those that are not passed without it:

python tools/test.py -m release -c dartk --vm-options "--reify
--reify_generic_functions" co19

BUG=
==========

[sjindel, 2017-08-24 18:21:33]

Description was changed from

==========
Fix several bugs in closure conversion.

Summary:

1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
   correctly forward parameters to converted closure functions when
   they were captured in the converted function's body. This could happen when,
for
   example, a closure was introduced into it by async conversion.

   Now, this is fixed by an approach that mirrors the technique in
   'BuildGraphOfFunction'.

2. Previously, local variables declared inside loop bodies were being saved in
   the loop's enclosing context, so closures within the loop would see new
   values initialized to the variable in subsequent iterations.

   Now, this is fixed by creating nested contexts for all loops, regardless of
   whether the loop variables are captured.

3. Previously, arity checks were not being performed on converted closures, so
   they could be called with too few or too many arguments. In the former case,
the
   missing arguments would be filled in with garbage on the stack.

   Now, the assembly generation in 'CompileGraph' inserts argument count checks
   for converted closures as well as regular closures.

Test Plan:

Introduced new tests in the closure conversion suite to test each bug:

1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart

With these changes, closure conversion passes all co19 tests in non-checked
mode, except those that are not passed without it:

python tools/test.py -m release -c dartk --vm-options "--reify
--reify_generic_functions" co19

BUG=
==========

to

==========
Fix several bugs in closure conversion.

Summary:

1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
   correctly forward parameters to converted closure functions when
   they were captured in the converted function's body. This could happen when,
for
   example, a closure was introduced into it by async conversion.

   Now, this is fixed by an approach that mirrors the technique in
   'BuildGraphOfFunction'.

2. Previously, local variables declared inside loop bodies were being saved in
   the loop's enclosing context, so closures within the loop would see new
   values initialized to the variable in subsequent iterations.

   Now, this is fixed by creating nested contexts for all loops, regardless of
   whether the loop variables are captured.

3. Previously, arity checks were not being performed on converted closures, so
   they could be called with too few or too many arguments. In the former case,
the
   missing arguments would be filled in with garbage on the stack.

   Now, the assembly generation in 'CompileGraph' inserts argument count checks
   for converted closures as well as regular closures.

Test Plan:

Introduced new tests in the closure conversion suite to test each bug:

1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart

With these changes, closure conversion passes all co19 tests in non-checked
mode, except those that are not passed without it:

python tools/test.py -m release -c dartk --vm-options "--reify
--reify_generic_functions" co19

BUG=
R=dmitryas@google.com

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

[sjindel, 2017-08-24 18:21:35]

Committed patchset #6 (id:100001) manually as
7b53e209c584ee6ce7ff4c7bbc1afc4f013889b9 (presubmit successful).