Convert closures in all initializers, and share the context between them.

Convert closures in all initializers, and share the context between them.

Summary:

Previously, we only handled `FieldInitializer` and `LocalInitializer`.

Now we handle all initializers.

Previously, we would create separate contexts for each initializers, which was
incorrect because it changes made to an argument from a closure within one
initializer would not be seen by a closure within another.

Now, we create the context in a `LocalInitializer` so all initializers will see
the same copy of the argument variables.

There is still an outstanding issue where variables introduced as local
initializers and later captured by closures in subsequent initializers are not
placed into the context. However, this will at least trigger an assert in the closure conversion pass.

Test Plan:

'closures_initializers/initializers.dart(.expect)' has been updated with very
simple test cases for super and redirecting initializers. The second bug
mentioned (captured local initializers) has not been reproduced yet.

BUG=
R=dmitryas@google.com

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

[sjindel, 2017-07-12 11:16:04]

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

[sjindel, 2017-07-12 15:51:01]

Description was changed from

==========
Convert closures in all initializers, and share the context between them.

Summary:

Previously, we only handled `FieldInitializer` and `LocalInitializer`.

Now we handle all initializers.

Previously, we would create separate contexts for each initializers, which was
incorrect because it changes made to an argument from a closure within one
initializer would not be seen by a closure within another.

Now, we create the context in a `LocalInitializer` so all initializers will see
the same copy of the argument variables.

This revision introduces an issue where variables that are considered captured
but not actually used in any closures in initializeres are still placed into a
context anyway, causing unnecessary context creation for most constructors.

There is still an outstanding issue where variables introduced as local
initializers and later captured by closures in subsequent initializers are not
placed into the context.

Test Plan:

'closures_initializers/initializers.dart(.expect)' has been updated with very
simple test cases for super and redirecting initializers. The second bug
mentioned (captured local initializers) has not been reproduced yet.

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

to

==========
Convert closures in all initializers, and share the context between them.

Summary:

Previously, we only handled `FieldInitializer` and `LocalInitializer`.

Now we handle all initializers.

Previously, we would create separate contexts for each initializers, which was
incorrect because it changes made to an argument from a closure within one
initializer would not be seen by a closure within another.

Now, we create the context in a `LocalInitializer` so all initializers will see
the same copy of the argument variables.

There is still an outstanding issue where variables introduced as local
initializers and later captured by closures in subsequent initializers are not
placed into the context.

Test Plan:

'closures_initializers/initializers.dart(.expect)' has been updated with very
simple test cases for super and redirecting initializers. The second bug
mentioned (captured local initializers) has not been reproduced yet.

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

[Dmitry Stefantsov, 2017-07-13 07:19:54]

Great update! I have some small comments that I think should be resolved before
landing. Please, find them below.

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:30: // Let,
If this import is not longer needed, it should be deleted.

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:125: /// initializer list.
Since the default "visitChildren" method of
I suppose you can also use square-bracket notation here: [visitChildren],
because you refer to an identifier in the code.

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/rewriter.dart (right):

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/rewriter.dart:89: final Constructor ctor;
Please, use a more descriptive name than [ctor].

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures_initializers/initializers.dart (right):

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures_initializers/initializers.dart:34: S2.wat(X i) :
this((() => i)());
How this test case is different from the one for the constructor of [B] below?

[sjindel, 2017-07-13 11:34:05]

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/converter.dart (right):

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/converter.dart:30: // Let,
On 2017/07/13 07:19:53, Dmitry Stefantsov wrote:
> If this import is not longer needed, it should be deleted.

Done.

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:125: /// initializer list.
Since the default "visitChildren" method of
On 2017/07/13 07:19:53, Dmitry Stefantsov wrote:
> I suppose you can also use square-bracket notation here: [visitChildren],
> because you refer to an identifier in the code.

Done.

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/rewriter.dart (right):

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/rewriter.dart:89: final Constructor ctor;
On 2017/07/13 07:19:53, Dmitry Stefantsov wrote:
> Please, use a more descriptive name than [ctor].

Done.

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/testcases/cl...
File pkg/kernel/testcases/closures_initializers/initializers.dart (right):

https://codereview.chromium.org/2981603002/diff/20001/pkg/kernel/testcases/cl...
pkg/kernel/testcases/closures_initializers/initializers.dart:34: S2.wat(X i) :
this((() => i)());
On 2017/07/13 07:19:53, Dmitry Stefantsov wrote:
> How this test case is different from the one for the constructor of [B] below?

It wasn't -- I fixed this in patch set 2 by changing the argument extraction
method to extract arguments from field initializers instead, based on whether
the field name ended in "_li".

[sjindel, 2017-07-13 11:37:38]

Description was changed from

==========
Convert closures in all initializers, and share the context between them.

Summary:

Previously, we only handled `FieldInitializer` and `LocalInitializer`.

Now we handle all initializers.

Previously, we would create separate contexts for each initializers, which was
incorrect because it changes made to an argument from a closure within one
initializer would not be seen by a closure within another.

Now, we create the context in a `LocalInitializer` so all initializers will see
the same copy of the argument variables.

There is still an outstanding issue where variables introduced as local
initializers and later captured by closures in subsequent initializers are not
placed into the context.

Test Plan:

'closures_initializers/initializers.dart(.expect)' has been updated with very
simple test cases for super and redirecting initializers. The second bug
mentioned (captured local initializers) has not been reproduced yet.

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

to

==========
Convert closures in all initializers, and share the context between them.

Summary:

Previously, we only handled `FieldInitializer` and `LocalInitializer`.

Now we handle all initializers.

Previously, we would create separate contexts for each initializers, which was
incorrect because it changes made to an argument from a closure within one
initializer would not be seen by a closure within another.

