Mirrors overhaul.

dart/sdk/lib/mirrors/mirrors.dart

Index: dart/sdk/lib/mirrors/mirrors.dart
diff --git a/dart/sdk/lib/mirrors/mirrors.dart b/dart/sdk/lib/mirrors/mirrors.dart
index 313026f26ee74379a00199a0e78258661820fd51..fc9b03fb70f2d2204b6ac070679491fef15aa245 100644
--- a/dart/sdk/lib/mirrors/mirrors.dart
+++ b/dart/sdk/lib/mirrors/mirrors.dart
@@ -12,6 +12,10 @@
 // 'myField='.  This allows us to assign unique names to getters and
 // setters for the purposes of member lookup.
 
+// Open questions:
+// Turn all getters into final fields?

[Johnni Winther, 2013/09/03 12:36:57]

Why?

+// Need a way to invoke super methods.
+
 /**
  * Basic reflection in Dart,
  * with support for introspection and dynamic evaluation.
@@ -74,8 +78,8 @@ abstract class MirrorSystem {
    * Returns an iterable of all libraries in the mirror system whose library
    * name is [libraryName].

[Johnni Winther, 2013/09/03 12:36:57]

Comment needs update.

[gbracha, 2013/10/03 22:11:25]

Yes, can we please fix these issues in the CL so that it really reflects where
we want to be?

[ahe, 2013/10/30 07:48:31]

Done.

    */
