Support for dart:typed_data

Support for dart:typed_data

Fixes #441
Fixes #444

R=jmesserly@google.com

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

[vsm, 2016-02-09 20:54:28]

Description was changed from

==========
Support for dart:typed_data

Fixes #441
Fixes #444
==========

to

==========
Support for dart:typed_data

Fixes #441
Fixes #444
==========

[vsm, 2016-02-09 20:54:28]

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

[Jennifer Messerly, 2016-02-10 00:20:30]

LGTM, with one comment about the change to setType/realRuntimeType which I don't
think should be needed. We may have another bug there, or we are somehow special
casing JSArray...

(it is important that all constructors call setType somehow for a generic -- but
for your fix to work, you must've been doing that already?)

https://codereview.chromium.org/1680263002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1295: if (node.isStatic) {
Perhaps factor this out into _emitNativeFunctionBody?

https://codereview.chromium.org/1680263002/diff/40001/test/browser/language_t...
File test/browser/language_tests.js (right):

https://codereview.chromium.org/1680263002/diff/40001/test/browser/language_t...
test/browser/language_tests.js:329: // TODO(vsm): Right shift should not
propagate sign
spaces instead of tabs?

https://codereview.chromium.org/1680263002/diff/40001/test/codegen/expect/dom...
File test/codegen/expect/dom/dom.js (right):

https://codereview.chromium.org/1680263002/diff/40001/test/codegen/expect/dom...
test/codegen/expect/dom/dom.js:22: return this["[]"](index);
Hmm. This looks a bit dubious. We never use "[]" or "[]=" as a property name as
it's hard to read. We always use "get" and "set".

I wonder if this dart2js annotation is supposed to be interpreted as "use native
indexing"

If so, we can support that. We'll need some code when we emit IndexExpression to
understand it.

Anyway would be fine to file this as a bug & cross that bridge when we get
there.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/classes.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/classes.dart:355: // invoked on the generic type (e.g.,
JSArray<dynamic>, not JSArray<int>).
Given a generic type T, it's possible to get its raw type R, then from that you
can get its extension type. This is all handled by dart.generic

So it shouldn't be necessary to put this here. We can already find the type for
List<int> and such, AFAIK. I'll add some code that should work on the other end.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/classes.dart:356: $obj.__proto__[$_extensionType] = type;
(possibly moot based on above comment)

I think this should be $type in the quasiquote scheme. (Whether this makes any
difference, I'm not sure. We really need to change the way the SDK libs are
written... it's kind of in an awkward place now)

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/operations.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/operations.dart:169: actual == $int && type == $double)
return true;
should we put this logic in isSubtype?

at runtime we have no way of distinguishing between doubles and ints. For
example, if I have a List<double> it may contain ints too:

var list = <double>[1.0, 1.5, 2.0];
print(list.runtimeType); // List<double>
print(list[0].runtimeType); // int
print(list[1].runtimeType); // double
print(list[2].runtimeType); // int

Feels like it'd be more consistent to allow `int <: double` subtyping at
runtime.

Otherwise I think there are places we might call subtype instead of
strongInstanceOf and get the wrong result...

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/rtti.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:101: result = $obj.runtimeType;
huh, the old code here looks broken. Shouldn't this go through:

    obj[dartx.runtimeType] ?

I'm not sure how user-defined Iterable/List types can work otherwise:

class MyIterable extends Iterable {
  get runtimeType => String; // Take that, type system! ;-)
}

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:105: result = $obj[$_extensionType];
we can just go with realRuntimeType here, right? That's the fallback.

(I have some suggestions for that below)

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:108: // TODO(vsm): Return JSObject?
(this may be moot based on realRuntimeType comment above) We should return
something. Object.runtimeType shouldn't return null.

Do we want to pretend all objects inherit from Dart's Object? If so,
core.Object's runtimeType is the universal fallback.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:136: result = $obj[$_extensionType];
Fallback to the constructor should find our type, that was set via `setType`.
That should be the extension type already. For example we set type to
`JSArray<T>` for Dart lists.

It may not be entirely obvious, but for JSArray<T> we set up the prototype chain
is like:

    obj --> JSArray<T>.prototype --> Array.prototype.

In one operation, we simultaneously record the correct Dart "real runtime type",
as well as getting the correctly typed (closing over "T") methods for
JSArray<T>, and in general, all of our operations (realRuntimeType, but anything
using reflection) will find the correct Dart metadata. Method tearoffs, etc.

If we don't set this up, things can go wrong:
* when we look up signatures/metadata it will break.
* we'd JSArray<dynamic> members which would be no good.

Because of the inheritance relationship, obj[_extensionType] will point back to
JSArray, from any JSArray.

[vsm, 2016-02-11 22:36:06]

Landing, but still need to shake out the runtime type tagging.

https://codereview.chromium.org/1680263002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1295: if (node.isStatic) {
On 2016/02/10 00:20:30, John Messerly wrote:
> Perhaps factor this out into _emitNativeFunctionBody?

Done.

