fixes #40, extension methods for primitive types

fixes #40, extension methods for primitive types

fixes #138, extension methods for Iterable/List injected as needed, regardless of how the methods started

fixes #142, extension methods and dynamic dispatch

small change that fixes #139, recognize that '.length' doesn't need an extension method

Also restores JSArray, moved the methods out of List, which is back to just being an interface.

NOTE: this removes the capability of types that mix native JS members and some Dart extension members, instead it now *hides* all JS members for these native types. We'll want to bring that feature back to support dart:html, so it's just a simplification for now. Should be easy to add that capability when we pull in dart:html for reals.

R=leafp@google.com, vsm@google.com

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

[Jennifer Messerly, 2015-05-30 00:00:51]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2015-05-30 00:04:04]

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

[Jennifer Messerly, 2015-05-30 00:04:04]

Not 100% sure this is ready, but I wanted to send it out. I'm going to continue
some manual testing. Fixes a bunch of issues in how we dispatch calls on JS
primitive types.

For Angular, it gets as far as master, but Angular isn't working currently on
master, so not sure if it breaks something later on.

I did some manual console testing (via sunflower) of some of the dynamic ops,
and Dart extensions, at least some of them seem to be working as intended.

[vsm, 2015-06-02 17:59:10]

This looks nice!

