dart:mirrors

dart:mirrors

This is a very bare bones version - just enough to get angular running.

See #199

R=jmesserly@google.com

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

[vsm, 2015-06-13 00:15:13]

Add spreadcalls flag check

[vsm, 2015-06-13 00:15:42]

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

[Jennifer Messerly, 2015-06-15 16:42:02]

wow, really neat to see progress on this!

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

https://codereview.chromium.org/1186683005/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:66: exportFrom(operations, [
I wonder if we should support `export *`

(ES6 modules have this feature)

https://codereview.chromium.org/1186683005/diff/20001/tool/build_sdk.sh
File tool/build_sdk.sh (right):

https://codereview.chromium.org/1186683005/diff/20001/tool/build_sdk.sh#newco...
tool/build_sdk.sh:19: --dart-sdk tool/generated_sdk -o lib/runtime/ dart:mirrors
\
does dart:js or dart:mirrors depend on eachother? if so are we blocked on #219?
I can take a look at it
quick hack would be to make an internal library, like dart:_all or something,
that imports the others. Though that'd get emitted into the output I think...

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/lib/mirr...
File tool/input_sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/lib/mirr...
tool/input_sdk/lib/mirrors/mirrors.dart:46: * across isolates, or a map with
keys and values that are all serializable across
long line, I'm guessing was from original code tho? maybe leave it then

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
File tool/input_sdk/private/js_mirrors.dart (right):

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
2015?

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:16: return JS('String',
'#.toString().substring(8, #.toString().length - 2)', symbol, symbol);
Not sure toString the best way to implement this? We could be able to get ahold
of our _name field:

    import 'dart:_internal' as _internal;

    String getName(Symbol symbol) => _internal.Symbol.getName(symbol);