https://codereview.chromium.org/1680263002/diff/40001/test/browser/language_t...
File test/browser/language_tests.js (right):

https://codereview.chromium.org/1680263002/diff/40001/test/browser/language_t...
test/browser/language_tests.js:329: // TODO(vsm): Right shift should not
propagate sign
On 2016/02/10 00:20:30, John Messerly wrote:
> spaces instead of tabs?

Done.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/classes.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/classes.dart:355: // invoked on the generic type (e.g.,
JSArray<dynamic>, not JSArray<int>).
On 2016/02/10 00:20:30, John Messerly wrote:
> Given a generic type T, it's possible to get its raw type R, then from that
you
> can get its extension type. This is all handled by dart.generic
> 
> So it shouldn't be necessary to put this here. We can already find the type
for
> List<int> and such, AFAIK. I'll add some code that should work on the other
end.

I need this to go the other way around - i.e., to map the raw object back to the
generic type.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/classes.dart:356: $obj.__proto__[$_extensionType] = type;
On 2016/02/10 00:20:30, John Messerly wrote:
> (possibly moot based on above comment)
> 
> I think this should be $type in the quasiquote scheme. (Whether this makes any
> difference, I'm not sure. We really need to change the way the SDK libs are
> written... it's kind of in an awkward place now)

Done.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/operations.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/operations.dart:169: actual == $int && type == $double)
return true;
On 2016/02/10 00:20:30, John Messerly wrote:
> should we put this logic in isSubtype?
> 
> at runtime we have no way of distinguishing between doubles and ints. For
> example, if I have a List<double> it may contain ints too:
> 
> var list = <double>[1.0, 1.5, 2.0];
> print(list.runtimeType); // List<double>
> print(list[0].runtimeType); // int
> print(list[1].runtimeType); // double
> print(list[2].runtimeType); // int
> 
> Feels like it'd be more consistent to allow `int <: double` subtyping at
> runtime.
> 
> Otherwise I think there are places we might call subtype instead of
> strongInstanceOf and get the wrong result...

I thought about doing that, but it's a bigger change.  E.g.,: this throws (in
checked mode) today on the VM and dart2js:

void main() {
  List l1 = <int>[1, 2];

  List<double> l2 = l1;
  print(l2);
}

I do agree that it's more consistent though with how we (and dart2js) treat
numbers.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/rtti.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:101: result = $obj.runtimeType;
On 2016/02/10 00:20:30, John Messerly wrote:
> huh, the old code here looks broken. Shouldn't this go through:
> 
>     obj[dartx.runtimeType] ?
> 
> I'm not sure how user-defined Iterable/List types can work otherwise:
> 
> class MyIterable extends Iterable {
>   get runtimeType => String; // Take that, type system! ;-)
> }

For object properties, we either:

(1) use the unsymbolized name (obj.runtimeType)

or

(2) use staticDispatch (dart.runtimeType(obj))

The latter does the right thing on null.  There was bug here - we need to do
staticDispatch on potential extension types - fixed in codegen.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:105: result = $obj[$_extensionType];
On 2016/02/10 00:20:30, John Messerly wrote:
> we can just go with realRuntimeType here, right? That's the fallback.
> 
> (I have some suggestions for that below)

Yeah, was try to avoid the redundant check on primitive types.  Just refactored
out the common part.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:108: // TODO(vsm): Return JSObject?
On 2016/02/10 00:20:30, John Messerly wrote:
> (this may be moot based on realRuntimeType comment above) We should return
> something. Object.runtimeType shouldn't return null.
> 
> Do we want to pretend all objects inherit from Dart's Object? If so,
> core.Object's runtimeType is the universal fallback.

Removed

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:136: result = $obj[$_extensionType];
On 2016/02/10 00:20:30, John Messerly wrote:
> Fallback to the constructor should find our type, that was set via `setType`.
> That should be the extension type already. For example we set type to
> `JSArray<T>` for Dart lists.
> 
> It may not be entirely obvious, but for JSArray<T> we set up the prototype
chain
> is like:
> 
>     obj --> JSArray<T>.prototype --> Array.prototype.
> 
> In one operation, we simultaneously record the correct Dart "real runtime
type",
> as well as getting the correctly typed (closing over "T") methods for
> JSArray<T>, and in general, all of our operations (realRuntimeType, but
anything
> using reflection) will find the correct Dart metadata. Method tearoffs, etc.
> 
> If we don't set this up, things can go wrong:
> * when we look up signatures/metadata it will break.
> * we'd JSArray<dynamic> members which would be no good.
> 
> Because of the inheritance relationship, obj[_extensionType] will point back
to
> JSArray, from any JSArray.
> 
> 

Hmm, I think there are (at least) 3 interesting cases here:

(1) A generic extension type where we explicitly call setType.  E.g., <int>[1,
2, 3]
Here, the constructor is set up as you say.  Printing the real runtime type
shows JSArray<int> - good!

