Update dart:mirrors to use Symbol.

Update dart:mirrors to use Symbol.

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

[ahe, 2013-04-11 15:14:21]

I'll attempt to make the required changes to the VM, unless Ivan would rather do
so himself.

However, I'd like to get the API changes reviewed and ready.

[Johnni Winther, 2013-04-11 17:34:09]

lgtm

[gbracha, 2013-04-11 18:12:20]

https://codereview.chromium.org/14173005/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/14173005/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:37: Map<Symbol, LibraryMirror> get libraries;
So, didn't we just change this to be a map of URIs to library mirrors? After
discussing with Ivan, I am satisfied that this will work.

https://codereview.chromium.org/14173005/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:186: [Map<Symbol,Object> namedArguments]);
Just an observation: this is an excellent example of how the use of Symbol
without language support makes the API hard to use. Until now, you could use a
literal map as an argument, but since our literal maps only work with string
keys, that is no longer possible.

[ahe, 2013-04-11 20:12:27]

I have updated (and extended) the test to use Symbol, and gotten it to work. 
There is still a bit of work to be done so I get get rid of Symbol.getName (or
make it private).

But I'd really like to know what you think about the current approach, and what
I'd have to do to get this CL approved.

https://codereview.chromium.org/14173005/diff/1/dart/sdk/lib/mirrors/mirrors....
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/14173005/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:37: Map<Symbol, LibraryMirror> get libraries;
On 2013/04/11 18:12:20, gbracha wrote:
> So, didn't we just change this to be a map of URIs to library mirrors? After
> discussing with Ivan, I am satisfied that this will work.

The CL from Johnni was to modify the copy of this API that dartdoc uses.  Johnni
has not yet been able to consolidate the two (I'm not sure what remains).

I think I have a way to modify the the Dart code so this returns a Map<Uri,
LibraryMirror>.  Let me know if you want me to address that.

Also, I have added a TODO regarding LibraryMirror.url.  I think the type should
be Uri, and it should be renamed to LibraryMirror.uri (that is an i (eye)
instead of an l (ell)).

https://codereview.chromium.org/14173005/diff/1/dart/sdk/lib/mirrors/mirrors....
dart/sdk/lib/mirrors/mirrors.dart:186: [Map<Symbol,Object> namedArguments]);
On 2013/04/11 18:12:20, gbracha wrote:
> Just an observation: this is an excellent example of how the use of Symbol
> without language support makes the API hard to use. Until now, you could use a
> literal map as an argument, but since our literal maps only work with string
> keys, that is no longer possible.

I agree.  I had the same issue when updating Function.apply.  Fortunately, there
only seems to be two tests that use Function.apply.

