Better ability to have hand-written custom rules and various cleanups.

pkg/serialization/lib/src/serialization_rule.dart

Index: pkg/serialization/lib/src/serialization_rule.dart
diff --git a/pkg/serialization/lib/src/serialization_rule.dart b/pkg/serialization/lib/src/serialization_rule.dart
index a4602373c1f33f40ff2264d3bfe3a6e143f64d6c..bc772f48c09e0ebdb672524b62368976ff8b280c 100644
--- a/pkg/serialization/lib/src/serialization_rule.dart
+++ b/pkg/serialization/lib/src/serialization_rule.dart
@@ -33,8 +33,11 @@ abstract class SerializationRule {
     _number = x;
   }
 
-  /** Return true if this rule applies to this object, false otherwise. */
-  bool appliesTo(object);
+  /**
+   * Return true if this rule applies to this object, in the context
+   * where we're writing it, false otherwise.
+   */
+  bool appliesTo(object, Writer writer);
 
   /**
    * This extracts the state from the object, calling [f] for each value
@@ -42,7 +45,7 @@ abstract class SerializationRule {
    * state at the end. The state that results will still have direct
    * pointers to objects, rather than references.
    */
-  Object extractState(object, void f(value));
+  extractState(object, void f(value));
 
   /**
    * Given the variables representing the state of an object, flatten it
@@ -98,11 +101,11 @@ abstract class SerializationRule {
   inflateNonEssential(state, object, Reader reader);
 
   /**
-   * If we have an object [o] as part of our state, should we represent that
+   * If we have [object] as part of our state, should we represent that
    * directly, or should we make a reference for it. By default we use a
    * reference for everything.
    */
-  bool shouldUseReferenceFor(Object o, Writer w) => true;
+  bool shouldUseReferenceFor(object, Writer w) => true;
 
   /**
    * This writes the data from our internal representation into a List.
@@ -121,8 +124,7 @@ abstract class SerializationRule {
     var intermediate = new List();
     var totalLength = 0;
     for (var eachList in ruleData) {
-      // TODO(alanknight): Abstract this out better, this really won't scale.
-      if (this is ListRule)
+      if (writeLengthInFlatFormat())

[Jennifer Messerly, 2012/12/12 20:38:28]

fwiw, I think our style is usually either to have this on one line:

if (writeLengthInFlatFormat()) intermediate.add(eachList.length);

or use curlies:

if (writeLengthInFlatFormat()) {
  intermediate.add(eachList.length);
}

(yeah ... it's 2012 and we're still worried about dangling else and indent+close
brace mismatches :\ )

[Alan Knight, 2012/12/12 21:19:33]

Done.

         intermediate.add(eachList.length);
       for (var eachRef in eachList) {
         if (eachRef == null) {
@@ -136,10 +138,52 @@ abstract class SerializationRule {
   }
 
   /**
+   * Return true if this rule writes a length value before each entry in
+   * the flat format. Return false if the results are fixed length.
+   */
+  // TODO(alanknight): This should probably go away with more general formats.
+  bool writeLengthInFlatFormat() => false;

[Jennifer Messerly, 2012/12/12 20:38:28]

make a getter?

[Alan Knight, 2012/12/12 21:19:33]

Done.

+
+  /**
    * The inverse of dumpStateInto, this reads the rule's state from an
    * iterator in a flat format.
    */
-  pullStateFrom(Iterator stream);
+  pullStateFrom(Iterator stream) {
+    var numberOfEntries = stream.next();
+    var ruleData = new List();
+    for (var i = 0; i < numberOfEntries; i++) {
+      var subLength = dataLengthIn(stream);
+      var subList = new List();

[Jennifer Messerly, 2012/12/12 20:38:28]

personally I prefer [] form

[Alan Knight, 2012/12/12 21:19:33]

Done.

+      ruleData.add(subList);
+      for (var j = 0; j < subLength; j++) {
+        var a = stream.next();
+        var b = stream.next();
+        if (!(a is int)) {
+          // This wasn't a reference, just use the first object as a literal.
+          // particularly used for the case of null.
+          subList.add(a);
+        } else {
+          subList.add(new Reference(this, a, b));
+        }
+      }
+    }
+    return ruleData;
+  }
+
+  /**
+   * Return the length of the list of data we expect to see on a particular
+   * iterator in a flat format. This may have been encoded in the stream if we
+   * are variable length, or it may be constant. Note that this is expressed in
+   *
+   */
+  dataLengthIn(Iterator stream) =>
+      writeLengthInFlatFormat() ? stream.next() : dataLength();
+
+  /**
+   * If the data is fixed length, return it here. Unused in the non-flat
+   * format, or if the data is variable length.
+   */
+  int dataLength() => 0;

