Improve documentation for MirrorsUsed.

sdk/lib/mirrors/mirrors.dart

Index: sdk/lib/mirrors/mirrors.dart
diff --git a/sdk/lib/mirrors/mirrors.dart b/sdk/lib/mirrors/mirrors.dart
index 715342430186bd2824323216a6f574a61de82c7f..89472cf69431a53e79ecb9c8e9b82d4d48a633e8 100644
--- a/sdk/lib/mirrors/mirrors.dart
+++ b/sdk/lib/mirrors/mirrors.dart
@@ -1196,7 +1196,7 @@ class Comment {
  *
  * Example usage:
  *
- *     @MirrorsUsed(symbols: 'foo', override: '*')
+ *     @MirrorsUsed(symbols: 'foo')
  *     import 'dart:mirrors';
  *
  *     class Foo {
@@ -1209,12 +1209,26 @@ class Comment {
  *       new Foo().foo(); // Prints "foo".
  *       new Foo().bar(); // Might print an arbitrary (mangled) name, "bar".
  *     }
+ *
+ * For a detailed description of the parameters to the [MirrorsUsed] constructor
+ * see the comments for the respective fields [symbols]. [targets] and 

[Kathy Walrath, 2015/05/05 19:24:57]

delete "the respective fields "

. -> ,

[herhut, 2015/05/06 14:24:16]

Done.

+ * [metaTargets].

[Kathy Walrath, 2015/05/05 19:24:57]

what about [override]?

[herhut, 2015/05/06 14:24:17]

Good catch, thanks!

+ *
+ * An import of `dart:mirrors` may have multiple [MirrorsUsed] annotations. This
+ * is particularly helpful to specify overrides for specific libraries. For 
+ * example

[Kathy Walrath, 2015/05/05 19:24:57]

example -> example:

[herhut, 2015/05/06 14:24:15]

Done.

+ *
+ *     @MirrorsUsed(targets: 'foo.Bar', override: 'foo')
+ *     @MirrorsUsed(targets: 'Bar')
+ *     import 'dart:mirrors';
+ *
+ * will ensure that the target `Bar` from the current library and from library
+ * `foo` is available for reflection. See also [override].
  */
-// TODO(ahe): Remove ", override: '*'" when it isn't necessary anymore.
 class MirrorsUsed {
   // Note: the fields of this class are untyped.  This is because the most
-  // convenient way to specify to specify symbols today is using a single
-  // string. In some cases, a const list of classes might be convenient. Some
+  // convenient way to specify symbols today is using a single string. In 
+  // some cases, a const list of classes might be convenient. Some
   // might prefer to use a const list of symbols.
 
   /**
@@ -1228,19 +1242,29 @@ class MirrorsUsed {
    *
    * The following text is non-normative:
    *
+   * Dart2js currently supports the below formats to specify symbols:

[Kathy Walrath, 2015/05/05 19:24:58]

below -> following

[herhut, 2015/05/06 14:24:16]

Done.

+   *
+   * * A [List] of [String] constants representing symbol names, e.g., 

[Kathy Walrath, 2015/05/05 19:24:57]

A -> A constant
? (it must be a const List, right?)

[herhut, 2015/05/06 14:24:16]

Done.

+   *   `const ['foo', 'bar']`.
+   * * A single [String] constant whose value is a comma separated list of

[Kathy Walrath, 2015/05/05 19:24:58]

comma separated -> comma-separated

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

comma + whitespace separated apparently?
Are multiple whitespaces allowed? Are whitespaces allowed before the comma?
Before first value and/or after last? Are empty entries allowed ("x,,y")?

[herhut, 2015/05/06 14:24:16]

Done.

[herhut, 2015/05/06 14:24:16]

This is meant to be a documentation, not a formal specification. I don't think
adding such detail is overly helpful, especially as the example shows that
whitespace is OK. 

+   *   symbol names, e.g., `"foo, bar"`.
+   *
    * Specifying this option turns off the following warnings emitted by

[Kathy Walrath, 2015/05/05 19:24:57]

this option -> the `symbols` field

[herhut, 2015/05/06 14:24:17]

Done.

    * dart2js:
    *
    * * Using "MirrorSystem.getName" may result in larger output.
    * * Using "new #{name}" may result in larger output.
    *
-   * Use symbols = "*" to turn off the warnings mentioned above.
-   *
    * For example, if using [noSuchMethod] to interact with a database, extract

[Kathy Walrath, 2015/05/05 19:24:57]

using -> you're using

[herhut, 2015/05/06 14:24:16]

Done.

    * all the possible column names and include them in this list.  Similarly,
    * if using [noSuchMethod] to interact with another language (JavaScript, for

[Kathy Walrath, 2015/05/05 19:24:58]

using -> you're using

[herhut, 2015/05/06 14:24:17]

Done.

    * example) extract all the identifiers from API used and include them in

[Kathy Walrath, 2015/05/05 19:24:57]

API -> the API you use

[herhut, 2015/05/06 14:24:15]

Done.

    * this list.
+   *
+   * Note that specifying a symbol only ensures that the symbol will be
+   * available under that name at runtime. It does not mark targets with
+   * that name as available for reflection. See [targets] and [metaTargets]
+   * for that purpose.
    */
   final symbols;
 
@@ -1252,17 +1276,78 @@ class MirrorsUsed {
    *
    * The following text is non-normative:
    *
+   * Dart2js currently supports the below formats to specify targets:

[Kathy Walrath, 2015/05/05 19:24:58]

Same comments as above:

below -> following
[List] -> constant [List]  (?)
comma separated -> comma-separated

[herhut, 2015/05/06 14:24:16]

Done.

+   *
+   * * A [List] containing [String] constants representing (qualified) 
+   *   names of targets and Dart types.
+   * * A single [String] constant whose value is a comma separated list of
+   *   (qualified) names.
+   * * A single Dart type.
+   *
+   * A (qualified) name is resolved to a target as follows:
+   *
+   * 1. If the qualified name matches a library name, the matching library is

[Lasse Reichstein Nielsen, 2015/05/06 07:41:20]

"matches" meaning?
I guess equals-after-canonicalization (removing whitespace). Is whitespaces
allowed in qualified names? Around commas?

[herhut, 2015/05/06 14:24:17]

...not a spec.

+   *    the target.
+   * 2. Else, find the longest prefix of the name such that the prefix ends
+   *    with a `.` and is a library name. 

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

ends with -> ends just before
(Otherwise the prefix would end with '.' and can't be a valid library name).

[herhut, 2015/05/06 14:24:16]

Done.

+   * 3. Use that library as current scope. If no matching prefix was found, use
+   *    the current library, i.e., the library where the [MirrorsUsed] 
+   *    annotation was placed.
+   * 4. Split the remaining suffix into a list of [String] using `.` as a 

[Lasse Reichstein Nielsen, 2015/05/06 07:41:20]

Split the remaining suffix (the entire name in case item 3 above was used) into
...

(non-mathematicians can have a problem with accepting suffices that are not
proper :)

[herhut, 2015/05/06 14:24:16]

Done.

+   *    separator.
+   * 5. Select all targets in the current scope whose name matches a [String] 

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

"matches" means? Equals for non-setters or equals-without-the-= for setters?

[herhut, 2015/05/06 14:24:16]

without the = for setters. Again this is not a spec.

+   *    from the list.

[Lasse Reichstein Nielsen, 2015/05/06 07:41:20]

Step 5 looks wrong. Not saying it's not what's happening, but it sure *looks*
wrong!

As I read it "foo.bar.baz.qux" can match a library named "foo.bar", and then
match either "baz" or "qux" declared in that library - that is, something with
fully qualified name "foo.bar.qux"? 
But it won't match the qux in "class baz { static var qux; }" in the library?
(And methods can't be targeted directly).

That is, the '.'s in the suffix are used as separators between equivalent names,
not as a descending path. That's ... unconventional.

So if this is really true (which I hear that it is), maybe add a comment saying
"yes, this IS weird, it's not you misunderstanding it".

[herhut, 2015/05/06 14:24:16]

This is how it is and I added an example to highlight this oddity.

+   *

[Kathy Walrath, 2015/05/05 19:24:58]

A brief example would be nice here.

[herhut, 2015/05/06 14:24:16]

Added.

[Kathy Walrath, 2015/05/06 20:54:25]

That helps. Thanks!

+   * Note that everything within a target also is available for reflection.
+   * So, if a library is specified as target, all classes in that library
+   * become targets for reflection. Likewise, if a class is a target, all
+   * its methods and fields become targets for reflection.
+   *
    * For now, there is no formal description of what a reflective target is.
-   * Informally, it is a list of things that are expected to have fully
-   * functional mirrors.
+   * Informally, a target is a library, a class, a method or a field.
+   *
    */
   final targets;
 
   /**
    * A list of classes that when used as metadata indicates a reflective
-   * target.
+   * target. See also [targets].
+   *
+   * The following text is non-normative:

[Lasse Reichstein Nielsen, 2015/05/06 07:41:20]

What does that mean? Where it the normative definition?

[herhut, 2015/05/06 14:24:17]

There is none.

+   *
+   * The format for specifying the list of classes is the same as used for
+   * specifying [targets]. However, as a library cannot be used as a metadata
+   * annotation in Dart, adding a library to the list of [metaTargets] has no
+   * effect. In particular, adding a library to [metaTargets] does not make
+   * the library's classes a valid metadata annotation to enable reflection.

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

a valid metadata annotation -> valid metadata annotations

[herhut, 2015/05/06 14:24:17]

Done.

+   *
+   * If a class specified in [metaTargets] is used as metadata annotation

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

If an instance of a class ...

[herhut, 2015/05/06 14:24:17]

Done.

+   * on a class, field or method, that class, field or method is added to 

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

But not library?

[herhut, 2015/05/06 14:24:16]

Good catch, thanks!

+   * the set of targets for reflection. 
+   *
+   * Example usage:
    *
-   * See [targets].
+   *     library example;
+   *     @MirrorsUsed(metaTargets: "example.Reflectable")
+   *     import "dart:mirrors";
+   *
+   *     class Reflectable {
+   *       const Reflectable();
+   *     }
+   *
+   *     class Foo {
+   *       @Reflectable()
+   *       reflectableMethod() { ... }
+   *
+   *       nonReflectableMethod() { ... }
+   *     }
+   *
+   * In the above example. `reflectableMethod` is marked as reflectable by
+   * using the `Reflectable` class, which in turn is specified in the 
+   * [metaTargets] annotation.
+   *
+   * The method `nonReflectableMethod` lacks a metadata annotation and thus 
+   * will not be reflectable at runtime.
    */
   final metaTargets;
 
@@ -1271,10 +1356,41 @@ class MirrorsUsed {
    *
    * When used as metadata on an import of "dart:mirrors", this metadata does
    * not apply to the library in which the annotation is used, but instead
-   * applies to the other libraries (all libraries if "*" is used).
+   * applies to the other libraries (all libraries if "*" is used). In

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

So it *does* apply to itself if "*" is used?
And it won't remove itself in that case?

[herhut, 2015/05/06 14:24:17]

I had to rewrite the entire section after looking at the code again. It is
actually just adding mirrorsUsed annotations. There is no overriding in any
form.

+   * particular, if the other library has a metadata annotation of its own,
+   * it will be shadowed by the override.

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

The word "shadow" suggests an ordering where the original is still there, but
the new one is used instead.

What if two libraries both override a third - will the third library have zero,
one or two annotations that apply? If one, which one?

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

What if you specify yourself without using "*"?

library foo;
@MirrorsUsed(....)
@MirrorsUsed(..., "override":"foo,bar");
import "dart:mirrors";

Will this remove the MirrorsUsed annotations from "foo"?

[herhut, 2015/05/06 14:24:16]

Removed.

[herhut, 2015/05/06 14:24:17]

Removed.

+   *
+   * If two libraries have mutual overrides, they will shadow each other and
+   * an empty annotation will be assumed.

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

That's not what "shadow each other" means (if it means anything).
Drop the "shadow" part of the explanation.
"an empty annotation" -> both will be considered to have no `MirrorsUsed`
annotation.

[herhut, 2015/05/06 14:24:16]

Removed.

+   *
+   * The following text is non-normative:

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

Where is the normative text then?

[herhut, 2015/05/06 14:24:16]

Does not exist.

+   *
+   * While the annotation will apply to the given target libraries, the
+   * [symbols], [targets] and [metaTargets] are still evaluated in the 
+   * scope of the annotation. Thus, to select a target from library `foo`,
+   * a qualified name has to be used or, if the target is visible in the
+   * current scope, its type may be referenced.
+   * 
+   * For example

[Kathy Walrath, 2015/05/05 19:24:57]

-> For example, the following code marks all targets in... library.

[herhut, 2015/05/06 14:24:16]

Done.

+   *
+   *     @MirrorsUsed(metaTargets: "foo.Reflectable", override: "foo")
+   *
+   * will mark all targets in the library `foo` as reflectable that have
+   * a metadata annotation using the `Reflectable` class from the same library.
+   * However,

[Kathy Walrath, 2015/05/05 19:24:58]

Delete previous two lines (moved, actually). Change this one to:

However, the following code would require....

[herhut, 2015/05/06 14:24:17]

Done.

+   *
+   *     @MirrorsUsed(metaTargets: "Reflectable", override: "foo")
+   *
+   * would require the use of the `Reflectable` class from the current

[Kathy Walrath, 2015/05/05 19:24:58]

Delete these 2 lines (move up above, actually).

[herhut, 2015/05/06 14:24:16]

Done.

+   * library, instead.
    */

[Lasse Reichstein Nielsen, 2015/05/06 07:41:21]

So, if I get this right:
Override specifies a set of libraries, or "*" for all libraries.
To apply the override:
- First find all libraries that are affected by an override.
- Then remove the MirrorsUsed annotations on dart:mirrors imports of those
libraries (except for the declaring library when using "*"?).
- Then, for all remaining MirrorsUsed annotations with an override:
- Apply the override MirrorsUsed annotation to dart:mirror imports on the
libraries they specify.
- From here on, ignore "override".

There is still some vagueness about whether an override can remove itself.

[herhut, 2015/05/06 14:24:16]

Removed.

   final override;
 
+  /**
+   * See the documentation for [MirrorsUsed.symbols], [MirrorsUsed.targets], 
+   * [MirrorsUsed.metaTargets] and [MirrorsUsed.override] for documentation 
+   * of the parameters.
+   */
   const MirrorsUsed(
       {this.symbols, this.targets, this.metaTargets, this.override});
 }