[kernel] Fix bug with redirecting constructors in closure conversion.

[kernel] Fix bug with redirecting constructors in closure conversion.

Previously, the captured variable analysis did not provide sufficient
information for the conversion phase regarding variable uses in initalizers: in
particular, it did not differentiate the case when a variable is used in an
initializer and captured in the body vs. being captured in the body and not used
in an initializer. In addition, there were a few bugs stemming from the use
of lazy iterables and OR conjunctives with effectful operations.

Now, we separate the information about which variables are captured from flags
indicating whether variables are used in initializers. The other bugs are fixed
in obvious ways.

Finally, we reintroduce some code that ensures that redirecting factory
constructors listed in "_redirecting#" field (a hack used when writing DILL
files) remain with one-expression bodies after closure conversion.

Test Plan:

Added a test case for the initializers bug, ensured that the patched SDK builds
with closure conversion always-on.

Reviewers: dmitryas@google.com

BUG=
R=dmitryas@google.com

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

[sjindel, 2017-08-16 12:49:45]

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

[Dmitry Stefantsov, 2017-08-16 13:15:00]

I have one question about one of the .expect files.

https://codereview.chromium.org/2995083002/diff/1/pkg/kernel/testcases/closur...
File pkg/kernel/testcases/closures/contexts_in_field_initializers.dart.expect
(right):