-  Iterable<LibraryMirror> findLibrary(Symbol libraryName) {
-    return libraries.values.where(
+  LibraryMirror findLibrary(Symbol libraryName) {
+    return libraries.values.singleWhere(

[Alan Knight, 2013/09/03 20:08:31]

Might this be more useful if it used firstWhere so as to let you provide an
orElse?

[ahe, 2013/09/03 20:14:30]

What I don't like about firstWhere is that it doesn't give me an error if the
library isn't unique.

We could have an optionally named parameter 'ifError' to this method.

[Alan Knight, 2013/09/03 20:30:56]

Does the error distinguish between the case of no match and multiple matches? It
seems awkward to have to get an error and then try to distinguish between the
two different problems. 

An advantage of firstWhere is that the implementation can stop looking once it
finds the first one, so for what I presume is the common case it's faster.

I can't help but note that if findInContext worked, quite a few cases where code
has to do this would both go away and get better results.

         (library) => library.simpleName == libraryName);
   }
 
@@ -112,12 +116,6 @@ abstract class MirrorSystem {
 external MirrorSystem currentMirrorSystem();
 
 /**
- * Creates a [MirrorSystem] for the isolate which is listening on
- * the [SendPort].
- */
-external Future<MirrorSystem> mirrorSystemOf(SendPort port);
-
-/**
  * Returns an [InstanceMirror] reflecting [reflectee].
  * If [reflectee] is function or an instance of a class
  * that has a [:call:] method, the returned instance mirror
@@ -149,12 +147,7 @@ external ClassMirror reflectClass(Type key);
  *
  * Every [Mirror] originates from some [MirrorSystem].
  */
-abstract class Mirror {
-  /**
-   * The [MirrorSystem] that contains this mirror.
-   */
-  MirrorSystem get mirrors;
-}
+abstract class Mirror {}
 
 /**
  * An [IsolateMirror] reflects an isolate.
@@ -266,6 +259,8 @@ abstract class DeclarationMirror implements Mirror {
 
   /**
    * The source location of this Dart language entity.
+   *
+   * This operation is optional and may return [:null:].
    */
   SourceLocation get location;
 
@@ -327,6 +322,7 @@ abstract class ObjectMirror implements Mirror {
                         List positionalArguments,
                         [Map<Symbol,dynamic> namedArguments]);
 
+

[Johnni Winther, 2013/09/03 12:36:57]

Extra line.

   /**
    * Invokes a getter and returns a mirror on the result. The getter
    * can be the implicit getter for a field or a user-defined getter
@@ -339,6 +335,19 @@ abstract class ObjectMirror implements Mirror {
    * in a scope that has access to the private members
    * of *o* (if *o* is a class or library) or the private members of the
    * class of *o* (otherwise).
+   *
+   * If this mirror is an [InstanceMirror], and [fieldName] denotes an instance
+   * method on its reflectee, the result of the invocation is an instance
+   * mirror on a closure corresponding to that method.
+   *
+   * If this mirror is a [LibraryMirror], and [fieldName] denotes a top-level
+   * method in the corresponding library, the result of the invocation is an
+   * instance mirror on a closure corresponding to that method.
+   *
+   * If this mirror is a [ClassMirror], and [fieldName] denotes a static method
+   * in the corresponding class, the result of the invocation is an instance
+   * mirror on a closure corresponding to that method.

[gbracha, 2013/10/03 22:11:25]

I'd tweak the wording a bit at some point, because "closure corresponding to a
method" is not very specific. We know we want the closurization per tha language
spec, and I'll eventually put in better wording, but this is ok for now.

[ahe, 2013/10/30 07:48:31]

A kind soul fixed that for me in another CL :-)

+   *
    * If the invocation returns a result *r*, this method returns
    * the result of calling [reflect](*r*).
    * If the invocation causes a compilation error
@@ -346,7 +355,8 @@ abstract class ObjectMirror implements Mirror {
    * If the invocation throws an exception *e* (that it does not catch)
    * this method throws *e*.
    */
-  /* TODO(turnidge): Handle ambiguous names.*/
+  // TODO(ahe): Remove stuff about scope and private members. [fieldName] is a
+  // capability giving access to private members.
   InstanceMirror getField(Symbol fieldName);
 
   /**
@@ -421,6 +431,20 @@ abstract class ObjectMirror implements Mirror {
    * in a scope that has access to the private members
    * of *o* (if *o* is a class or library) or the private members of the
    * class of *o*(otherwise).
+   *

[Johnni Winther, 2013/09/03 12:36:57]

Extra line.

+   *
+   * If this mirror is an [InstanceMirror], and [fieldName] denotes an instance
+   * method on its reflectee, the result of the invocation is an instance
+   * mirror on a closure corresponding to that method.
+   *
+   * If this mirror is a [LibraryMirror], and [fieldName] denotes a top-level
+   * method in the corresponding library, the result of the invocation is an
+   * instance mirror on a closure corresponding to that method.
+   *
+   * If this mirror is a [ClassMirror], and [fieldName] denotes a static method
+   * in the corresponding class, the result of the invocation is an instance
+   * mirror on a closure corresponding to that method.
+   *
    * The method returns a future *k*.
    * If the invocation returns a result *r*, *k* will be completed
    * with the result of calling [reflect](*r*).
@@ -521,6 +545,28 @@ abstract class InstanceMirror implements ObjectMirror {
    *             invocation.namedArguments);
    */
   delegate(Invocation invocation);
+
+  /**
+   * Returns closure for invoking the regular method named [name].
+   *
+   * If [:type.instanceLookup(name:] is a regular method, the result of this

[Johnni Winther, 2013/09/03 12:36:57]

'(name' -> '(name)'.
'is a regular method' -> 'returns a regular method'?
What is 'type'?

[gbracha, 2013/10/03 22:11:25]

We have made some tweaks to this wording in other CLs. Minor, see

https://codereview.chromium.org/25741005/diff/6001/sdk/lib/mirrors/mirrors.dart

reproduced below

/**
  552    * Returns a closure for invoking the regular method named [name].
  553    *
  554    * If [:type.instanceLookup(name):] returns a regular method, the result
of
  555    * this method is a closure equivalent to:
  556    *
  557    *     (r1, .., rn, {p1: d1, ..., pk: dk}) {
  558    *       return this.invoke(name, [r1, .., rn], {#p1: p1,  .., #pk:
pk});
  559    *     }
  560    *
  561    * if m has required parameters r1, ..., rn, and named parameters p1,
..., pk
  562    * with defaults d1, ..., dk.
  563    *
  564    *     (r1, .., rn, [p1 = d1, …, pk = dk]) {
  565    *       return this.invoke(name, [r1, .., rn, p1, .., pk]);
  566    *     }
  567    *
  568    * if m has required parameters r1, ..., rn, and optional positional
  569    * parameters p1, ..., pk with defaults d1, ..., dk.
  570    */

[ahe, 2013/10/30 07:48:31]

I assume the other CL will land this change.

+   * method is a closure equivalent to:
+   *
+   *     (r1, .., rn, {p1: d1, ..., pk: dk}) {
+   *       return this.invoke(name, [r1, .., rn], {#p1: p1,  .., #pk: pk});
+   *     }
+   *
+   * if m has required parameters r1, ..., rn, and named parameters p1, ..., pk
+   * with defaults d1, ..., dk.
+   *
+   *     (r1, .., rn, [p1 = d1, …, pk = dk]) {
+   *       return this.invoke(name, [r1, .., rn, p1, .., pk]);
+   *     }
+   *
+   * if m has required parameters r1, ..., rn, and optional positional
+   * parameters p1, ..., pk with defaults d1, ..., dk.
+   */
+  Function operator [](Symbol name);
 }
 
 /**
@@ -532,6 +578,12 @@ abstract class InstanceMirror implements ObjectMirror {
 abstract class ClosureMirror implements InstanceMirror {
   /**
    * A mirror on the function associated with this closure.
+   *
+   * The function associated with an implicit closure of a function is that
+   * function.

[Johnni Winther, 2013/09/03 12:36:57]

'of a function is that function' -> 'of a method is the method itself'?

+   *
+   * The function associated with an instance of a class that has a [:call:]
+   * method is that [:call:] method.
    */
   MethodMirror get function;
 
@@ -600,6 +652,7 @@ abstract class ClosureMirror implements InstanceMirror {
    * The returned value is the result of invoking the method [reflect] on
    * *v*.
    */
+  // TODO(ahe): Drop from release 1.0?

[Michael Lippautz (Google), 2013/09/03 17:24:50]

Or "This operation is optional and may return [:null:]."? :)

(Also sync instead of async?)

[gbracha, 2013/10/03 22:11:25]

We have real demand for this. What are its prospects in dart2js?

[rmacnak, 2013/10/03 22:26:41]

This should be doable in dart2js because it compiles closure definitions like
mini classes wherein we can find the variables closed-over. (In NS2V8's
compilation strategy of using JS functions directly, implementing this would be
quite a challenge.) I'm not familiar with dart2js internals, but I would think
this is easier than generics.

   Future<InstanceMirror> findInContext(Symbol name);
 }
 
@@ -615,43 +668,35 @@ abstract class LibraryMirror implements DeclarationMirror, ObjectMirror {
   Uri get uri;
 
   /**
-   * An immutable map from from names to mirrors for all members in
+   * An immutable map from from names to mirrors for all members declared in
    * this library.
    *
-   * The members of a library are its top-level classes,
-   * functions, variables, getters, and setters.
-   */
-  Map<Symbol, Mirror> get members;
-
-  /**
-   * An immutable map from names to mirrors for all class
-   * declarations in this library.
-   */
-  Map<Symbol, ClassMirror> get classes;
-
-  /**
-   * An immutable map from names to mirrors for all function, getter,
-   * and setter declarations in this library.
-   */
-  Map<Symbol, MethodMirror> get functions;
-
-  /**
-   * An immutable map from names to mirrors for all getter
-   * declarations in this library.
-   */
-  Map<Symbol, MethodMirror> get getters;
-
-  /**
-   * An immutable map from names to mirrors for all setter
-   * declarations in this library.
+   * The members of a library are its top-level classes, functions, variables,

[gbracha, 2013/10/03 22:11:25]

also typedefs

+   * getters, and setters.
    */
-  Map<Symbol, MethodMirror> get setters;
+  Map<Symbol, DeclarationMirror> get declarations;

[gbracha, 2013/09/17 18:27:53]

See comments on declarations in ClassMirror below

 
   /**
-   * An immutable map from names to mirrors for all variable
-   * declarations in this library.
+   * Returns closure for invoking the regular method named [name].
+   *
+   * If [:declarations[name]:] is a regular method, the result of this method
+   * is a closure equivalent to:
+   *
+   *     (r1, .., rn, {p1: d1, ..., pk: dk}) {
+   *       return this.invoke(name, [r1, .., rn], {#p1: p1,  .., #pk: pk});
+   *     }
+   *
+   * if m has required parameters r1, ..., rn, and named parameters p1, ..., pk
+   * with defaults d1, ..., dk.
+   *
+   *     (r1, .., rn, [p1 = d1, …, pk = dk]) {
+   *       return this.invoke(name, [r1, .., rn, p1, .., pk]);
+   *     }
+   *
+   * if m has required parameters r1, ..., rn, and optional positional
+   * parameters p1, ..., pk with defaults d1, ..., dk.
    */
-  Map<Symbol, VariableMirror> get variables;
+  Function operator [](Symbol name);
 
   /**
    * Returns [:true:] if this mirror is equal to [other].
@@ -665,7 +710,7 @@ abstract class LibraryMirror implements DeclarationMirror, ObjectMirror {
    * are
    * the same library in the same isolate.
    */
-   bool operator == (other);
+   bool operator ==(other);
 }
 
 /**
@@ -705,7 +750,7 @@ abstract class ClassMirror implements TypeMirror, ObjectMirror {
   List<ClassMirror> get superinterfaces;
 
   /**
-   * An immutable map from from names to mirrors for all members of
+   * An immutable map from from names to mirrors for all declared members of
    * this type.
    *
    * The members of a type are its methods, fields, getters, and
@@ -714,38 +759,33 @@ abstract class ClassMirror implements TypeMirror, ObjectMirror {
    *
    * This does not include inherited members.
    */
-  Map<Symbol, Mirror> get members;
+  Map<Symbol, DeclarationMirror> get declarations;

[gbracha, 2013/09/17 18:27:53]

I am slightly confused. I thought the plan was to add a declarations method with
named arguments that allowed us to filter the desired results (inherited or not,
getters, setters etc.) and we would keep the existing members until after that
was added. Then we would drop members as part of a big breaking change.

Also same applies above.

[gbracha, 2013/09/26 19:50:35]

In our latest discussion, we assume that all declarations, including
constructors and type variables, are included.  This also requires a rule in the
spec that forbids name conflicts between type variables and members and/or
constructors, which is desired anyway.

[gbracha, 2013/10/03 22:11:25]

We now decided not to include type variables, because we want to keep the getter
for typeVariables. It needs to be defined in TypeMirror so TypeDefMirror
supports it (because typedefs can be generic).

Can we get the wording updated to reflect these discussions?

   /**
-   * An immutable map from names to mirrors for all method,
-   * declarations for this type.  This does not include getters and
-   * setters.
-   */
-  Map<Symbol, MethodMirror> get methods;
-
-  /**
-   * An immutable map from names to mirrors for all getter
-   * declarations for this type.
-   */
-  Map<Symbol, MethodMirror> get getters;
-
-  /**
-   * An immutable map from names to mirrors for all setter
-   * declarations for this type.
-   */
-  Map<Symbol, MethodMirror> get setters;
-
-  /**
-   * An immutable map from names to mirrors for all variable
-   * declarations for this type.
+   * Returns closure for invoking the regular method named [name].
+   *
+   * If [:declarations[name]:] is a regular static method, the result of this
+   * method is a closure equivalent to:
+   *
+   *     (r1, .., rn, {p1: d1, ..., pk: dk}) {
+   *       return this.invoke(name, [r1, .., rn], {#p1: p1,  .., #pk: pk});
+   *     }
+   *
+   * if m has required parameters r1, ..., rn, and named parameters p1, ..., pk
+   * with defaults d1, ..., dk.
+   *
+   *     (r1, .., rn, [p1 = d1, …, pk = dk]) {
+   *       return this.invoke(name, [r1, .., rn, p1, .., pk]);
+   *     }
+   *
+   * if m has required parameters r1, ..., rn, and optional positional
+   * parameters p1, ..., pk with defaults d1, ..., dk.
    */
-  Map<Symbol, VariableMirror> get variables;
+  Function operator [](Symbol name);

[gbracha, 2013/09/17 18:27:53]

nits in wording:

Returns *a* closure ...


if m has ... with defaults ... *Or*

... if m has .. optional ...

[gbracha, 2013/09/26 19:50:35]

Also, why is this restricted to static methods? What about getters and setters,
and instance members?

And what does it do if there is no member name [name]? Do we get null, or an
error of some sort?

[rmacnak, 2013/09/26 22:47:52]

From a subscript operator on InstanceMirror?  Hoist this to ObjectMirror?

-  /**
-   * An immutable map from names to mirrors for all constructor
-   * declarations for this type.
-   */
-  Map<Symbol, MethodMirror> get constructors;
+  /// Finds the instance member named [name] declared or inherited in the

[rmacnak, 2013/09/03 18:42:37]

Does this include getters and setters? If so, how do we distinguish getters from
regular methods?

[ahe, 2013/09/03 18:47:20]

Yes.
I probably don't understand your question, because the only answer I can think
of is:

ClassMirror c = ...;
var m = c.instanceLookup(#foo);
if (m is MethodMirror) {
  if (m.isGetter) print('foo is a getter');
  if (m.isRegularMethod) print('foo is regular method');
}

[rmacnak, 2013/09/03 19:44:26]

I was thinking something like this, but the override rules seem to forbid it.

import 'dart:mirrors';

class S {
  get foo => 'S-foo';
}
class C extends S {
  foo() => 'C-foo';
}
reflectClass(C)[#foo]

[ahe, 2013/09/03 19:52:36]

Yes. We have tried very hard to ensure a single namespace, unlike Java, for
example, which has separate namespaces for methods and fields.

+  /// reflected class.
+  DeclarationMirror instanceLookup(Symbol name);

[ahe, 2013/09/26 18:03:36]

We also need a way to get a list or map of all instance members (including
inherited members).

Codename: "api".

Perhaps "instanceMembers".

This method returns getters, setters, and methods, not fields (but their implied
getters and setters).

This means that we need to say that the original declaration of an implicit
getter, for example, is its field.

[rmacnak, 2013/09/26 22:47:52]

And perhaps add DeclarationMirror.isSynthetic

 
   /**
    * An immutable map from names to mirrors for all type variables for

[Michael Lippautz (Google), 2013/09/03 17:24:50]

comment out of sync

@@ -758,7 +798,7 @@ abstract class ClassMirror implements TypeMirror, ObjectMirror {
    *
    * This map preserves the order of declaration of the type variables.
    */
-  Map<Symbol, TypeVariableMirror> get typeVariables;
+  List<TypeVariableMirror> get typeVariables;

[Alan Knight, 2013/09/03 20:08:31]

Would Iterable be better than List?

[ahe, 2013/09/03 20:14:30]

I think length and operator[] are important properties, so I think List is the
right interface.

[Alan Knight, 2013/09/03 20:30:56]

Important for what in this context? Are we expecting the indices in this to
correspond to something else? I'm just thinking that it might be useful for the
implementation to be able to generate these lazily, or otherwise optimize in
ways that might be trickier if it's a List.

 
   /**
    * An immutable map from names to mirrors for all type arguments for

[Michael Lippautz (Google), 2013/09/03 17:24:50]

comment out of sync

@@ -773,7 +813,7 @@ abstract class ClassMirror implements TypeMirror, ObjectMirror {
    * it has no type arguments and this method returns an empty map.
    * This map preserves the order of declaration of the type variables.
    */
-  Map<Symbol, TypeMirror> get typeArguments;
+  List<TypeMirror> get typeArguments;
 
   /**
    * Is this the original declaration of this type?
@@ -862,7 +902,7 @@ abstract class ClassMirror implements TypeMirror, ObjectMirror {
    * with the result of calling [reflect](*r*).
    * If the invocation throws an exception *e* (that it does not catch)
    * then *k* is completed with a [MirrorError] wrapping *e*.
-*/
+   */
   Future<InstanceMirror> newInstanceAsync(Symbol constructorName,
                                           List positionalArguments,
                                           [Map<Symbol, dynamic> namedArguments]);
@@ -1123,88 +1163,6 @@ abstract class SourceLocation {
 }
 
 /**
- * When an error occurs during the mirrored execution of code, a
- * [MirroredError] is thrown.
- *
- * In general, there are three main classes of failure that can happen
- * during mirrored execution of code in some isolate:
- *
- * - An exception is thrown but not caught.  This is caught by the
- *   mirrors framework and a [MirroredUncaughtExceptionError] is
- *   created and thrown.
- *
- * - A compile-time error occurs, such as a syntax error.  This is
- *   suppressed by the mirrors framework and a
- *   [MirroredCompilationError] is created and thrown.
- *
- * - A truly fatal error occurs, causing the isolate to be exited.  If
- *   the reflector and reflectee share the same isolate, then they
- *   will both suffer.  If the reflector and reflectee are in distinct
- *   isolates, then we hope to provide some information about the
- *   isolate death, but this has yet to be implemented.
- *
- * TODO(turnidge): Specify the behavior for remote fatal errors.
- */
-abstract class MirroredError implements Exception {
-}
-
-/**
- * When an uncaught exception occurs during the mirrored execution
- * of code, a [MirroredUncaughtExceptionError] is thrown.
- *
- * This exception contains a mirror on the original exception object.
- * It also contains an object which can be used to recover the
- * stacktrace.
- */
-class MirroredUncaughtExceptionError extends MirroredError {

[rmacnak, 2013/09/03 18:42:37]

We will need this for cross-isolate reflection, but I'm okay with knocking it
out for now.

-  MirroredUncaughtExceptionError(this.exception_mirror,
-                                 this.exception_string,
-                                 this.stacktrace) {}
-
-  /** A mirror on the exception object. */
-  final InstanceMirror exception_mirror;
-
-  /** The result of toString() for the exception object. */
-  final String exception_string;
-
-  /** A stacktrace object for the uncaught exception. */
-  final Object stacktrace;
-
-  String toString() {
-    return
-        "Uncaught exception during mirrored execution: <${exception_string}>";
-  }
-}
-
-/**
- * When a compile-time error occurs during the mirrored execution
- * of code, a [MirroredCompilationError] is thrown.
- *
- * This exception includes the compile-time error message that would
- * have been displayed to the user, if the function had not been
- * invoked via mirror.
- */
-class MirroredCompilationError extends MirroredError {

[Michael Lippautz (Google), 2013/09/03 17:24:50]

Note: I suspect that removing this classes will break the vm mirrors. So this
needs to be in sync with a change there?

[rmacnak, 2013/09/03 18:42:37]

We should keep this. Catching compilation errors is interesting for interactive
development. Dart2js need not do anything because either a method will be
missing from tree-shaking and a normal NoSuchMethodError results, or all the
relevant code will have been eagerly compiled.

[ahe, 2013/09/03 18:47:20]

Yes, I think I would start by moving these classes into the VM patch file.

-  MirroredCompilationError(this.message) {}
-
-  final String message;
-
-  String toString() {
-    return "Compile-time error during mirrored execution: <$message>";
-  }
-}
-
-/**
- * A [MirrorException] is used to indicate errors within the mirrors
- * framework.
- */
-class MirrorException implements Exception {
-  const MirrorException(String this._message);
-  String toString() => "MirrorException: '$_message'";
-  final String _message;
-}
-
-/**
  * Class used for encoding comments as metadata annotations.
  */
 class Comment {