Implement invoke, setField, and getField (unminified).

Implement invoke, setField, and getField (unminified).

R=johnniwinther@google.com, ngeoffray@google.com

Committed: https://code.google.com/p/dart/source/detail?r=22722

[ahe, 2013-05-07 23:59:11]

Untested: I cannot guarantee that no baby seals will die if you run this code.

Besides the occasional baby seal, this code is ready for review, but I may find
some problems when I run all the tests when I'm at my desktop machine.

https://codereview.chromium.org/14646031/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart
(right):

https://codereview.chromium.org/14646031/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:39:
constructor, () => new _ClassMirror(className, constructor));
This isn't right.  Restore the expando and figure out why it didn't work.

[Johnni Winther, 2013-05-08 08:22:07]

lgtm

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:338: if (uri.scheme
!= 'dart' && !uri.path.startsWith('_')) {
What are you trying to compute in this block?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/js_rti.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:31: int get
hashCode => _typeName.hashCode;
How about multiplying with a constant prime?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart
(right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:23:
_MirrorSystem _currentMirrorSystem = new _MirrorSystem();
How about making this final so it will be created upon first access?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:41: throw
new UnsupportedError('Cannot find constructor for: $className');
The error message seems a bit internal. Maybe change 'constructor' to 'class'.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:59:
ClassMirror get type => reflectClass(reflectee.runtimeType);
What about reflection on [:null:]?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:63:
[Map<String,Object> namedArguments]) {
I think we have an inconsistency in the signatures for invoke and invokeAsync:
One uses Object as type argument, the other uses raw types and dynamic.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:104:
fieldName, JSInvocationMirror.SETTER, 'set\$${_n(fieldName)}', [arg]);
'set\$...' doesn't work for minification, right?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:110:
fieldName, JSInvocationMirror.GETTER, 'get\$${_n(fieldName)}', []);
Ditto.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart
(right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart:1556:
return mapping[node] = element.declaration;
I'm not sure this works with the current invariant for patch library accesses.
What problems have you experienced with this?

https://codereview.chromium.org/14646031/diff/6002/dart/tests/lib/mirrors/mir...
File dart/tests/lib/mirrors/mirrors_test.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/tests/lib/mirrors/mir...
dart/tests/lib/mirrors/mirrors_test.dart:37: instMirror.setFieldAsync(const
Symbol('field'), 44);
How unorthodox to use 44 ;)

[ahe, 2013-05-08 08:57:43]

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:338: if (uri.scheme
!= 'dart' && !uri.path.startsWith('_')) {
On 2013/05/08 08:22:08, Johnni Winther wrote:
> What are you trying to compute in this block?

Top-level members.  However, some of the special classes like JsNull proved
problematic, so I decided to ignore all "private" implementation libraries.

Not sure this answers your question.  I'll add a comment when I understand your
question better.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/js_rti.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:31: int get
hashCode => _typeName.hashCode;
On 2013/05/08 08:22:08, Johnni Winther wrote:
> How about multiplying with a constant prime?

I thought so as well.  Then it occurred to me: how do we generally choose such
primes in the platform libraries to avoid collision?  I'm sure this isn't the
only class that's essentially a wrapper around a string.  Or perhaps I'm
overcomplicating things.  Please tell me I am :-)

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart
(right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:23:
_MirrorSystem _currentMirrorSystem = new _MirrorSystem();
On 2013/05/08 08:22:08, Johnni Winther wrote:
> How about making this final so it will be created upon first access?

I think making it final is a good idea, but I don't think it changes its
creation time :-)

I actually meant to make it final.  Good catch.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:41: throw
new UnsupportedError('Cannot find constructor for: $className');
On 2013/05/08 08:22:08, Johnni Winther wrote:
> The error message seems a bit internal. Maybe change 'constructor' to 'class'.

No problem, but this is a short-term solution.  The exception is simply easier
to read than the error message from the bowels of Expando on line 43.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:59:
ClassMirror get type => reflectClass(reflectee.runtimeType);
On 2013/05/08 08:22:08, Johnni Winther wrote:
> What about reflection on [:null:]?

The object "null" is an instance of a subclass of Object.  Hence it implements
runtimeType.  But I'm sure I'll have a hard time finding its constructor as I
have for all other intercepted classes.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:104:
fieldName, JSInvocationMirror.SETTER, 'set\$${_n(fieldName)}', [arg]);
On 2013/05/08 08:22:08, Johnni Winther wrote:
> 'set\$...' doesn't work for minification, right?