A couple quick comments - still working through it.

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:109: throwNoSuchMethod(obj, displayName, args);
displayName isn't passed in by dload / dput above.  Use name if that's
undefined?

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:792: jsProto[_extensionType] = extProto;
assert that this isn't already defined?

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:812: function implementExtension(type) {
Hmm, you're calling this everywhere with a second arg, but ignoring it here. 
Could you use the second arg (the implemented type) to filter what to copy
instead of moving everything?

[Jennifer Messerly, 2015-06-02 18:16:04]

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:812: function implementExtension(type) {
On 2015/06/02 17:59:10, vsm wrote:
> Hmm, you're calling this everywhere with a second arg, but ignoring it here. 
> Could you use the second arg (the implemented type) to filter what to copy
> instead of moving everything?

Sigh, I wish we could. The doc comment above explains the unfortunate situation.

I actually had implementExtension take just one arg. It seemed hard to
understand, because you can't easily read what is being implemented in the code.
But we don't need it here.

I wanted to emit a comment, but there isn't a good way to emit trailing
comments, e.g.

    dart.implementExtension(ListBase); // core.Iterable, core.List

Also harder to goto def, etc.

Eventually if we preserve interface members, maybe for dart:mirrors, my plan was
to update this code to copy only the methods.

But I'm not sure we should assume mirrors are always compiled in.

So it's ... tricky. Appreciate thoughts. Maybe there's a better local minima I'm
not seeing.

[Leaf, 2015-06-02 21:35:04]

Nice!  LGTM.

-leaf

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:115: function callMethod(obj, name, displayName,
args) {
Slightly confusing that callMethod and _canonicalFieldName take identical
argument lists but with last two args swapped.  If it's all the same, might be
worth swapping one or the other.

https://codereview.chromium.org/1153003003/diff/20001/tool/sdk_expected_error...
File tool/sdk_expected_errors.txt (right):

https://codereview.chromium.org/1153003003/diff/20001/tool/sdk_expected_error...
tool/sdk_expected_errors.txt:1085: @SupportJsExtensionMethods()
Typo - the definition uses ExtensionMethod vs ExtensionMethods

[Jennifer Messerly, 2015-06-02 23:43:42]

FYI, found a bug ...

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:818: defineProperty(proto, symbol,
getOwnPropertyDescriptor(proto, name));
Oops. Just noticed, this needs to generate a stub that virtually dispatches. Or,
we need to generate it whenever a concrete method is implemented.

[vsm, 2015-06-03 15:48:40]

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:812: function implementExtension(type) {
On 2015/06/02 18:16:04, John Messerly wrote:
> On 2015/06/02 17:59:10, vsm wrote:
> > Hmm, you're calling this everywhere with a second arg, but ignoring it here.

> > Could you use the second arg (the implemented type) to filter what to copy
> > instead of moving everything?
> 
> Sigh, I wish we could. The doc comment above explains the unfortunate
situation.
> 
> I actually had implementExtension take just one arg. It seemed hard to
> understand, because you can't easily read what is being implemented in the
code.
> But we don't need it here.
> 
> I wanted to emit a comment, but there isn't a good way to emit trailing
> comments, e.g.
> 
>     dart.implementExtension(ListBase); // core.Iterable, core.List
> 
> Also harder to goto def, etc.
> 
> Eventually if we preserve interface members, maybe for dart:mirrors, my plan
was
> to update this code to copy only the methods.
> 
> But I'm not sure we should assume mirrors are always compiled in.
> 
> So it's ... tricky. Appreciate thoughts. Maybe there's a better local minima
I'm
> not seeing.

General thoughts: 
(1) it's worth declaring the second arg here since you're passing it in from
generated code even if you're currently ignoring.
(2) beyond this CL - it'd be nice to use the metadata if present to copy only
what's needed
  - we'd need to extend the metadata to record abstract methods (needed for
mirrors)
  - your approach might still make sense as a fallback if no / insufficient
metadata
(3) also beyond this CL - We'd also need a way to deal with laziness / eagerness
(of the second arg - currently a closure) if we want to use the metadata.

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:818: defineProperty(proto, symbol,
getOwnPropertyDescriptor(proto, name));
On 2015/06/02 23:43:42, John Messerly wrote:
> Oops. Just noticed, this needs to generate a stub that virtually dispatches.
Or,
> we need to generate it whenever a concrete method is implemented.

Good catch.  :-)

[Jennifer Messerly, 2015-06-04 21:31:19]

PTAL

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:109: throwNoSuchMethod(obj, displayName, args);
On 2015/06/02 17:59:10, vsm wrote:
> displayName isn't passed in by dload / dput above.  Use name if that's
> undefined?

Done.

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:115: function callMethod(obj, name, displayName,
args) {
On 2015/06/02 21:35:04, Leaf wrote:
> Slightly confusing that callMethod and _canonicalFieldName take identical
> argument lists but with last two args swapped.  If it's all the same, might be
> worth swapping one or the other.

Done.

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:792: jsProto[_extensionType] = extProto;
On 2015/06/02 17:59:10, vsm wrote:
> assert that this isn't already defined?

Done.

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:812: function implementExtension(type) {
On 2015/06/03 15:48:39, vsm wrote:
> On 2015/06/02 18:16:04, John Messerly wrote:
> > On 2015/06/02 17:59:10, vsm wrote:
> > > Hmm, you're calling this everywhere with a second arg, but ignoring it
here.
> 
> > > Could you use the second arg (the implemented type) to filter what to copy
> > > instead of moving everything?
> > 
> > Sigh, I wish we could. The doc comment above explains the unfortunate
> situation.
> > 
> > I actually had implementExtension take just one arg. It seemed hard to
> > understand, because you can't easily read what is being implemented in the
> code.
> > But we don't need it here.
> > 
> > I wanted to emit a comment, but there isn't a good way to emit trailing
> > comments, e.g.
> > 
> >     dart.implementExtension(ListBase); // core.Iterable, core.List
> > 
> > Also harder to goto def, etc.
> > 
> > Eventually if we preserve interface members, maybe for dart:mirrors, my plan
> was
> > to update this code to copy only the methods.
> > 
> > But I'm not sure we should assume mirrors are always compiled in.
> > 
> > So it's ... tricky. Appreciate thoughts. Maybe there's a better local minima
> I'm
> > not seeing.
> 
> General thoughts: 
> (1) it's worth declaring the second arg here since you're passing it in from
> generated code even if you're currently ignoring.
> (2) beyond this CL - it'd be nice to use the metadata if present to copy only
> what's needed
>   - we'd need to extend the metadata to record abstract methods (needed for
> mirrors)
>   - your approach might still make sense as a fallback if no / insufficient
> metadata
> (3) also beyond this CL - We'd also need a way to deal with laziness /
eagerness
> (of the second arg - currently a closure) if we want to use the metadata.

Update: redesigned this. We now emit extension method names explicitly

https://codereview.chromium.org/1153003003/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:818: defineProperty(proto, symbol,
getOwnPropertyDescriptor(proto, name));
On 2015/06/03 15:48:40, vsm wrote:
> On 2015/06/02 23:43:42, John Messerly wrote:
> > Oops. Just noticed, this needs to generate a stub that virtually dispatches.
> Or,
> > we need to generate it whenever a concrete method is implemented.
> 
> Good catch.  :-)

fixed now.

https://codereview.chromium.org/1153003003/diff/20001/tool/sdk_expected_error...
File tool/sdk_expected_errors.txt (right):

https://codereview.chromium.org/1153003003/diff/20001/tool/sdk_expected_error...
tool/sdk_expected_errors.txt:1085: @SupportJsExtensionMethods()
On 2015/06/02 21:35:04, Leaf wrote:
> Typo - the definition uses ExtensionMethod vs ExtensionMethods

turns out it wasn't just that, it was a dependency issue: dart:core using it but
defined in a private lib.

Anyway the annotation is unused, so removed it.

[vsm, 2015-06-04 21:50:03]

lgtm

https://codereview.chromium.org/1153003003/diff/60001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1153003003/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:819: // one of these methods.
I see.  So, the virtual dispatch is happening on foo[dartx.add] rather than on
foo.add?

[Jennifer Messerly, 2015-06-04 21:57:46]

https://codereview.chromium.org/1153003003/diff/60001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1153003003/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:819: // one of these methods.
On 2015/06/04 21:50:02, vsm wrote:
> I see.  So, the virtual dispatch is happening on foo[dartx.add] rather than on
> foo.add? 

exactly. If we wanted to do it the other way, we'd add a stub like:

MyType.prototype[dartx.add] = function() { return this.add.apply(this,
arguments); }

in practice it'd be `this[name].apply(this, arguments);` and the stub function
closes over the name.

Overall it's worse, IMO. Maybe this should just be a comment instead of a TODO

[Jennifer Messerly, 2015-06-04 21:58:23]

Committed patchset #3 (id:60001) manually as
74bfe5b8cf67e5685a741cab949548d88060869b (presubmit successful).

[Leaf, 2015-06-05 00:07:22]

On 2015/06/04 21:58:23, John Messerly wrote:
> Committed patchset #3 (id:60001) manually as
> 74bfe5b8cf67e5685a741cab949548d88060869b (presubmit successful).

I think this may have broken the browser tests?  Haven't walked back through the
commits yet though.

[Jennifer Messerly, 2015-06-05 17:00:38]

ah, runtime_test.js ... yeah, forgot about that one. Taking a look. Will send
follow up.