Kasper and Lars has promised me that they'll bring up revising the map literal
syntax to allow more general expressions as keys on the next language meeting.

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/mirrors_...
File dart/runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/mirrors_...
dart/runtime/lib/mirrors_impl.dart:23: class _PrivateSymbol implements Symbol {
This part is work in progress.  My intention is to eliminate this class before I
submit.

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/mirrors_...
dart/runtime/lib/mirrors_impl.dart:38: String _n(Symbol symbol) {
I should probably find a better name for this method.

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/mirrors_...
dart/runtime/lib/mirrors_impl.dart:49: return new Symbol((name ==
'<TODO:unnamed>') ? '' : name);
I need to be able to turn validation of Symbols of for the mirror API.  I think
this is also needed to implement Invocation (nee InvocationMirror) correctly.

The trouble is that I need a way to access something private from both dart:core
and dart:mirrors.  I think I may be able to use _LocalObjectMirrorImpl._invoke
do so.  Stay tuned.

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/symbol_p...
File dart/runtime/lib/symbol_patch.dart (right):

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/symbol_p...
dart/runtime/lib/symbol_patch.dart:36: if (name.isEmpty) return name;
Note to self: I need to update the symbol_test.dart to cover this case.

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/symbol_p...
dart/runtime/lib/symbol_patch.dart:58: /* patch */ static String getName(Symbol
symbol) => symbol._name;
I should not submit this change.

https://codereview.chromium.org/14173005/diff/10001/dart/sdk/lib/core/symbol....
File dart/sdk/lib/core/symbol.dart (right):

https://codereview.chromium.org/14173005/diff/10001/dart/sdk/lib/core/symbol....
dart/sdk/lib/core/symbol.dart:24: // TODO(ahe): Who to remove this?
I meant "How to remove this?".  I think I know now.  Using reflection directly.

[ahe, 2013-04-11 21:15:26]

I can definitely access the private field '_name' of a symbol using reflection:

String _n(Symbol symbol) {
  _LocalInstanceMirrorImpl symbolMirror = _Mirrors.reflect(symbol);
  _LocalInstanceMirrorImpl nameMirror =
      _LocalObjectMirrorImpl._getField(symbolMirror, '_name');
  return nameMirror.reflectee;
}

However, I get a stack overflow when I using reflection to get around validation
in the Symbol constructor:

Symbol _s(String name) {
  if (name == null) return null;
  if (name is! String) throw new ArgumentError('name is not a String');
  _LocalInstanceMirrorImpl symbol = _Mirrors.reflect(const Symbol(''));
  _LocalClassMirrorImpl symbolClass = instanceMirror.type;
  return ClassMirror._invokeConstructor(symbolClass, '_unvalidated', [name]);
}

I think I'll try writing some C++ code that creates an unvalidated Symbol
instance.  But not today :-)

[Ivan Posva, 2013-04-12 04:16:18]

With the amount of "This part is work in progress" and "do not submit this
change", this is not ready for submission at this point.

Overall it looks to me as if it is not really clear yet how to use these Symbols
and where they will lead us.

-Ivan

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/symbol_p...
File dart/runtime/lib/symbol_patch.dart (right):

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/symbol_p...
dart/runtime/lib/symbol_patch.dart:58: /* patch */ static String getName(Symbol
symbol) => symbol._name;
On 2013/04/11 20:12:27, ahe wrote:
> I should not submit this change.

Why not?

https://codereview.chromium.org/14173005/diff/10001/dart/sdk/lib/mirrors/mirr...
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/14173005/diff/10001/dart/sdk/lib/mirrors/mirr...
dart/sdk/lib/mirrors/mirrors.dart:187: [Map<Symbol,Object> namedArguments]);
Map<Symbol, Object> here and other places.

https://codereview.chromium.org/14173005/diff/10001/dart/sdk/lib/mirrors/mirr...
dart/sdk/lib/mirrors/mirrors.dart:438: * The arguments must be instances of
[InstanceMirror], [num],
Something is missing here.

[ahe, 2013-04-15 12:34:02]

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/symbol_p...
File dart/runtime/lib/symbol_patch.dart (right):

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/symbol_p...
dart/runtime/lib/symbol_patch.dart:36: if (name.isEmpty) return name;
On 2013/04/11 20:12:27, ahe wrote:
> Note to self: I need to update the symbol_test.dart to cover this case.

Done.

https://codereview.chromium.org/14173005/diff/10001/dart/runtime/lib/symbol_p...
dart/runtime/lib/symbol_patch.dart:58: /* patch */ static String getName(Symbol
symbol) => symbol._name;
On 2013/04/12 04:16:18, Ivan Posva wrote:
> On 2013/04/11 20:12:27, ahe wrote:
> > I should not submit this change.
> 
> Why not?

Because this method needs to be in the mirror API, not the core library.

https://codereview.chromium.org/14173005/diff/10001/dart/sdk/lib/mirrors/mirr...
File dart/sdk/lib/mirrors/mirrors.dart (right):

https://codereview.chromium.org/14173005/diff/10001/dart/sdk/lib/mirrors/mirr...
dart/sdk/lib/mirrors/mirrors.dart:187: [Map<Symbol,Object> namedArguments]);
On 2013/04/12 04:16:18, Ivan Posva wrote:
> Map<Symbol, Object> here and other places.

