Implement tear-off closure operator #

Implement tear-off closure operator #


BUG=
R=fschneider@google.com

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

[hausner, 2015-07-16 23:24:20]

hausner@google.com changed reviewers:
+ iposva@google.com

[hausner, 2015-07-16 23:25:41]

Ivan, if you don't have time for the review please let me know.

Tear-off of constructors and super calls not yet supported.

[Ivan Posva, 2015-07-17 08:35:42]

First set of comments.

-Ivan

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7394: result = Object::Clone(result, Heap::kOld);
Why is this necessary?

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:11361: Instance& setter_closure =
Instance::ZoneHandle(field.SetterClosure());
These closures are not cached anywhere, which means we can create an "unbounded"
amount of these.

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/resolver.cc
File runtime/vm/resolver.cc (right):

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/resolver.cc#newc...
runtime/vm/resolver.cc:118: if (field_name.CharAt(0) == '#') {
Please add explanation here how we generate the closurization lookups. As far as
I understand it is done by creating an instance call "obj.get:#property()".
Additional explanation will help though.

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/resolver.cc#newc...
runtime/vm/resolver.cc:123: if (!Field::IsSetterName(property_name)) {
Please add comment why the property name needs to be adjusted for getters but
not for setters.

[Ivan Posva, 2015-07-17 08:37:02]

iposva@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2015-07-17 09:42:02]

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7390: Object::Handle(field_owner.Evaluate(expr_src,
With precompilation we can't invoke the compiler at runtime.

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7407: Object::Handle(field_owner.Evaluate(expr_src,
Same issue here with precompiled code.

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:11392: return new(Z) LiteralNode(property_pos,
setter_closure);
My suggestion would be to do sth. like

return CreateClosureNode(func, pos, NULL)

here, where func is a kImplicitStaticSetter function. This would mean
introducing a new Function kind (like kImplicitSetter).

For precompilation we could then simply collect all functions of this type
(kImplicitStaticGetter/kImplicitStaticSetter) and add them together with their
closurization to the list of functions to compile.

[hausner, 2015-07-17 17:53:51]

Thanks for taking a look. Here my answers, in case you are still online this
evening.

Short take: I hope to avoid additional function kinds.

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7390: Object::Handle(field_owner.Evaluate(expr_src,
On 2015/07/17 09:42:02, Florian Schneider wrote:
> With precompilation we can't invoke the compiler at runtime.

Yes. This evaluation happens at (pre-)compile time. I was hoping that there is a
way to also compile the resulting closure object at pre-compile time.

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7394: result = Object::Clone(result, Heap::kOld);
On 2015/07/17 08:35:42, Ivan Posva wrote:
> Why is this necessary?

Because the result will be stuck into a LiteralNode in the parser, and the
constructor for LiteralNode wants the literal to be allocated in old space.
Close is a private method of object, so I can't call it in the parser, which
would otherwise be my preference.

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:11361: Instance& setter_closure =
Instance::ZoneHandle(field.SetterClosure());
On 2015/07/17 08:35:42, Ivan Posva wrote:
> These closures are not cached anywhere, which means we can create an
"unbounded"
> amount of these.

They get collected if they are not stored anywhere, right? If someone closurizes
the same static field over and over again, should they not get what they
deserve? :)

Let's assume we cache the closures. Where should they be stored? Adding them to
the class where we store other anonymous closures is tricky because those
closures are identified with token positions. Which token position should we
then use for the getter and setter closures of a field? The field token
position? Then we'd have two closures at the same token position.

Adding the closures to the field object is not desirable because it makes Field
instances bigger.

Another option I was contemplating is to store the closures as static fields in
the class, kind of how we store metadata values for fields. That is the most
promising option, IMP, but I wanted to save that for a follow-on CL if it's
really needed.

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/parser.cc#newcod...
runtime/vm/parser.cc:11392: return new(Z) LiteralNode(property_pos,
setter_closure);
On 2015/07/17 09:42:02, Florian Schneider wrote:
> My suggestion would be to do sth. like
> 
> return CreateClosureNode(func, pos, NULL)
> 
> here, where func is a kImplicitStaticSetter function. This would mean
> introducing a new Function kind (like kImplicitSetter).
> 
> For precompilation we could then simply collect all functions of this type
> (kImplicitStaticGetter/kImplicitStaticSetter) and add them together with their
> closurization to the list of functions to compile.

Yes, I was considering this. I am hoping that we can avoid yet more function
kinds.

The new setter and getter kinds will not be enough, we'd probably also need a
new kind for the constructor closures (which I've not implemented yet.)

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/resolver.cc
File runtime/vm/resolver.cc (right):

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/resolver.cc#newc...
runtime/vm/resolver.cc:118: if (field_name.CharAt(0) == '#') {
On 2015/07/17 08:35:42, Ivan Posva wrote:
> Please add explanation here how we generate the closurization lookups. As far
as
> I understand it is done by creating an instance call "obj.get:#property()".
> Additional explanation will help though.

Correct. I will add a comment. The # in the getter name signals that we want to
closurize a getter rather than calling it (which is what we do in the old
closurization semantics.)

https://codereview.chromium.org/1234883005/diff/1/runtime/vm/resolver.cc#newc...
runtime/vm/resolver.cc:123: if (!Field::IsSetterName(property_name)) {
On 2015/07/17 08:35:42, Ivan Posva wrote:
> Please add comment why the property name needs to be adjusted for getters but
> not for setters.

Will do.

[hausner, 2015-07-17 23:13:07]

PTAL and let me know whether the pre-compilation works.

Thank you.

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11415: const Function& cf =
This is how precompilation can be done, unless I am missing something. The
setter closure is evaluated at (pre)compile time. We fish out the function and
compile it at the same time, so it's compiled when invoked at runtime.

Once we have a pre-compile flag, the invocation of the compiler will be guarded
by the flag.

[Florian Schneider, 2015-07-20 07:00:16]

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7412: String::NewFormatted("(xxx) { return %s = xxx; }",
field_name));
s/xxx/value/

to have a name that is already in our symbol table.

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11415: const Function& cf =
On 2015/07/17 23:13:07, hausner wrote:
> This is how precompilation can be done, unless I am missing something. The
> setter closure is evaluated at (pre)compile time. We fish out the function and
> compile it at the same time, so it's compiled when invoked at runtime.
> 
> Once we have a pre-compile flag, the invocation of the compiler will be
guarded
> by the flag.

Doesn't evaluating the function expression in SetterClosure not create a fresh
Function every time? Maybe it should be attached to the Field, so that you get
the same Function when it is already compiled.

[Florian Schneider, 2015-07-20 17:22:07]

https://codereview.chromium.org/1234883005/diff/20001/tests/language/tearoff_...
File tests/language/tearoff_basic_test.dart (right):

https://codereview.chromium.org/1234883005/diff/20001/tests/language/tearoff_...
tests/language/tearoff_basic_test.dart:140: testDeferred();
As Ryan mentioned, equality for closurized getters/setters should work like
closurized methods. e.g.

var a = C#st;
var b = C#st;
Expect.isTrue(a == b);
Expect.isTrue(identical(a, b));

Please add a test for equality for the different cases (static-, instance-
getters/setters/operators).

[hausner, 2015-07-20 17:47:27]

https://codereview.chromium.org/1234883005/diff/20001/tests/language/tearoff_...
File tests/language/tearoff_basic_test.dart (right):

https://codereview.chromium.org/1234883005/diff/20001/tests/language/tearoff_...
tests/language/tearoff_basic_test.dart:140: testDeferred();
On 2015/07/20 17:22:07, Florian Schneider wrote:
> As Ryan mentioned, equality for closurized getters/setters should work like
> closurized methods. e.g.
> 
> var a = C#st;
> var b = C#st;
> Expect.isTrue(a == b);
> Expect.isTrue(identical(a, b));
> 
> Please add a test for equality for the different cases (static-, instance-
> getters/setters/operators).

The spec says this: iff identical(o1,o2) then o1#m == o2#m. It does not mandate
identical(o1#m, o2#m).

To satisfy the equality requirement, I do need to save the static getter closure
function created by Evaluate() somewhere.

I'll add the test cases.

[hausner, 2015-07-21 17:18:36]

The generated getter and setter closures for static fields are now cached and
reused. Thus, the equality requirement is also fulfilled, i.e. C#m == C#m.

PTAL

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7412: String::NewFormatted("(xxx) { return %s = xxx; }",
field_name));
On 2015/07/20 07:00:16, Florian Schneider wrote:
> s/xxx/value/
> 
> to have a name that is already in our symbol table.

Done.

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1234883005/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11415: const Function& cf =
On 2015/07/20 07:00:16, Florian Schneider wrote:
> On 2015/07/17 23:13:07, hausner wrote:
> > This is how precompilation can be done, unless I am missing something. The
> > setter closure is evaluated at (pre)compile time. We fish out the function
and
> > compile it at the same time, so it's compiled when invoked at runtime.
> > 
> > Once we have a pre-compile flag, the invocation of the compiler will be
> guarded
> > by the flag.
> 
> Doesn't evaluating the function expression in SetterClosure not create a fresh
> Function every time? Maybe it should be attached to the Field, so that you get
> the same Function when it is already compiled.

The created closures are now cached for later reuse. This was also necessary to
guarantee equality of extracted closures.

[Florian Schneider, 2015-07-22 08:25:54]

Lgtm.

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7396: String& closure_name = String::Handle();
String& closure_name = String::Handle(this->name())

to save vertical space below.

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7446: String& closure_name = String::Handle();
String& closure_name = String::Handle(this->name())

to save vertical space below.

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7461: // This is the first time a getter closure for this
field is requested.
Update comment for setter closure.

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7466: if (field_owner.IsTopLevel()) {
Why is the top-level case needed here, and not for the getter case above?

Could most of the two methods GetterClosure/SetterClosure otherwise be unified
into one helper method? So that the only diff in input is the name, and the
evaluated function string?

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7493: field_owner.AddField(closure_field);
Maybe factor the creation of the field into a file-local helper to avoid
duplicating the code from above.

static RawField* CreateClosureField(...) {

}

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11425: if (!is_setter_name) {
if (is_setter_name) { }
if (!is_setter_name) { }

can be simplified to

if (is_setter_name) { } else { }

[hausner, 2015-07-22 18:41:10]

Thank you.

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7396: String& closure_name = String::Handle();
On 2015/07/22 08:25:53, Florian Schneider wrote:
> String& closure_name = String::Handle(this->name())
> 
> to save vertical space below.

vertigo prevented :)

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7446: String& closure_name = String::Handle();
On 2015/07/22 08:25:53, Florian Schneider wrote:
> String& closure_name = String::Handle(this->name())
> 
> to save vertical space below.

Done.

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7461: // This is the first time a getter closure for this
field is requested.
On 2015/07/22 08:25:53, Florian Schneider wrote:
> Update comment for setter closure.

Done.

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7466: if (field_owner.IsTopLevel()) {
On 2015/07/22 08:25:54, Florian Schneider wrote:
> Why is the top-level case needed here, and not for the getter case above?
> 
> Could most of the two methods GetterClosure/SetterClosure otherwise be unified
> into one helper method? So that the only diff in input is the name, and the
> evaluated function string?

The class name is added to avoid name conflicts when the field name and
parameter name are the same. I realize now that the problem still exists for
top-level variables, i.e. if the TL variable is named 'value'.

Solving this by mangling the parameter name if necessary and eliminating the
class name.

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7493: field_owner.AddField(closure_field);
On 2015/07/22 08:25:54, Florian Schneider wrote:
> Maybe factor the creation of the field into a file-local helper to avoid
> duplicating the code from above.
> 
> static RawField* CreateClosureField(...) {
> 
> }

Factoring out 4 lines of code that would require 3 parameters seems a bit
overkill.

I factored out the entire function into a separate method of class Field, rather
than a static method. The problem with the static method is that it could not
call Object::Clone().

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1234883005/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11425: if (!is_setter_name) {
On 2015/07/22 08:25:54, Florian Schneider wrote:
> if (is_setter_name) { }
> if (!is_setter_name) { }
> 
> can be simplified to
> 
> if (is_setter_name) { } else { }

Done.

I also removed the pre-compilation code, since we don't have a pre-compile flag
yet. Added a comment to remind ourselves that the closure can be eagerly
compiled.

[hausner, 2015-07-22 19:51:19]

Committed patchset #4 (id:60001) manually as
f1da09741dd69d48f10204cf5974e5ce93805330 (presubmit successful).