Now, we create the context in a `LocalInitializer` so all initializers will see
the same copy of the argument variables.

There is still an outstanding issue where variables introduced as local
initializers and later captured by closures in subsequent initializers are not
placed into the context. However, this will at least trigger an assert in the
closure conversion pass.

Test Plan:

'closures_initializers/initializers.dart(.expect)' has been updated with very
simple test cases for super and redirecting initializers. The second bug
mentioned (captured local initializers) has not been reproduced yet.

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

[Dmitry Stefantsov, 2017-07-14 00:34:18]

Nice update! LGTM if you respond to some minor comments about naming and code
structuring.

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/argument_extraction.dart (right):

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction.dart:32: class
ArgumentExtractionForRedirecting extends Transformer {
Please, see the comments for argument_extraction_for_redirecting.dart.  They
apply to this class as well.

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart
(right):

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart:36: int
i = 0;
[i] is not a counter of a loop, so I guess it should have a more descriptive
name.

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart:37: for
(var fi in node.initializers) {
Please, use a more descriptive name in place of [fi] as well.

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart:41:
continue;
I think `continue` here doesn't help to eliminate all non-special cases; we do
have an `else` below anyway.  How about structuring the code like this:

```
int variableCounter = 0; // Previously known as [i].
for (var initializer in node.initializers) {
  if (initializer is FieldInitializer
      && initializer.field.name.name.endsWith("_li")) {
    // ... dealing with LocalInitializer here ...
    ++variableCounter;
  }
  newInits.add(initializer);
}
```

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart:52: var
li = new LocalInitializer(decl);
Please, use a more descriptive name in place of [li].

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:129: /// TODO(sjindel): Don't
visit the parameters twice.
DBC:  In my still-in-review CL I have a fix for that:
https://codereview.chromium.org/2891053003/

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/rewriter.dart (right):

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/rewriter.dart:89: final Constructor
parentCtor;
How about "parentConstructor"?

[sjindel, 2017-07-14 08:49:45]

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/argument_extraction.dart (right):

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction.dart:32: class
ArgumentExtractionForRedirecting extends Transformer {
On 2017/07/14 00:34:17, Dmitry Stefantsov wrote:
> Please, see the comments for argument_extraction_for_redirecting.dart.  They
> apply to this class as well.

I meant to delete this file.

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart
(right):

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart:36: int
i = 0;
On 2017/07/14 00:34:17, Dmitry Stefantsov wrote:
> [i] is not a counter of a loop, so I guess it should have a more descriptive
> name.

Done.

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart:37: for
(var fi in node.initializers) {
On 2017/07/14 00:34:17, Dmitry Stefantsov wrote:
> Please, use a more descriptive name in place of [fi] as well.

Done.

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart:41:
continue;
On 2017/07/14 00:34:17, Dmitry Stefantsov wrote:
> I think `continue` here doesn't help to eliminate all non-special cases; we do
> have an `else` below anyway.  How about structuring the code like this:
> 
> ```
> int variableCounter = 0; // Previously known as [i].
> for (var initializer in node.initializers) {
>   if (initializer is FieldInitializer
>       && initializer.field.name.name.endsWith("_li")) {
>     // ... dealing with LocalInitializer here ...
>     ++variableCounter;
>   }
>   newInits.add(initializer);
> }
> ```

Done.

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/argument_extraction_for_redirecting.dart:52: var
li = new LocalInitializer(decl);
On 2017/07/14 00:34:17, Dmitry Stefantsov wrote:
> Please, use a more descriptive name in place of [li].

Done.

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/rewriter.dart (right):

https://codereview.chromium.org/2981603002/diff/60001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/rewriter.dart:89: final Constructor
parentCtor;
On 2017/07/14 00:34:17, Dmitry Stefantsov wrote:
> How about "parentConstructor"?

Done.

[sjindel, 2017-07-14 08:59:23]

Description was changed from

==========
Convert closures in all initializers, and share the context between them.

Summary:

Previously, we only handled `FieldInitializer` and `LocalInitializer`.

Now we handle all initializers.

Previously, we would create separate contexts for each initializers, which was
incorrect because it changes made to an argument from a closure within one
initializer would not be seen by a closure within another.

Now, we create the context in a `LocalInitializer` so all initializers will see
the same copy of the argument variables.

There is still an outstanding issue where variables introduced as local
initializers and later captured by closures in subsequent initializers are not
placed into the context. However, this will at least trigger an assert in the
closure conversion pass.

Test Plan:

'closures_initializers/initializers.dart(.expect)' has been updated with very
simple test cases for super and redirecting initializers. The second bug
mentioned (captured local initializers) has not been reproduced yet.

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

to

==========
Convert closures in all initializers, and share the context between them.

Summary:

Previously, we only handled `FieldInitializer` and `LocalInitializer`.

Now we handle all initializers.

Previously, we would create separate contexts for each initializers, which was
incorrect because it changes made to an argument from a closure within one
initializer would not be seen by a closure within another.

Now, we create the context in a `LocalInitializer` so all initializers will see
the same copy of the argument variables.

There is still an outstanding issue where variables introduced as local
initializers and later captured by closures in subsequent initializers are not
placed into the context. However, this will at least trigger an assert in the
closure conversion pass.

Test Plan:

'closures_initializers/initializers.dart(.expect)' has been updated with very
simple test cases for super and redirecting initializers. The second bug
mentioned (captured local initializers) has not been reproduced yet.

BUG=
R=dmitryas@google.com

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

[sjindel, 2017-07-14 08:59:25]

Committed patchset #5 (id:80001) manually as
a459f73facd32a26514e92e691eb8ab8776e3e75 (presubmit successful).