[Jennifer Messerly, 2012/12/12 20:38:28]

getter?

[Alan Knight, 2012/12/12 21:19:33]

Done. Also renamed locals that were shadowing this. Shouldn't have mattered, but
potentially confusing.

 }
 
 /**
@@ -148,7 +192,7 @@ abstract class SerializationRule {
  */
 class ListRule extends SerializationRule {
 
-  appliesTo(object) => object is List;
+  appliesTo(object, Writer w) => object is List;
 
   state(List list) => new List.from(list);
 
@@ -206,6 +250,14 @@ class ListRule extends SerializationRule {
     }
     return ruleData;
   }
+
+  /**
+   * Return true because we need to write the length of each list in the flat
+   * format. */
+  bool writeLengthInFlatFormat() => true;
+
+  /** Return the length of the next list when reading the flat format. */
+  int dataLengthIn(Iterator stream) => stream.next();
 }
 
 /**
@@ -234,7 +286,7 @@ class ListRuleEssential extends ListRule {
  * num, String, and bool.
  */
 class PrimitiveRule extends SerializationRule {
-    appliesTo(object) {
+  appliesTo(object, Writer w) {
     return isPrimitive(object);
   }
   extractState(object, Function f) => object;
@@ -242,11 +294,12 @@ class PrimitiveRule extends SerializationRule {
   inflateEssential(state, Reader r) => state;
   inflateNonEssential(object, _, Reader r) {}
 
-  /** Indicate whether we should save pointers to this object as references
+  /**
+   * Indicate whether we should save pointers to this object as references
    * or store the object directly. For primitives this depends on the format,
    * so we delegate to the writer.
    */
-  bool shouldUseReferenceFor(Object o, Writer w) =>
+  bool shouldUseReferenceFor(object, Writer w) =>
       w.shouldUseReferencesForPrimitives;
 
   /**
@@ -276,24 +329,24 @@ class PrimitiveRule extends SerializationRule {
 }
 
 /** Helper function for PrimitiveRule to tell which objects it applies to. */
-bool isPrimitive(Object object) {
+bool isPrimitive(object) {
   return object is num || object is String || object is bool;
 }
 
-/** Typedef for the object construction closure used in ClosureToMapRule. */
-typedef Object ConstructType(Map m);
+/** Typedef for the object construction closure used in ClosureRule. */
+typedef ConstructType(Map m);
 
 /** Typedef for the state-getting closure used in ClosureToMapRule. */
-typedef Map<String, Object> GetStateType(Object o);
+typedef Map<String, dynamic> GetStateType(object);
 
 /** Typedef for the state-setting closure used in ClosureToMapRule. */
-typedef void NonEssentialStateType(Object o, Map m);
+typedef void NonEssentialStateType(object, Map m);
 
 /**
  * This is a rule where the extraction and creation are hard-coded as
  * closures. The result is expected to be a map indexed by field name.
  */
-class ClosureToMapRule extends SerializationRule {
+class ClosureRule extends CustomRule {
 
   /** The runtimeType of objects that this rule applies to. Used in appliesTo.*/
   final Type type;
@@ -302,7 +355,7 @@ class ClosureToMapRule extends SerializationRule {
   ConstructType construct;
 
   /** The function for returning an object's state as a Map. */
-  GetStateType getState;
+  GetStateType getStateFunction;
 
   /** The function for setting an object's state from a Map. */
   NonEssentialStateType setNonEssentialState;
@@ -312,56 +365,218 @@ class ClosureToMapRule extends SerializationRule {
    * state by calling [getState], creates a new object by calling [construct]
    * and sets the new object's state by calling [setNonEssentialState].
    */
-  ClosureToMapRule(this.type, this.getState, this.construct,
+  ClosureRule(this.type, this.getStateFunction, this.construct,
       this.setNonEssentialState);
 
+  bool appliesTo(object, Writer w) => object.runtimeType == type;
+
+  getState(object) => getStateFunction(object);
+
+  create(state) => construct(state);
+
+  setState(object, state) {
+    if (setNonEssentialState == null) return;
+    setNonEssentialState(object, state);
+  }
+}
+
+/**
+ * This rule handles things we can't pass directly, but only by reference.
+ * If objects are listed in the namedObjects in the writer or serialization,
+ * it will save the name rather than saving the state.
+ */
+class NamedObjectRule extends SerializationRule {
   /**
-   * If we deserialize a ClosureToMapRule we can't actually use it, because
-   * we don't have the closures, so generate a stub that just returns the
-   * raw state object.
+   * Return true if this rule applies to the object. Checked by looking up
+   * in the namedObjects collection.
    */
-  ClosureToMapRule.stub(this.type) {
-    getState = (x) { throw new SerializationException(
-        'Closures cannot be serialized'); };
-    construct = (state) => state;
-    setNonEssentialState = (object, state) {};
+  bool appliesTo(object, Writer writer) {
+    return writer.hasNameFor(object);
   }
 
-  bool appliesTo(object) => object.runtimeType == type;
+  /** Extract the state of the named objects as just the object itself. */
+  extractState(object, Function f) => [object];
 
-  extractState(object, Function f) {
-    Map state = getState(object);
-    values(state).forEach(f);
-    return state;
+  /** When we flatten the state we save it as the name. */
+  // TODO(alanknight): This seems questionable. In a truly flat format we may
+  // want to have extracted the name as a string first and flatten it into a
+  // reference to that. But that requires adding the Writer as a parameter to
+  // extractState, and I'm reluctant to add yet another parameter until
+  // proven necessary.
+  void flatten(state, Writer writer) {
+    state[0] = nameFor(state.first, writer);
   }
 
-  // TODO(alanknight): We're inflating twice here. How to avoid doing
-  // that without giving the user even more stuff to specify.
-  // Worse than that, by inflating everything in advance, we are are
-  // forcing all the state to be essential.
-  Object inflateEssential(Map<String, Object> state, Reader r) {
-    var inflated = values(state).map((x) => r.inflateReference(x));
-    return construct(inflated);
-  }
+  /** Look up the named object and return it. */
+  inflateEssential(state, Reader r) => r.objectNamed(state.first);
 
-  void inflateNonEssential(state, object, Reader r) {
-    if (setNonEssentialState == null) return;
-    var inflated = values(state).map((x) => r.inflateReference(x));
-    setNonEssentialState(inflated, object);
-  }
+  /** Set any non-essential state on the object. For this rule, a no-op. */
+  inflateNonEssential(state, object, Reader r) {}
+
+  /** Return the name for this object in the Writer. */
+  nameFor(object, Writer writer) => writer.nameFor(object);
 }
 
 /**
- * This rule handles things we can't pass directly, but only by reference.
- * It extracts an identifier we can use to pass them.
+ * This rule handles the special case of Mirrors, restricted to those that
+ * have a simpleName. It knows that it applies to any such mirror and
+ * automatically uses its simpleName as the key into the namedObjects.
+ * When reading, the user is still responsible for adding the appropriate
+ * mirrors to namedObject.
  */
-class ClassMirrorRule extends SerializationRule {
-  // TODO(alanknight): This probably generalizes to any named object.
-  bool appliesTo(object) {
-    return object is ClassMirror;
+class MirrorRule extends NamedObjectRule {
+  bool appliesTo(object, Writer writer) => object is DeclarationMirror;
+  nameFor(DeclarationMirror object, Writer writer) => object.simpleName;
+}
+
+/**
+ * This provides an abstract superclass for writing your own rules specific to
+ * a class. It makes some assumptions about behaviour, and so can have a
+ * simpler set of methods that need to be implemented in order to subclass it.
+ *
+ */
+abstract class CustomRule extends SerializationRule {
+  // TODO(alanknight): It would be nice if we could provide an implementation
+  // of appliesTo() here. If we add a type parameter to these classes
+  // we can "is" test against it, but we need to be able to rule out subclasses.
+  // => instance.runtimeType == T
+  // should work.
+  /**
+   * Return true if this rule applies to this object, in the context
+   * where we're writing it, false otherwise.
+   */
+  bool appliesTo(instance, Writer w);
+
+  /**
+   * Subclasses should implement this to return a list of the important fields
+   * in the object. The order of the fields doesn't matter, except that the
+   * create and setState methods need to know how to use it.
+   */
+  List getState(instance);
+
+  /**
+   * Given a [List] of the object's [state], re-create the object. This should
+   * do the minimum needed to create the object, just calling the constructor.
+   * Setting the remaining state of the object should be done in the [setState]
+   * method, which will be called only once all the objects are created, so
+   * it won't cause problems with cycles.
+   */
+  create(List state);
+
+  /**
+   * Set any state in [object] which wasn't set in the constructor. Between
+   * this method and [create] all of the information in [state] should be set
+   * in the new object.
+   */
+  void setState(object, List state);
+
+  extractState(instance, Function f) {
+    var state = getState(instance);
+    for (var each in values(state)) {
+      f(each);
+    }
+    return state;
   }
-  extractState(object, Function f) => f(object.simpleName);
-  void flatten(object, Writer writer) {}
-  inflateEssential(state, Reader r) => r.externalObjectNamed(state);
-  inflateNonEssential(state, object, Reader r) {}
+
+  inflateEssential(state, Reader r) => create(_lazy(state, r));
+
+  void inflateNonEssential(state, object, Reader r) =>
+      setState(object, _lazy(state, r));
+
+  // We don't want to have to make the end user tell us how long the list is
+  // separately, so write it out for each object, even though they're all
+  // expected to be the same length.
+  writeLengthInFlatFormat() => true;
+}
+
+/** Create a lazy list that will inflate its items on demand in [r]. */
+_lazy(l, Reader r)
+    => (l is List) ? new _LazyList(l, r) : new _LazyMap(l, r);

[Jennifer Messerly, 2012/12/12 20:38:28]

should this check for primitive types too?

[Alan Knight, 2012/12/12 21:19:33]

It really expects this to be either a List or a Map, so checked for both cases
and throw an exception otherwise. Thought about whether it would be reasonable
to let the user return a completely arbitrary object, e.g. a primitive or a
Reference. But decided it was likely making the API more confusing without
adding much power.

+
+/**
+ * This provides an implementation of Map that wraps a list which may
+ * contain references to (potentially) non-inflated objects. If these
+ * are accessed it will inflate them. This allows us to pass something that
+ * looks like it's just a list of objects to a [CustomRule] without needing
+ * to inflate all the references in advance.
+ */
+class _LazyMap implements Map {
+  _LazyMap(this.raw, this.reader);
+
+  Map raw;

[Jennifer Messerly, 2012/12/12 20:38:28]

make these private?

[Alan Knight, 2012/12/12 21:19:33]

Done.

+  Reader reader;
+
+  // This is the only operation that really matters.
+  operator [](x) => reader.inflateReference(raw[x]);
+
+  int get length => raw.length;
+  bool get isEmpty => raw.isEmpty;
+  List get keys => raw.keys;
+  bool containsKey(x) => raw.containsKey(x);
+
+  // These operations will work, but may be expensive, and are probably
+  // best avoided.
+  get _inflated => keysAndValues(raw).map(reader.inflateReference);
+  bool containsValue(x) => _inflated.containsValue(x);
+  List get values => _inflated.values;
+  void forEach(f) => _inflated.forEach(f);
+
+  // These operations are all invalid
+  _throw() => throw new UnsupportedError("Not modifiable");
+  operator []=(x, y) => _throw();
+  putIfAbsent(x, y) => _throw();
+  remove(x) => _throw();
+  clear() => _throw();
+}
+
+/**
+ * This provides an implementation of List that wraps a list which may
+ * contain references to (potentially) non-inflated objects. If these
+ * are accessed it will inflate them. This allows us to pass something that
+ * looks like it's just a list of objects to a [CustomRule] without needing
+ * to inflate all the references in advance.
+ */
+class _LazyList implements List {
+  _LazyList(this.raw, this.reader);
+
+  List raw;
+  Reader reader;
+
+  // This is the only operation that really matters.
+  operator [](x) => reader.inflateReference(raw[x]);
+
+  int get length => raw.length;
+  bool get isEmpty => raw.isEmpty;
+  get first => reader.inflateReference(raw.first);
+  get last => reader.inflateReference(raw.last);
+
+  // These operations will work, but may be expensive, and are probably
+  // best avoided.
+  get _inflated => raw.map(reader.inflateReference);
+  map(f) => _inflated.map(f);
+  filter(f) => _inflated.filter(f);
+  bool contains(element) => _inflated.filter(element);
+  forEach(f) => _inflated.forEach(f);
+  reduce(x, f) => _inflated.reduce(x, f);
+  every(f) => _inflated(f);
+  some(f) => _inflated(f);
+  iterator() => _inflated.iterator();
+  indexOf(x, [pos = 0]) => _inflated.indexOf(x);
+  lastIndexOf(x, [pos]) => _inflated.lastIndexOf(x);
+
+  // These operations are all invalid
+  _throw() => throw new UnsupportedError("Not modifiable");
+  operator []=(x, y) => _throw();
+  add(x) => _throw();
+  addLast(x) => _throw();
+  addAll(x) => _throw();
+  sort([f]) => _throw();
+  clear() => _throw();
+  removeAt(x) => _throw();
+  removeLast() => _throw();
+  getRange(x, y) => _throw();
+  setRange(x, y, z, [a]) => _throw();
+  removeRange(x, y) => _throw();
+  insertRange(x, y, [z]) => _throw();
+  void set length(x) => _throw();
 }