(2) A generic extension type where we don't call setType.  E.g., [1, 2, 3] or a
list we get from a JS API.
Here, the constructor is Array.  Printing the real runtime type shows
JSObject<Array> - a little weird.

(3) A non-generic extension type where we don't call setType (we can't as we get
it back from the DOM).   E.g., a Uint8List.
Here, the constructor is the native Uint8Array.  

In latter two cases, the _extensionType field maps back to the right type.

[vsm, 2016-02-11 22:36:35]

Description was changed from

==========
Support for dart:typed_data

Fixes #441
Fixes #444
==========

to

==========
Support for dart:typed_data

Fixes #441
Fixes #444

R=jmesserly@google.com

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

[vsm, 2016-02-11 22:36:36]

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

[Jennifer Messerly, 2016-02-11 23:04:57]

I can take a shot at cleaning this up if you want?

I'm trying to avoid redundant storage of data, which means more overhead...

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/classes.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/classes.dart:355: // invoked on the generic type (e.g.,
JSArray<dynamic>, not JSArray<int>).
On 2016/02/11 22:36:06, vsm wrote:
> On 2016/02/10 00:20:30, John Messerly wrote:
> > Given a generic type T, it's possible to get its raw type R, then from that
> you
> > can get its extension type. This is all handled by dart.generic
> > 
> > So it shouldn't be necessary to put this here. We can already find the type
> for
> > List<int> and such, AFAIK. I'll add some code that should work on the other
> end.
> 
> I need this to go the other way around - i.e., to map the raw object back to
the
> generic type.

I'm 99% sure this should not be needed. In JavaScript:

    obj.__proto__.contstructor == type

so you don't need the _extensionType field. That's what I'm saying.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/rtti.dart (right):

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:101: result = $obj.runtimeType;
On 2016/02/11 22:36:06, vsm wrote:
> On 2016/02/10 00:20:30, John Messerly wrote:
> > huh, the old code here looks broken. Shouldn't this go through:
> > 
> >     obj[dartx.runtimeType] ?
> > 
> > I'm not sure how user-defined Iterable/List types can work otherwise:
> > 
> > class MyIterable extends Iterable {
> >   get runtimeType => String; // Take that, type system! ;-)
> > }
> 
> For object properties, we either:
> 
> (1) use the unsymbolized name (obj.runtimeType)
> 
> or
> 
> (2) use staticDispatch (dart.runtimeType(obj))
> 
> The latter does the right thing on null.  There was bug here - we need to do
> staticDispatch on potential extension types - fixed in codegen.

yeah, I mean, this is a bug in how the hand-coded runtime code here is written.
Compiler would've generated the right thing.

I don't think my example about will work. Anyway, I'll file a bug.

https://codereview.chromium.org/1680263002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/rtti.dart:136: result = $obj[$_extensionType];
On 2016/02/11 22:36:06, vsm wrote:
> On 2016/02/10 00:20:30, John Messerly wrote:
> > Fallback to the constructor should find our type, that was set via
`setType`.
> > That should be the extension type already. For example we set type to
> > `JSArray<T>` for Dart lists.
> > 
> > It may not be entirely obvious, but for JSArray<T> we set up the prototype
> chain
> > is like:
> > 
> >     obj --> JSArray<T>.prototype --> Array.prototype.
> > 
> > In one operation, we simultaneously record the correct Dart "real runtime
> type",
> > as well as getting the correctly typed (closing over "T") methods for
> > JSArray<T>, and in general, all of our operations (realRuntimeType, but
> anything
> > using reflection) will find the correct Dart metadata. Method tearoffs, etc.
> > 
> > If we don't set this up, things can go wrong:
> > * when we look up signatures/metadata it will break.
> > * we'd JSArray<dynamic> members which would be no good.
> > 
> > Because of the inheritance relationship, obj[_extensionType] will point back
> to
> > JSArray, from any JSArray.
> > 
> > 
> 
> Hmm, I think there are (at least) 3 interesting cases here:
> 
> (1) A generic extension type where we explicitly call setType.  E.g., <int>[1,
> 2, 3]
> Here, the constructor is set up as you say.  Printing the real runtime type
> shows JSArray<int> - good!
> 
> (2) A generic extension type where we don't call setType.  E.g., [1, 2, 3] or
a
> list we get from a JS API.
> Here, the constructor is Array.  Printing the real runtime type shows
> JSObject<Array> - a little weird.

This shouldn't happen. setType must be called for a generic type. Without that,
you'll get the wrong generic methods, which is a much worse problem.

How are we getting generic types from JS? JS doesn't have reified generics ...
apologies if I'm super confused.

> 
> (3) A non-generic extension type where we don't call setType (we can't as we
get
> it back from the DOM).   E.g., a Uint8List.
> Here, the constructor is the native Uint8Array.  
> 
> In latter two cases, the _extensionType field maps back to the right type.

Since "setType" is how you populate _extensionType, I'm confused. :)

If I understand case (3), you want to map Array -> JSArray? Was that not working
before?

If it wasn't, the change here seems okay, but I don't get why you needed a
change to `setType`. That is what worries me.