More mirrors

More mirrors

A little more implementation and workarounds for Angular.  See #199.

R=jmesserly@google.com

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

[vsm, 2015-12-01 14:48:43]

Description was changed from

==========
More mirrors

A little more implementation and workarounds for Angular.  See #199.
==========

to

==========
More mirrors

A little more implementation and workarounds for Angular.  See #199.
==========

[vsm, 2015-12-01 14:48:43]

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

[Jennifer Messerly, 2015-12-01 16:41:40]

lgtm w/ one issue

https://codereview.chromium.org/1489043002/diff/1/tool/input_sdk/private/js_m...
File tool/input_sdk/private/js_mirrors.dart (right):

https://codereview.chromium.org/1489043002/diff/1/tool/input_sdk/private/js_m...
tool/input_sdk/private/js_mirrors.dart:127: return interfaces().map((t) => new
JsClassMirror._(t));
add a .toList() here?

and is this function allowed to return a fresh list each time? (I presume so)

https://codereview.chromium.org/1489043002/diff/1/tool/input_sdk/private/js_m...
tool/input_sdk/private/js_mirrors.dart:164: // TODO(vsm): Handle generic case. 
How should we represent an original
FWIW, the VM implementation:
https://github.com/dart-lang/sdk/blob/ab37dac248ecdb49a28ec6bb057c11e046d8be2...

it's pretty strange, but reflectClass always gets the unsubstituted generic (aka
"original declaration"). So this should hold:

reflect(obj).type.originalDeclaration == reflectClass(obj.runtimeType)

https://codereview.chromium.org/1489043002/diff/1/tool/sdk_expected_errors.txt
File tool/sdk_expected_errors.txt (right):

https://codereview.chromium.org/1489043002/diff/1/tool/sdk_expected_errors.tx...
tool/sdk_expected_errors.txt:277: warning: [DownCastComposite]
interfaces().map((t) => new JsClassMirror._(t)) (dynamic) will need runtime
check to cast to type List<ClassMirror> (dart:_js_mirrors, line 127, col 14)
this should go away after suggested fix :)

[vsm, 2015-12-01 17:00:01]

https://codereview.chromium.org/1489043002/diff/1/tool/input_sdk/private/js_m...
File tool/input_sdk/private/js_mirrors.dart (right):

https://codereview.chromium.org/1489043002/diff/1/tool/input_sdk/private/js_m...
tool/input_sdk/private/js_mirrors.dart:127: return interfaces().map((t) => new
JsClassMirror._(t));
On 2015/12/01 16:41:39, John Messerly wrote:
> add a .toList() here?

Done.
 
> and is this function allowed to return a fresh list each time? (I presume so)

Nothing in the docs suggesting either way.

https://codereview.chromium.org/1489043002/diff/1/tool/input_sdk/private/js_m...
tool/input_sdk/private/js_mirrors.dart:164: // TODO(vsm): Handle generic case. 
How should we represent an original
On 2015/12/01 16:41:39, John Messerly wrote:
> FWIW, the VM implementation:
>
https://github.com/dart-lang/sdk/blob/ab37dac248ecdb49a28ec6bb057c11e046d8be2...
> 
> it's pretty strange, but reflectClass always gets the unsubstituted generic
(aka
> "original declaration"). So this should hold:
> 
> reflect(obj).type.originalDeclaration == reflectClass(obj.runtimeType)

We don't implement isOriginalDeclaration yet.  We don't really have an object
for that at the moment either.

https://codereview.chromium.org/1489043002/diff/1/tool/sdk_expected_errors.txt
File tool/sdk_expected_errors.txt (right):

https://codereview.chromium.org/1489043002/diff/1/tool/sdk_expected_errors.tx...
tool/sdk_expected_errors.txt:277: warning: [DownCastComposite]
interfaces().map((t) => new JsClassMirror._(t)) (dynamic) will need runtime
check to cast to type List<ClassMirror> (dart:_js_mirrors, line 127, col 14)
On 2015/12/01 16:41:39, John Messerly wrote:
> this should go away after suggested fix :)

Still have the JS one.  Is there a way to tell it '()->List<Type>'?

[Jennifer Messerly, 2015-12-01 17:13:49]

https://codereview.chromium.org/1489043002/diff/1/tool/sdk_expected_errors.txt
File tool/sdk_expected_errors.txt (right):

https://codereview.chromium.org/1489043002/diff/1/tool/sdk_expected_errors.tx...
tool/sdk_expected_errors.txt:277: warning: [DownCastComposite]
interfaces().map((t) => new JsClassMirror._(t)) (dynamic) will need runtime
check to cast to type List<ClassMirror> (dart:_js_mirrors, line 127, col 14)
On 2015/12/01 17:00:01, vsm wrote:
> On 2015/12/01 16:41:39, John Messerly wrote:
> > this should go away after suggested fix :)
> 
> Still have the JS one.  Is there a way to tell it '()->List<Type>'?

No. But we should have it always trust JS builtin (or to put it another way,
allow downward inference to fix the type of that expression). I think we have
this request sitting in a github issue somewhere :)

[vsm, 2015-12-01 19:01:35]

Description was changed from

==========
More mirrors

A little more implementation and workarounds for Angular.  See #199.
==========

to

==========
More mirrors

A little more implementation and workarounds for Angular.  See #199.

R=jmesserly@google.com

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

[vsm, 2015-12-01 19:01:36]

Committed patchset #2 (id:20001) manually as
eded9e240c099f8ecb1294187f416663e527e49f (presubmit successful).