Fix equality of implicit closures in the Dart VM.

Fix equality of implicit closures in the Dart VM.

This CL also lifts the restriction that classes could not
extends or implement 'Function'.

BUG=https://code.google.com/p/dart/issues/detail?id=10849
BUG=https://code.google.com/p/dart/issues/detail?id=12411

TEST=tests/language/bound_closure_equality_test.dart,
tests/language/black_listed_test.dart

R=iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26114

[Ivan Posva, 2013-08-09 21:24:30]

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

https://codereview.chromium.org/22425006/diff/1/runtime/vm/object.cc#newcode4751
runtime/vm/object.cc:4751: // Add == function to implicit instance closures.
This will not work because f, g and h in the example below do have the same
signature class.

class A {
  foo() => "A.foo";
}
bar() => "bar";
baz() => () => "I don't even…";

main() {
  var a = new A();
  var f = a.foo;
  var g = bar;
  var h = baz();
}

And thus you will be missing the injected functions depending on the order of
execution. Let's talk offline about the proper approach for this problem.

Thanks,
-Ivan

[Florian Schneider, 2013-08-12 09:32:12]

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

https://codereview.chromium.org/22425006/diff/1/runtime/vm/object.cc#newcode4751
runtime/vm/object.cc:4751: // Add == function to implicit instance closures.
Done. You're right. I'll make sure that == and hashcode is added to all
signature classes by moving the code that creates them to
Class::NewSignatureClass. 

On 2013/08/09 21:24:30, Ivan Posva wrote:
> This will not work because f, g and h in the example below do have the same
> signature class.
> 
> class A {
>   foo() => "A.foo";
> }
> bar() => "bar";
> baz() => () => "I don't even…";
> 
> main() {
>   var a = new A();
>   var f = a.foo;
>   var g = bar;
>   var h = baz();
> }
> 
> And thus you will be missing the injected functions depending on the order of
> execution. Let's talk offline about the proper approach for this problem.
> 
> Thanks,
> -Ivan

[Florian Schneider, 2013-08-12 18:03:05]

Uploaded a much simplified version as you suggested. Instead of having signature
classes implement "Function", they now extend "Function".

I had to remove an ASSERT to make it work, but it did not seem to be used
anyway.

https://codereview.chromium.org/22425006/diff/17001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (left):

https://codereview.chromium.org/22425006/diff/17001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1533: ASSERT(super_type.IsNull() ||
super_type.IsObjectType());
Do we rely on this assertion somewhere else?

[Ivan Posva, 2013-08-13 07:35:09]

https://codereview.chromium.org/22425006/diff/17001/runtime/lib/function_patc...
File runtime/lib/function_patch.dart (right):

https://codereview.chromium.org/22425006/diff/17001/runtime/lib/function_patc...
runtime/lib/function_patch.dart:31: bool operator ==(other) native
"Function_equals";
These should be on a separate class such as
class _FunctionImpl implements Function {
  bool operator ==(other) ...
  int get hashCode ...
}

so that other user-defined classes that extend Function do not inherit these
methods as this would break the assumption that the receiver is a Closure within
the VM.

https://codereview.chromium.org/22425006/diff/17001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/22425006/diff/17001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1884: // Signature classes, which are compiler
generated and represent a
I don't think this is a valid restriction.

https://codereview.chromium.org/22425006/diff/17001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1930: // any of bool, num, int, double, String,
Function, dynamic.
ditto for Function

[Florian Schneider, 2013-08-13 14:52:20]

Addressed comments. The CL got quite a bit bigger mostly because the additional
abstract _DartFunctionImpl class adds a lot of boilerplate code.

https://codereview.chromium.org/22425006/diff/17001/runtime/lib/function_patc...
File runtime/lib/function_patch.dart (right):

https://codereview.chromium.org/22425006/diff/17001/runtime/lib/function_patc...
runtime/lib/function_patch.dart:31: bool operator ==(other) native
"Function_equals";
On 2013/08/13 07:35:09, Ivan Posva wrote:
> These should be on a separate class such as
> class _FunctionImpl implements Function {
>   bool operator ==(other) ...
>   int get hashCode ...
> }
> 
> so that other user-defined classes that extend Function do not inherit these
> methods as this would break the assumption that the receiver is a Closure
within
> the VM.

Done.

https://codereview.chromium.org/22425006/diff/17001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/22425006/diff/17001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1884: // Signature classes, which are compiler
generated and represent a
On 2013/08/13 07:35:09, Ivan Posva wrote:
> I don't think this is a valid restriction.

Done.

https://codereview.chromium.org/22425006/diff/17001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1930: // any of bool, num, int, double, String,
Function, dynamic.
On 2013/08/13 07:35:09, Ivan Posva wrote:
> ditto for Function

Done.

[Florian Schneider, 2013-08-13 16:23:20]

Uploaded new patch.

[Florian Schneider, 2013-08-13 17:21:26]

Synced to bleeding_edge.

[Ivan Posva, 2013-08-13 17:58:48]

