fix #552, Object members on native types

fix #552, Object members on native types

We initially hard coded a few of these. That worked until native_typed_data and html were added. Those two libraries have a lot of native types.

This also fixes TypeRep to extend dart:core Object, and fixes the dart:_runtime so we can write Dart classes. Cleans up a few of the JS builtins as well.

R=vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/e5c92d956ad307226e96825d0b2a717ab8004241

[Jennifer Messerly, 2016-05-09 19:18:12]

Description was changed from

==========
fix #552, Object members on native types
We initially hard coded a few of these. That worked until native_typed_data and
html were added. Those two libraries have a lot of native types.
==========

to

==========
fix #552, Object members on native types

We initially hard coded a few of these. That worked until native_typed_data and
html were added. Those two libraries have a lot of native types.

This also fixes TypeRep to extend dart:core Object, and fixes the dart:_runtime
so we can write Dart classes. Cleans up a few of the JS builtins as well.
==========

[Jennifer Messerly, 2016-05-09 19:18:13]

jmesserly@google.com changed reviewers:
+ vsm@google.com

[vsm, 2016-05-09 20:55:56]

lgtm

https://codereview.chromium.org/1962823002/diff/1/lib/src/compiler/code_gener...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1962823002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:340: } else {
Is it worth skipping items already in runtimeHelpers?

https://codereview.chromium.org/1962823002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:1255: if (!m.isStatic && m.isPublic)
extensionMembers.add(m.name);
Do we still need Bob's fix here:

https://codereview.chromium.org/1944293003/diff/1/tool/input_sdk/private/ddc_...

https://codereview.chromium.org/1962823002/diff/1/tool/input_sdk/private/ddc_...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/1962823002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:461: // do we need to add a
cast here.
Can we sanity check?

var extension = getExtensionType(obj);
if (extension != null) {
  throwNoSuchMethod(...);
}

?

[Jennifer Messerly, 2016-05-10 00:33:57]

Thanks. Let me know what to do RE: noSuchMethod

https://codereview.chromium.org/1962823002/diff/1/lib/src/compiler/code_gener...
File lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/1962823002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:340: } else {
On 2016/05/09 20:55:56, vsm wrote:
> Is it worth skipping items already in runtimeHelpers?

Hmmm. We actually can't as we need the Element -> Node mapping to emit them. But
your observation is good, I don't think there's any reason for me to populate
runtimeHelpers in this method. Let me pull that code out of this function, I
think that may be what's confusing things.

https://codereview.chromium.org/1962823002/diff/1/lib/src/compiler/code_gener...
lib/src/compiler/code_generator.dart:1255: if (!m.isStatic && m.isPublic)
extensionMembers.add(m.name);
On 2016/05/09 20:55:56, vsm wrote:
> Do we still need Bob's fix here:
> 
>
https://codereview.chromium.org/1944293003/diff/1/tool/input_sdk/private/ddc_...

Well, Bob's test is still passing so that's a good sign :)

But I don't think they're related. Bob's fix was runtime helpers that need to
operate on private members like `dsend(this, _somePrivateField, ...)`.

This code is building up the list of members we want to implement:

class MyIter implements Iterable {
  get iterator => ...;
}
// DDC generates code here to also implement [dartx.iterator]


I think it follows from Dart's SDK library structure that it is impossible for a
user-defined type to implement a private member on an SDK interface (because
they are not part of that dart:* library)

(In general, private symbols on interfaces are a bit of an anti-pattern for that
very reason. No one can implement them outside the library.)

https://codereview.chromium.org/1962823002/diff/1/tool/input_sdk/private/ddc_...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/1962823002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:461: // do we need to add a
cast here.
On 2016/05/09 20:55:56, vsm wrote:
> Can we sanity check?
> 
> var extension = getExtensionType(obj);
> if (extension != null) {
>   throwNoSuchMethod(...);
> }
> 
> ?

Not sure I follow that code example, but yes I went ahead and implemented this
TODO.

Nothing actually overrides nSM in the SDK (can be verified by grepping), so
that's why I didn't. Also we don't actually implement nSM yet which makes it
hard to test :)

I tried to write a test, and I have it in another branch, but it's tied up in
implementing nSM so it has more impacts. If it's all right with you I'll leave
this path untested until I can get nSM going.

[vsm, 2016-05-10 00:36:36]

lgtm

[Leaf, 2016-05-10 02:16:45]

leafp@google.com changed reviewers:
+ leafp@google.com

[Leaf, 2016-05-10 02:16:46]

https://codereview.chromium.org/1962823002/diff/20001/lib/runtime/dart_sdk.js
File lib/runtime/dart_sdk.js (right):

https://codereview.chromium.org/1962823002/diff/20001/lib/runtime/dart_sdk.js...
lib/runtime/dart_sdk.js:733: +    if
(dart.notNull(dart.as(dart.isFunctionType(type), core.bool))) return type;
The boolean conversion here is a little unfortunate.  I wonder if we should keep
this as JS until we have special case code to make it faster?

[Jennifer Messerly, 2016-05-10 17:26:39]

https://codereview.chromium.org/1962823002/diff/20001/lib/runtime/dart_sdk.js
File lib/runtime/dart_sdk.js (right):

https://codereview.chromium.org/1962823002/diff/20001/lib/runtime/dart_sdk.js...
lib/runtime/dart_sdk.js:733: +    if
(dart.notNull(dart.as(dart.isFunctionType(type), core.bool))) return type;
On 2016/05/10 02:16:46, Leaf wrote:
> The boolean conversion here is a little unfortunate.  I wonder if we should
keep
> this as JS until we have special case code to make it faster?

Good catch! 

I can get rid of the `bool` cast easily by adding a return type to
`isFunctionType`. Have done that in a few places already.

Less sure about the notNull check. I think we may assume all inline-JS returns
non-null (whether that's a fair assumption, I'm not sure :) ). So I can call
isFunctionType from JS to get rid of the cast. Another idea is to assume we do
not need notNull checks inside the special dart:_runtime library. 

I also filed https://github.com/dart-lang/dev_compiler/issues/560

I'll land it with the bool-return-type fix, but advice appreciated on how to
best proceed with the null check.

[Jennifer Messerly, 2016-05-10 18:35:43]

Description was changed from

==========
fix #552, Object members on native types

We initially hard coded a few of these. That worked until native_typed_data and
html were added. Those two libraries have a lot of native types.

This also fixes TypeRep to extend dart:core Object, and fixes the dart:_runtime
so we can write Dart classes. Cleans up a few of the JS builtins as well.
==========

to

==========
fix #552, Object members on native types

We initially hard coded a few of these. That worked until native_typed_data and
html were added. Those two libraries have a lot of native types.

This also fixes TypeRep to extend dart:core Object, and fixes the dart:_runtime
so we can write Dart classes. Cleans up a few of the JS builtins as well.

R=vsm@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/e5c92d956ad307226e96825d0b2a...
==========

[Jennifer Messerly, 2016-05-10 18:35:45]

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