Make the ClosedWorld build the closure class on the kernel side.

Make the ClosedWorld build the closure class on the kernel side.

This was discussed with Johnni to avoid having to add the class index after construction of the KernelClosureClass.

BUG=
R=johnniwinther@google.com, sra@google.com

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

[Emily Fortuna, 2017-07-27 00:32:18]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2017-07-27 16:38:25]

Patchset #1 (id:20001) has been deleted

[Emily Fortuna, 2017-07-27 16:38:30]

Patchset #1 (id:20002) has been deleted

[Emily Fortuna, 2017-07-27 16:40:01]

efortuna@google.com changed reviewers:
+ johnniwinther@google.com, sra@google.com

[Emily Fortuna, 2017-07-27 16:40:02]

https://codereview.chromium.org/2984263002/diff/50001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/2984263002/diff/50001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/element_map_impl.dart:2027: // PROBLEM: the type of
ir.Node that we want to pass in
Johnni, please take a look at this issue. What do you recommend?

[sra1, 2017-07-27 19:41:16]

https://codereview.chromium.org/2984263002/diff/50001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/2984263002/diff/50001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/element_map_impl.dart:2000: int i = 0;
should this be outside the loop?

[Johnni Winther, 2017-07-28 15:18:40]

lgtm

https://codereview.chromium.org/2984263002/diff/50001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/2984263002/diff/50001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/element_map_impl.dart:2027: // PROBLEM: the type of
ir.Node that we want to pass in
On 2017/07/27 16:40:02, Emily Fortuna wrote:
> Johnni, please take a look at this issue. What do you recommend?

You need to add the field to `_memberList` as well (otherwise it gets mixed with
a constructor body): 

    FieldEntity field = new ClosureField(...);
    cls.localToFieldMap[capturedLocal] = field;
    _memberList.add(field);
    _memberData.add(new MemberData(...))

Also, in builder_kernel (KernelSsaGraphBuilder.build), we shouldn't encounter
`MemberKind.closureField`, use this case handling:

    case MemberKind.closureField:
      failedAt(targetElement, "Unexpected closure field: $targetElement");
      break;

[Emily Fortuna, 2017-07-28 20:10:04]

ptal, sra@?

https://codereview.chromium.org/2984263002/diff/50001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/2984263002/diff/50001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/element_map_impl.dart:2000: int i = 0;
On 2017/07/27 19:41:16, sra1 wrote:
> should this be outside the loop?

doh. yes

https://codereview.chromium.org/2984263002/diff/50001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/element_map_impl.dart:2027: // PROBLEM: the type of
ir.Node that we want to pass in
On 2017/07/28 15:18:40, Johnni Winther wrote:
> On 2017/07/27 16:40:02, Emily Fortuna wrote:
> > Johnni, please take a look at this issue. What do you recommend?
> 
> You need to add the field to `_memberList` as well (otherwise it gets mixed
with
> a constructor body): 
> 
>     FieldEntity field = new ClosureField(...);
>     cls.localToFieldMap[capturedLocal] = field;
>     _memberList.add(field);
>     _memberData.add(new MemberData(...))
> 
> Also, in builder_kernel (KernelSsaGraphBuilder.build), we shouldn't encounter
> `MemberKind.closureField`, use this case handling:
> 
>     case MemberKind.closureField:
>       failedAt(targetElement, "Unexpected closure field: $targetElement");
>       break;
ahhh got it. Yeah, that branch shouldn't be hit now -- it was happening because
this wasn't getting added to memberList properly.

[sra1, 2017-07-28 20:38:51]

lgtm

https://codereview.chromium.org/2984263002/diff/70001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/common/codegen.dart (right):

https://codereview.chromium.org/2984263002/diff/70001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/codegen.dart:150: void
registerInstantiatedClosure(Local element) {
assert this is a function that can be used to find the synthetic closure class

[Emily Fortuna, 2017-07-28 21:37:06]

https://codereview.chromium.org/2984263002/diff/70001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/common/codegen.dart (right):

https://codereview.chromium.org/2984263002/diff/70001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/codegen.dart:150: void
registerInstantiatedClosure(Local element) {
On 2017/07/28 20:38:51, sra1 wrote:
> assert this is a function that can be used to find the synthetic closure class

You're right, this isn't an easy fix. I put it back to LocalFunctionElement and
updated the status file.

Johnni, can you please take a look at this? Basically we have a JLocal, but a
little further on we expect it to be a KLocalFunction (and it would be nice to
be sure that the thing passed in is a form of LocalFunction rather than just a
Local). I'm not sure if this is due to not having propagated K->J throughout the
backend or if I should be storing a different local type for the "original"
closure.

[Emily Fortuna, 2017-07-30 20:00:53]

Description was changed from

==========
Make the ClosedWorld build the closure class on the kernel side.

This was discussed with Johnni to avoid having to add the class index after
construction of the KernelClosureClass.

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

to

==========
Make the ClosedWorld build the closure class on the kernel side.

This was discussed with Johnni to avoid having to add the class index after
construction of the KernelClosureClass.

BUG=
R=johnniwinther@google.com, sra@google.com

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

[Emily Fortuna, 2017-07-30 20:00:56]

Committed patchset #3 (id:90001) manually as
a5ea160a44b70a4aefd52178ec4bba6977323460 (presubmit successful).

[Johnni Winther, 2017-07-31 10:54:29]

https://codereview.chromium.org/2984263002/diff/70001/pkg/compiler/lib/src/co...
File pkg/compiler/lib/src/common/codegen.dart (right):

https://codereview.chromium.org/2984263002/diff/70001/pkg/compiler/lib/src/co...
pkg/compiler/lib/src/common/codegen.dart:150: void
registerInstantiatedClosure(Local element) {
On 2017/07/28 21:37:06, Emily Fortuna wrote:
> On 2017/07/28 20:38:51, sra1 wrote:
> > assert this is a function that can be used to find the synthetic closure
class
> 
> You're right, this isn't an easy fix. I put it back to LocalFunctionElement
and
> updated the status file.
> 
> Johnni, can you please take a look at this? Basically we have a JLocal, but a
> little further on we expect it to be a KLocalFunction (and it would be nice to
> be sure that the thing passed in is a form of LocalFunction rather than just a
> Local). I'm not sure if this is due to not having propagated K->J throughout
the
> backend or if I should be storing a different local type for the "original"
> closure.

It is okay to use [Local] here. Long term we should just register the
call-method instead (we use it to compute if the signature is needed for RTI).