-Ivan

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:31: DEFINE_NATIVE_ENTRY(Function_equals, 2) {
FunctionImpl_equals

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:32: const Instance& receiver = Instance::CheckedHandle(
Instance::CheckedHandle should really be a Closure::CheckedHandle, which is
something we should add in object.h. Due to the fact of Closure not being a
subclass of Object, it needs to be added manually.

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:41: ASSERT(!func_a.IsImplicitStaticClosureFunction());
Please add a comment why this assertion is here.

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:55: DEFINE_NATIVE_ENTRY(Function_hashCode, 1) {
ditto

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:56: const Instance& receiver = Instance::CheckedHandle(
Same here about the Closure::CheckedHandle, which will remove the UNREACHABLE
below.

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:60: intptr_t result =
String::Handle(func.name()).Hash();
How about hashing both the function name, the class name and the signature
together?

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.dart
File runtime/lib/function.dart (right):

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.dart...
runtime/lib/function.dart:5: class _DartFunctionImpl implements Function {
_FunctionImpl

[Ivan Posva, 2013-08-14 05:27:57]

LGTM with comments addressed. The addition of Closure::CheckedHandle can happen
in a follow up CL.

-Ivan

https://codereview.chromium.org/22425006/diff/51001/runtime/vm/object_store.h
File runtime/vm/object_store.h (right):

https://codereview.chromium.org/22425006/diff/51001/runtime/vm/object_store.h...
runtime/vm/object_store.h:69: RawType* dart_function_impl_type() const { return
dart_function_impl_type_; }
function_impl_type(), set_function_impl_type and function_impl_type_.

https://codereview.chromium.org/22425006/diff/51001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/22425006/diff/51001/runtime/vm/symbols.h#newc...
runtime/vm/symbols.h:80: V(DartFunctionImpl, "_DartFunctionImpl")               
                     \
FunctionImpl

https://codereview.chromium.org/22425006/diff/51001/tests/language/black_list...
File tests/language/black_listed_test.dart (right):

https://codereview.chromium.org/22425006/diff/51001/tests/language/black_list...
tests/language/black_listed_test.dart:40: class MyFunction implements Function
{}
You should both allow implements Function and extends Function.

[Florian Schneider, 2013-08-14 09:07:48]

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc
File runtime/lib/function.cc (right):

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:31: DEFINE_NATIVE_ENTRY(Function_equals, 2) {
On 2013/08/13 17:58:49, Ivan Posva wrote:
> FunctionImpl_equals

Done.

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:32: const Instance& receiver = Instance::CheckedHandle(
On 2013/08/13 17:58:49, Ivan Posva wrote:
> Instance::CheckedHandle should really be a Closure::CheckedHandle, which is
> something we should add in object.h. Due to the fact of Closure not being a
> subclass of Object, it needs to be added manually.

Ok, I'll make a separate CL.

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:41: ASSERT(!func_a.IsImplicitStaticClosureFunction());
On 2013/08/13 17:58:49, Ivan Posva wrote:
> Please add a comment why this assertion is here.

Done.

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:55: DEFINE_NATIVE_ENTRY(Function_hashCode, 1) {
On 2013/08/13 17:58:49, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:56: const Instance& receiver = Instance::CheckedHandle(
On 2013/08/13 17:58:49, Ivan Posva wrote:
> Same here about the Closure::CheckedHandle, which will remove the UNREACHABLE
> below.

Will do in a separate CL.

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.cc#n...
runtime/lib/function.cc:60: intptr_t result =
String::Handle(func.name()).Hash();
On 2013/08/13 17:58:49, Ivan Posva wrote:
> How about hashing both the function name, the class name and the signature
> together?

Done.

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.dart
File runtime/lib/function.dart (right):

https://codereview.chromium.org/22425006/diff/51001/runtime/lib/function.dart...
runtime/lib/function.dart:5: class _DartFunctionImpl implements Function {
On 2013/08/13 17:58:49, Ivan Posva wrote:
> _FunctionImpl

Done.

https://codereview.chromium.org/22425006/diff/51001/runtime/vm/object_store.h
File runtime/vm/object_store.h (right):

https://codereview.chromium.org/22425006/diff/51001/runtime/vm/object_store.h...
runtime/vm/object_store.h:69: RawType* dart_function_impl_type() const { return
dart_function_impl_type_; }
On 2013/08/14 05:27:57, Ivan Posva wrote:
> function_impl_type(), set_function_impl_type and function_impl_type_.

Done.

https://codereview.chromium.org/22425006/diff/51001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/22425006/diff/51001/runtime/vm/symbols.h#newc...
runtime/vm/symbols.h:80: V(DartFunctionImpl, "_DartFunctionImpl")               
                     \
On 2013/08/14 05:27:57, Ivan Posva wrote:
> FunctionImpl

Done.

https://codereview.chromium.org/22425006/diff/51001/tests/language/black_list...
File tests/language/black_listed_test.dart (right):

https://codereview.chromium.org/22425006/diff/51001/tests/language/black_list...
tests/language/black_listed_test.dart:40: class MyFunction implements Function
{}
On 2013/08/14 05:27:57, Ivan Posva wrote:
> You should both allow implements Function and extends Function.

Done.

[Florian Schneider, 2013-08-14 11:44:03]

Committed patchset #9 manually as r26114 (presubmit successful).