(from input_sdk/lib/internal/symbol.dart)

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:29: return new
JsInstanceMirror._(reflectee);
silly: could be => ?

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:33: // FIXME(vsm): Might not be a class.
TODO? or did you mean to fix before landing?

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:37: final dynamic _dart = JS('', 'dart');
Not sure this field is worth it? if we're hardcoding "dart" global, it can be
hard coded inside the the places it's used. The level of indirection doesn't do
anything right?

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:40: dynamic _dload(obj, String name) {
btw, if we wanted to make these APIs available statically. We could add a
private library to libraries.dart, with type signatures, and then import it. And
maybe have the compiler skip actual codegen for that library, since it's hand
coded, but it would still be able to generate imports to it.

filed https://github.com/dart-lang/dev_compiler/issues/220

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:53: Object _instance;
final?

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:57: Object get reflectee {
This could just make `reflectee` be the final field name, and skip `_instance` ?

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:73: InstanceMirror invoke(Symbol symbol,
List<dynamic> args, [Map<Symbol, dynamic> namedArgs]) {
long line

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:75: var result = _dsend(_instance, name,
args);
TODO: named args?

or maybe implement it:

    if (namedArgs != null) {
      args = new List.from(args)
      args.add(_toJsMap(namedArgs));
    }
    _dsend(...)

dynamic _toJsMap(Map<Symbol, dynamic> map) {
  var obj = JS('', '{}');
  map.forEach((Symbol key, value) {
    JS('', '#[#] = #', obj, getName(key), value);
  });
  return obj;
}

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:82: Symbol _simpleName;
final Symbol simpleName?

and init like:

JsClassMirror._(Type cls) {
  : _cls = cls,
    simpleName = new Symbol(JS('String', '#.name', cls));

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:84: List<InstanceMirror> _metadata;
these could probably be final too, but would need a bit more restructuring :)

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:97: var fn = JS('List<InstanceMirror>',
'#[dart.metadata]', _cls);
fyi, I don't think we parse this. (but we should)

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:98: _metadata = (fn == null) ? [] :
fn().map((i) => new JsInstanceMirror._(i));
this looks like it's assigning Iterable<InstanceMirror> to List<InstanceMirror>
?

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:106: InstanceMirror newInstance(Symbol
constructorName, List args, [Map<Symbol, dynamic> namedArgs]) {
long line

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:134: JsMethodMirror._(JsClassMirror cls,
this._method) : _name = getName(cls._simpleName) {
long line

[Leaf, 2015-06-15 17:53:09]

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

https://codereview.chromium.org/1186683005/diff/20001/lib/runtime/_operations...
lib/runtime/_operations.js:12: 'dart/collection',
Nit: Alphabetize?

https://codereview.chromium.org/1186683005/diff/20001/lib/runtime/_operations...
lib/runtime/_operations.js:18: ], function(exports, async, core, collection,
_js_helper, classes, errors, rtti, types) {
Nit: line length.

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

https://codereview.chromium.org/1186683005/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:66: exportFrom(operations, [
On 2015/06/15 16:42:01, John Messerly wrote:
> I wonder if we should support `export *`
> 
> (ES6 modules have this feature)

Not everything that is exported from the internal libraries needs to be exported
externally - some things are exported for use by other internal modules.  To be
honest, I'm also a really big fan of explicit imports and exports - I really
like to have a visible audit trail as to where the names are coming from (I
know, I know, "here's a nickle, go buy yourself a real IDE..." :)

[Jennifer Messerly, 2015-06-15 18:00:20]

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

https://codereview.chromium.org/1186683005/diff/20001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:66: exportFrom(operations, [
On 2015/06/15 17:53:09, Leaf wrote:
> On 2015/06/15 16:42:01, John Messerly wrote:
> > I wonder if we should support `export *`
> > 
> > (ES6 modules have this feature)
> 
> Not everything that is exported from the internal libraries needs to be
exported
> externally - some things are exported for use by other internal modules.  To
be
> honest, I'm also a really big fan of explicit imports and exports - I really
> like to have a visible audit trail as to where the names are coming from (I
> know, I know, "here's a nickle, go buy yourself a real IDE..." :)

Yeah, with real `export *` tooling should have no problem taking you to the
original definition.

I tend to favor `export *` if the entire library is public and is meant to all
be exposed as a part of the other one, which I think is true for `operations`,
at least as we currently use it.

I also can't remember things :) ... so if I have to add something to one place,
and then add it to a bunch of other lists to make it work ... it makes me
confused.

That said: longer term, we can import things without qualified name, and we
won't need to care about how short our library names are. At that point, we can
stop the `dart.` madness, remove the re-export entirely, and import things from
the appropriate lib.

[vsm, 2015-06-15 20:59:47]

thanks - PTAL

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

https://codereview.chromium.org/1186683005/diff/20001/lib/runtime/_operations...
lib/runtime/_operations.js:12: 'dart/collection',
On 2015/06/15 17:53:09, Leaf wrote:
> Nit: Alphabetize?

Done.

https://codereview.chromium.org/1186683005/diff/20001/lib/runtime/_operations...
lib/runtime/_operations.js:18: ], function(exports, async, core, collection,
_js_helper, classes, errors, rtti, types) {
On 2015/06/15 17:53:09, Leaf wrote:
> Nit: line length.

Done.

https://codereview.chromium.org/1186683005/diff/20001/tool/build_sdk.sh
File tool/build_sdk.sh (right):

https://codereview.chromium.org/1186683005/diff/20001/tool/build_sdk.sh#newco...
tool/build_sdk.sh:19: --dart-sdk tool/generated_sdk -o lib/runtime/ dart:mirrors
\
On 2015/06/15 16:42:01, John Messerly wrote:
> does dart:js or dart:mirrors depend on eachother? if so are we blocked on
#219?
> I can take a look at it
> quick hack would be to make an internal library, like dart:_all or something,
> that imports the others. Though that'd get emitted into the output I think...

No, I'm pretty sure dart:js and dart:mirrors depend on each other.  I added a
semi-bogus import to dart:js to work around this.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/lib/mirr...
File tool/input_sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/lib/mirr...
tool/input_sdk/lib/mirrors/mirrors.dart:46: * across isolates, or a map with
keys and values that are all serializable across
On 2015/06/15 16:42:01, John Messerly wrote:
> long line, I'm guessing was from original code tho? maybe leave it then

Yeah, this is the upstream - same version as the rest.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
File tool/input_sdk/private/js_mirrors.dart (right):

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2015/06/15 16:42:01, John Messerly wrote:
> 2015?

Done.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:16: return JS('String',
'#.toString().substring(8, #.toString().length - 2)', symbol, symbol);
On 2015/06/15 16:42:02, John Messerly wrote:
> Not sure toString the best way to implement this? We could be able to get
ahold
> of our _name field:
> 
>     import 'dart:_internal' as _internal;
> 
>     String getName(Symbol symbol) => _internal.Symbol.getName(symbol);
> 
> (from input_sdk/lib/internal/symbol.dart)

Done - thanks!

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:29: return new
JsInstanceMirror._(reflectee);
On 2015/06/15 16:42:02, John Messerly wrote:
> silly: could be => ?

Done.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:33: // FIXME(vsm): Might not be a class.
On 2015/06/15 16:42:02, John Messerly wrote:
> TODO? or did you mean to fix before landing?

I'll update the mirrors bug.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:37: final dynamic _dart = JS('', 'dart');
On 2015/06/15 16:42:02, John Messerly wrote:
> Not sure this field is worth it? if we're hardcoding "dart" global, it can be
> hard coded inside the the places it's used. The level of indirection doesn't
do
> anything right?

Leaf had been moving / hiding some of our runtime libs - so not sure we want to
keep this all in the global.dart namespace?

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:40: dynamic _dload(obj, String name) {
On 2015/06/15 16:42:02, John Messerly wrote:
> btw, if we wanted to make these APIs available statically. We could add a
> private library to libraries.dart, with type signatures, and then import it.
And
> maybe have the compiler skip actual codegen for that library, since it's hand
> coded, but it would still be able to generate imports to it.
> 
> filed https://github.com/dart-lang/dev_compiler/issues/220

thanks for filing!

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:53: Object _instance;
On 2015/06/15 16:42:01, John Messerly wrote:
> final?

Done.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:57: Object get reflectee {
On 2015/06/15 16:42:02, John Messerly wrote:
> This could just make `reflectee` be the final field name, and skip `_instance`
?

Done.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:73: InstanceMirror invoke(Symbol symbol,
List<dynamic> args, [Map<Symbol, dynamic> namedArgs]) {
On 2015/06/15 16:42:02, John Messerly wrote:
> long line

Done.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:75: var result = _dsend(_instance, name,
args);
On 2015/06/15 16:42:01, John Messerly wrote:
> TODO: named args?
> 
> or maybe implement it:
> 
>     if (namedArgs != null) {
>       args = new List.from(args)
>       args.add(_toJsMap(namedArgs));
>     }
>     _dsend(...)
> 
> dynamic _toJsMap(Map<Symbol, dynamic> map) {
>   var obj = JS('', '{}');
>   map.forEach((Symbol key, value) {
>     JS('', '#[#] = #', obj, getName(key), value);
>   });
>   return obj;
> }
> 
> 

Code stolen!  :-)

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:82: Symbol _simpleName;
On 2015/06/15 16:42:02, John Messerly wrote:
> final Symbol simpleName?
> 
> and init like:
> 
> JsClassMirror._(Type cls) {
>   : _cls = cls,
>     simpleName = new Symbol(JS('String', '#.name', cls));

Done.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:84: List<InstanceMirror> _metadata;
On 2015/06/15 16:42:01, John Messerly wrote:
> these could probably be final too, but would need a bit more restructuring :)

Yes, I also need to make this immutable (see comment below).

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:98: _metadata = (fn == null) ? [] :
fn().map((i) => new JsInstanceMirror._(i));
On 2015/06/15 16:42:02, John Messerly wrote:
> this looks like it's assigning Iterable<InstanceMirror> to
List<InstanceMirror>
> ?

Good catch! - this is the problem with just emitting type check warnings at
runtime - too easy to ignore.  :-/

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:106: InstanceMirror newInstance(Symbol
constructorName, List args, [Map<Symbol, dynamic> namedArgs]) {
On 2015/06/15 16:42:01, John Messerly wrote:
> long line

Done.

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:134: JsMethodMirror._(JsClassMirror cls,
this._method) : _name = getName(cls._simpleName) {
On 2015/06/15 16:42:01, John Messerly wrote:
> long line

Done.

[Jennifer Messerly, 2015-06-15 21:40:17]

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
File tool/input_sdk/private/js_mirrors.dart (right):

https://codereview.chromium.org/1186683005/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:37: final dynamic _dart = JS('', 'dart');
On 2015/06/15 20:59:47, vsm wrote:
> On 2015/06/15 16:42:02, John Messerly wrote:
> > Not sure this field is worth it? if we're hardcoding "dart" global, it can
be
> > hard coded inside the the places it's used. The level of indirection doesn't
> do
> > anything right?
> 
> Leaf had been moving / hiding some of our runtime libs - so not sure we want
to
> keep this all in the global.dart namespace?

eventually, yeah, things will move ... but we'll have to search for all of the
"dart." in JS snippets anyway at that point :)

the right long term solution is probably along the lines of
https://github.com/dart-lang/dev_compiler/issues/220

Not a big deal tho.

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

https://codereview.chromium.org/1186683005/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:105: // FIXME(vsm): This is only
populating the default constructor right now.
not a big deal but ... we usually use TODO instead of FIXME.

https://codereview.chromium.org/1186683005/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:112: // FIXME(vsm): Support named
constructors and named arguments.
here too

[Jennifer Messerly, 2015-06-15 21:40:28]

oops, forgot LGTM

[vsm, 2015-06-15 22:46:47]

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

https://codereview.chromium.org/1186683005/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:105: // FIXME(vsm): This is only
populating the default constructor right now.
On 2015/06/15 21:40:17, John Messerly wrote:
> not a big deal but ... we usually use TODO instead of FIXME.

Done.

https://codereview.chromium.org/1186683005/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/js_mirrors.dart:112: // FIXME(vsm): Support named
constructors and named arguments.
On 2015/06/15 21:40:16, John Messerly wrote:
> here too

Done.

[vsm, 2015-06-15 23:23:04]

Committed patchset #5 (id:80001) manually as
e3cb97170c8dce531887cf1969769cd523513d1d (presubmit successful).