Done.

https://codereview.chromium.org/14173005/diff/10001/dart/sdk/lib/mirrors/mirr...
dart/sdk/lib/mirrors/mirrors.dart:438: * The arguments must be instances of
[InstanceMirror], [num],
On 2013/04/12 04:16:18, Ivan Posva wrote:
> Something is missing here.

Done.

[ahe, 2013-04-15 13:08:03]

This CL is ready for a complete review now.

https://codereview.chromium.org/14173005/diff/28001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart
(right):

https://codereview.chromium.org/14173005/diff/28001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/mirrors_patch.dart:28: throw
new UnimplementedError('MirrorSystem.getName is not yet implemented '
Implementing this method in dart2js is complicated due to minification.  It is
beyond the scope of this CL.  The mirror system in dart2js is barely implemented
anyways, and this CL is paving the way to get it fully implemented.

[Søren Gjesse, 2013-04-15 14:08:31]

lgtm

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/mirrors_...
File dart/runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/mirrors_...
dart/runtime/lib/mirrors_impl.dart:68: _LocalMirrorSystemImpl(Map<String,
LibraryMirror> libraries, this.isolate)
Four space indent - more below.

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/mirrors_...
File dart/runtime/lib/mirrors_patch.dart (right):

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/mirrors_...
dart/runtime/lib/mirrors_patch.dart:6: import "dart:_collection-dev" as
_collection_dev;
Maybe use a different "as" name (e.g. _mirror) to indicate what it actually is.
And some todo to move it to a separate internal library.

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/symbol_p...
File dart/runtime/lib/symbol_patch.dart (right):

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/symbol_p...
dart/runtime/lib/symbol_patch.dart:6: /* patch */ const Symbol(String name)
Four space indent.

https://codereview.chromium.org/14173005/diff/28002/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/lib/collection_dev_patch.dart
(right):

https://codereview.chromium.org/14173005/diff/28002/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/collection_dev_patch.dart:6:
patch const Symbol(String name)
Fits one line.

[ahe, 2013-04-15 14:29:55]

Thank you, Søren.

I'm going to submit this now, and deal with any follow-up comments from Ivan and
Gilad later.

Cheers,
Peter

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/mirrors_...
File dart/runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/mirrors_...
dart/runtime/lib/mirrors_impl.dart:68: _LocalMirrorSystemImpl(Map<String,
LibraryMirror> libraries, this.isolate)
On 2013/04/15 14:08:31, Søren Gjesse wrote:
> Four space indent - more below.

Done.

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/mirrors_...
File dart/runtime/lib/mirrors_patch.dart (right):

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/mirrors_...
dart/runtime/lib/mirrors_patch.dart:6: import "dart:_collection-dev" as
_collection_dev;
On 2013/04/15 14:08:31, Søren Gjesse wrote:
> Maybe use a different "as" name (e.g. _mirror) to indicate what it actually
is.
> And some todo to move it to a separate internal library.

Done.  (I called it _symbol_dev).

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/symbol_p...
File dart/runtime/lib/symbol_patch.dart (right):

https://codereview.chromium.org/14173005/diff/28002/dart/runtime/lib/symbol_p...
dart/runtime/lib/symbol_patch.dart:6: /* patch */ const Symbol(String name)
On 2013/04/15 14:08:31, Søren Gjesse wrote:
> Four space indent.

Done.

https://codereview.chromium.org/14173005/diff/28002/dart/sdk/lib/_internal/co...
File
dart/sdk/lib/_internal/compiler/implementation/lib/collection_dev_patch.dart
(right):

https://codereview.chromium.org/14173005/diff/28002/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/collection_dev_patch.dart:6:
patch const Symbol(String name)
On 2013/04/15 14:08:31, Søren Gjesse wrote:
> Fits one line.

I think the style guide recommends putting it on a separate line.

[ahe, 2013-04-15 15:01:23]

Committed patchset #11 manually as r21460.