Yes.  That could have been clearer.  The CL description contains the word
"unminified" in parens to indicate that this is only works in unminified mode.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart
(right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart:1556:
return mapping[node] = element.declaration;
On 2013/05/08 08:22:08, Johnni Winther wrote:
> I'm not sure this works with the current invariant for patch library accesses.
> What problems have you experienced with this?

Good you spotted this, then.  The problem is that a use of "reflectClass"
resolved to the implementation in mirrors_patch.dart.  I think I know how to
work around this problem and can file a bug we can look at later.

[ngeoffray, 2013-05-08 12:34:05]

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:40: = new
Map<String, Link<Element>>();
I think this should be in the world. It will be the same for both the codegen
enqueuer and the resolution enqueuer right? So putting it in world will avoid
building it twice.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:291: if
(Elements.isUnresolved(element)) {
Could you just avoid adding them? Also, because you're iterating over the
contained elements of libraries and classes, how can you get unresolved?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:343: } else if
(element.isClass() && !element.isMixinApplication) {
What could go wrong with a mixin application?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:350:
addMemberByName(link.head.element);
Why do you want to add type parameters? Potential future uses?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:19:
result.forEach((Symbol key, value) {
what?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:140: return
receiver.noSuchMethod(this);
Please add a comment why you have to explicitly call noSuchMethod. Isn't the
apply taking care of that for you?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart
(right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:16: class
_MirrorSystem implements MirrorSystem {
What can you do with an instance of this class?

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:39: //
Probably an incepted class.
intercepted

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3653: final bool
isSymbolConstructor = element == compiler.symbolConstructor;
Remove final.

[ahe, 2013-05-08 21:47:53]

PTAL.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:40: = new
Map<String, Link<Element>>();
On 2013/05/08 12:34:06, ngeoffray wrote:
> I think this should be in the world. It will be the same for both the codegen
> enqueuer and the resolution enqueuer right? So putting it in world will avoid
> building it twice.

I'm really opposed to the class World.  I'll move it to the task.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:291: if
(Elements.isUnresolved(element)) {
On 2013/05/08 12:34:06, ngeoffray wrote:
> Could you just avoid adding them? Also, because you're iterating over the
> contained elements of libraries and classes, how can you get unresolved?

This is defensive programming.  Whenever you look at an element, you better make
sure that it isn't erroneous.

I have filed http://dartbug.com/10541 to improve this.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:343: } else if
(element.isClass() && !element.isMixinApplication) {
On 2013/05/08 12:34:06, ngeoffray wrote:
> What could go wrong with a mixin application?

Not sure. Added a TODO.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/enqueue.dart:350:
addMemberByName(link.head.element);
On 2013/05/08 12:34:06, ngeoffray wrote:
> Why do you want to add type parameters? Potential future uses?

Yes.  testNames in mirrors_test.dart will test that.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:19:
result.forEach((Symbol key, value) {
On 2013/05/08 12:34:06, ngeoffray wrote:
> what?

Yeah.  WTF? Reverted.

I misdiagnosed this exception and made a stupid "fix":

  Caught NoSuchMethodError : method not found: '<unknown>'
  Receiver: null
  Arguments: []
  TypeError: Cannot call method 'forEach$1' of undefined
      at $._symbolMapToStringMap (out.js:8246:7)
      at _ClassMirror.$$.Object.noSuchMethod$1 (out.js:6643:132)
      at _ClassMirror.eval [as get$owner] (eval at <anonymous>
(out.js:10595:34), <anonymous>:2:13)
      at $.testLibraryUri (out.js:10026:62)
      at mainWithArgument_anon7.$$.mainWithArgument_anon7.call$0 (out.js:7126:7)
      at TestCase.$$.TestCase.testFunction$0 (out.js:2162:30)
      at TestCase.$$.TestCase._runTest$0 (out.js:2187:18)
      at TestCase.$$.TestCase._run$0 (out.js:2209:16)
      at BoundClosure$0.$$.BoundClosure$0.call$0 (out.js:7191:34)
      at $._guardAsync (out.js:7716:18)

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:140: return
receiver.noSuchMethod(this);
On 2013/05/08 12:34:06, ngeoffray wrote:
> Please add a comment why you have to explicitly call noSuchMethod. Isn't the
> apply taking care of that for you?

Sorry, I thought it was obvious.

If receiver doesn't implement "name", "method" is null/undefined.  So this
becomes an NPE when it should invoke noSuchMethod.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart
(right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:16: class
_MirrorSystem implements MirrorSystem {
On 2013/05/08 12:34:06, ngeoffray wrote:
> What can you do with an instance of this class?

Not much yet.  But all the tests in mirrors_test.dart expect such an instance as
their first argument.  The most important features this class will expose
eventually is "findLibrary" and the "libraries" getter.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:63:
[Map<String,Object> namedArguments]) {
On 2013/05/08 08:22:08, Johnni Winther wrote:
> I think we have an inconsistency in the signatures for invoke and invokeAsync:
> One uses Object as type argument, the other uses raw types and dynamic.

I should clean this up in the VM as well, but I'd rather not touch the VM in
this CL.  So I filed http://dartbug.com/10542.

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3653: final bool
isSymbolConstructor = element == compiler.symbolConstructor;
On 2013/05/08 12:34:06, ngeoffray wrote:
> Remove final.

Why?

[ahe, 2013-05-13 10:28:05]

Ping.

[ngeoffray, 2013-05-13 10:31:40]

LGTM

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/14646031/diff/6002/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3653: final bool
isSymbolConstructor = element == compiler.symbolConstructor;
On 2013/05/08 21:47:53, ahe wrote:
> On 2013/05/08 12:34:06, ngeoffray wrote:
> > Remove final.
> 
> Why?

Consistency? We never use "final" for local variables.

[ahe, 2013-05-15 08:11:39]

Committed patchset #4 manually as r22722 (presubmit successful).