https://codereview.chromium.org/2995083002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/contexts_in_field_initializers.dart.expect:15:
class B extends core::Object {
As I understand it, the redirecting technique is not required here.  I think,
the output should look somewhat like the following:


```
class B extends core::Object {
  field self::X x;
  constructor •(self::X x) → void
    : self::B::x = x, super core::Object::•() {
    final Vector #context = MakeVector(2);
    #context[1] = x;
    final () → dynamic fn = MakeClosure<() →
dynamic>(self::closure#B#function#fn, #context);
    fn.call();
  }
}
```

[sjindel, 2017-08-16 13:28:56]

https://codereview.chromium.org/2995083002/diff/1/pkg/kernel/testcases/closur...
File pkg/kernel/testcases/closures/contexts_in_field_initializers.dart.expect
(right):

https://codereview.chromium.org/2995083002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/contexts_in_field_initializers.dart.expect:15:
class B extends core::Object {
On 2017/08/16 13:15:00, Dmitry Stefantsov wrote:
> As I understand it, the redirecting technique is not required here.  I think,
> the output should look somewhat like the following:
> 
> 
> ```
> class B extends core::Object {
>   field self::X x;
>   constructor •(self::X x) → void
>     : self::B::x = x, super core::Object::•() {
>     final Vector #context = MakeVector(2);
>     #context[1] = x;
>     final () → dynamic fn = MakeClosure<() →
> dynamic>(self::closure#B#function#fn, #context);
>     fn.call();
>   }
> }
> ```

It's technically not needed, but the analysis is not yet sophisticated enough to
know that.
We could improve it, but I would rather keep that to a separate CL.

[Dmitry Stefantsov, 2017-08-17 12:59:09]

LGTM with a couple of comments.

One small suggestion for the future: it would be more obvious if you added the
test case in Patch Set 1 with the .expect file generated with "broken"
transformation, then fix the transformation in Patch Set 2 and update the
.expect file.  This way, the diff of the .expect file between Patch Set 1 and 2
will show the difference.

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

https://codereview.chromium.org/2995083002/diff/1/pkg/kernel/lib/transformati...
pkg/kernel/lib/transformations/closure/converter.dart:241: handleParam(decl) {
I think specifying type of the parameter may help avoid an implicit cast in the
body of the method.  I may be mistaken, though.

https://codereview.chromium.org/2995083002/diff/1/pkg/kernel/testcases/closur...
File pkg/kernel/testcases/closures/contexts_in_field_initializers.dart.expect
(right):

https://codereview.chromium.org/2995083002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/contexts_in_field_initializers.dart.expect:15:
class B extends core::Object {
On 2017/08/16 13:28:56, sjindel wrote:
> On 2017/08/16 13:15:00, Dmitry Stefantsov wrote:
> > As I understand it, the redirecting technique is not required here.  I
think,
> > the output should look somewhat like the following:
> > 
> > 
> > ```
> > class B extends core::Object {
> >   field self::X x;
> >   constructor •(self::X x) → void
> >     : self::B::x = x, super core::Object::•() {
> >     final Vector #context = MakeVector(2);
> >     #context[1] = x;
> >     final () → dynamic fn = MakeClosure<() →
> > dynamic>(self::closure#B#function#fn, #context);
> >     fn.call();
> >   }
> > }
> > ```
> 
> It's technically not needed, but the analysis is not yet sophisticated enough
to
> know that.
> We could improve it, but I would rather keep that to a separate CL.

Ok.  At the first glance, based on the CL description, it seemed to me that this
is exactly the kind of issues the CL is supposed to solve.

[sjindel, 2017-08-17 15:46:04]

https://codereview.chromium.org/2995083002/diff/1/pkg/kernel/testcases/closur...
File pkg/kernel/testcases/closures/contexts_in_field_initializers.dart.expect
(right):

https://codereview.chromium.org/2995083002/diff/1/pkg/kernel/testcases/closur...
pkg/kernel/testcases/closures/contexts_in_field_initializers.dart.expect:15:
class B extends core::Object {
On 2017/08/17 12:59:08, Dmitry Stefantsov wrote:
> On 2017/08/16 13:28:56, sjindel wrote:
> > On 2017/08/16 13:15:00, Dmitry Stefantsov wrote:
> > > As I understand it, the redirecting technique is not required here.  I
> think,
> > > the output should look somewhat like the following:
> > > 
> > > 
> > > ```
> > > class B extends core::Object {
> > >   field self::X x;
> > >   constructor •(self::X x) → void
> > >     : self::B::x = x, super core::Object::•() {
> > >     final Vector #context = MakeVector(2);
> > >     #context[1] = x;
> > >     final () → dynamic fn = MakeClosure<() →
> > > dynamic>(self::closure#B#function#fn, #context);
> > >     fn.call();
> > >   }
> > > }
> > > ```
> > 
> > It's technically not needed, but the analysis is not yet sophisticated
enough
> to
> > know that.
> > We could improve it, but I would rather keep that to a separate CL.
> 
> Ok.  At the first glance, based on the CL description, it seemed to me that
this
> is exactly the kind of issues the CL is supposed to solve.

This CL is address the correct issue, not the inefficiency of the extra function
call.

[sjindel, 2017-08-17 16:15:50]

Description was changed from

==========
[kernel] Fix bug with redirecting constructors in closure conversion.

Previously, the captured variable analysis did not provide sufficient
information for the conversion phase regarding variable uses in initalizers: in
particular, it did not differentiate the case when a variable is used in an
initializer and captured in the body vs. being captured in the body and not used
in an initializer. In addition, there were a few bugs stemming from the use
of lazy iterables and OR conjunctives with effectful operations.

Now, we separate the information about which variables are captured from flags
indicating whether variables are used in initializers. The other bugs are fixed
in obvious ways.

Finally, we reintroduce some code that ensures that redirecting factory
constructors listed in "_redirecting#" field (a hack used when writing DILL
files) remain with one-expression bodies after closure conversion.

Test Plan:

Added a test case for the initializers bug, ensured that the patched SDK builds
with closure conversion always-on.

Reviewers: dmitryas@google.com

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

to

==========
[kernel] Fix bug with redirecting constructors in closure conversion.

Previously, the captured variable analysis did not provide sufficient
information for the conversion phase regarding variable uses in initalizers: in
particular, it did not differentiate the case when a variable is used in an
initializer and captured in the body vs. being captured in the body and not used
in an initializer. In addition, there were a few bugs stemming from the use
of lazy iterables and OR conjunctives with effectful operations.

Now, we separate the information about which variables are captured from flags
indicating whether variables are used in initializers. The other bugs are fixed
in obvious ways.

Finally, we reintroduce some code that ensures that redirecting factory
constructors listed in "_redirecting#" field (a hack used when writing DILL
files) remain with one-expression bodies after closure conversion.

Test Plan:

Added a test case for the initializers bug, ensured that the patched SDK builds
with closure conversion always-on.

Reviewers: dmitryas@google.com

BUG=
R=dmitryas@google.com

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

[sjindel, 2017-08-17 16:15:51]

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