Change path-observer to lookup properties aggressively and report errors

pkg/observe/lib/src/path_observer.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library observe.src.path_observer;
 
 import 'dart:async';
 import 'dart:collection';
 import 'dart:math' show min;
 
@@ skipping 63 matching lines @@
   bool _check({bool skipChanges: false}) {
     var oldValue = _value;
     _value = _path.getValueFrom(_object);
     if (skipChanges || _value == oldValue) return false;
 
     _report(_value, oldValue);
     return true;
   }
 }
 
 /// A dot-delimieted property path such as "foo.bar" or "foo.10.bar".

[Jennifer Messerly, 2014/03/25 22:32:44]

this was an issue before but: do you mind adding a newline after this intro
sentence?

[Siggi Cherem (dart-lang), 2014/03/26 00:32:07]

Done.

 /// The path specifies how to get a particular value from an object graph, where
-/// the graph can include arrays.
+/// the graph can include arrays and maps. Each segment of the path describes
+/// how to take a single step in the object graph. Properties like 'foo' or
+/// 'bar' are read as properties on objects, or as keys if the object is a Map
+/// or an [StringIndexer], while integer values are read as indexes in an array.

[Jennifer Messerly, 2014/03/25 22:32:44]

should be "a [StringIndexer]"

also, this was an issue with the old comment too, but I think "indexes in a
[List]" is more accurate

[Siggi Cherem (dart-lang), 2014/03/26 00:32:07]

Done.

 // TODO(jmesserly): consider specialized subclasses for:
 // * empty path
 // * "value"
 // * single token in path, e.g. "foo"
 class PropertyPath {
   /// The segments of the path.
   final List<Object> _segments;
 
   /// Creates a new [PropertyPath]. These can be stored to avoid excessive
   /// parsing of path strings.
@@ skipping 76 matching lines @@
     for (int i = 0, len = _segments.length; i < len; i++) {
       hash = 0x1fffffff & (hash + _segments[i].hashCode);
       hash = 0x1fffffff & (hash + ((0x0007ffff & hash) << 10));
       hash = hash ^ (hash >> 6);
     }
     hash = 0x1fffffff & (hash + ((0x03ffffff & hash) <<  3));
     hash = hash ^ (hash >> 11);
     return 0x1fffffff & (hash + ((0x00003fff & hash) << 15));
   }
 
-  /// Returns the current of the path from the provided [obj]ect.
+  /// Returns the current value of the path from the provided [obj]ect.
   getValueFrom(Object obj) {
     if (!isValid) return null;
     for (var segment in _segments) {
       if (obj == null) return null;
       obj = _getObjectProperty(obj, segment);
     }
     return obj;
   }
 
   /// Attempts to set the [value] of the path from the provided [obj]ect.
@@ skipping 32 matching lines @@
   if (record is PropertyChangeRecord) {
     return (record as PropertyChangeRecord).name == key;
   }
   if (record is MapChangeRecord) {
     if (key is Symbol) key = smoke.symbolToName(key);
     return (record as MapChangeRecord).key == key;
   }
   return false;
 }
 
+/// Properties in [Map] that need to be read as properties and not as keys in
+/// the map. We exclude methods ('containsValue', 'containsKey', 'putIfAbsent',
+/// 'addAll', 'remove', 'clear', 'forEach') because there is no use in reading
+/// them as part of path-observer segments.
+const _MAP_PROPERTIES =
+    const ['keys', 'values', 'length', 'isEmpty', 'isNotEmpty'];
+
 _getObjectProperty(object, property) {
   if (object == null) return null;
 
   if (property is int) {
     if (object is List && property >= 0 && property < object.length) {
       return object[property];
     }
   } else if (property is Symbol) {
-    final type = object.runtimeType;
+    var name = smoke.symbolToName(property);

[Jennifer Messerly, 2014/03/25 22:32:44]

move this inside the if, below? and make _MAP_PROPERTIES use symbols then?

[Siggi Cherem (dart-lang), 2014/03/26 00:32:07]

Done.

+    // Support indexer if available, e.g. Maps or polymer_expressions Scope.
+    // This is the default syntax used by polymer/nodebind and
+    // polymer/observe-js PathObserver.
+    // TODO(sigmund): should we also support using checking dynamically for
+    // whether the type practically implements the indexer API
+    // (smoke.hasInstanceMethod(type, const Symbol('[]')))?
+    if (object is StringIndexer ||
+        object is Map && !_MAP_PROPERTIES.contains(name)) {
+      return object[name];
+    }
     try {
-      if (smoke.hasGetter(type, property) || smoke.hasNoSuchMethod(type)) {
-        return smoke.read(object, property);
-      }
-      // Support indexer if available, e.g. Maps or polymer_expressions Scope.
-      // This is the default syntax used by polymer/nodebind and
-      // polymer/observe-js PathObserver.
-      if (smoke.hasInstanceMethod(type, const Symbol('[]'))) {
-        return object[smoke.symbolToName(property)];
-      }
-    } on NoSuchMethodError catch (e) {
+      return smoke.read(object, property);
+    } on NoSuchMethodError catch (e, s) {
       // Rethrow, unless the type implements noSuchMethod, in which case we
       // interpret the exception as a signal that the method was not found.
-      if (!smoke.hasNoSuchMethod(type)) rethrow;
+      if (!smoke.hasNoSuchMethod(object.runtimeType)) {
+        new Completer().completeError(e, s);

[Jennifer Messerly, 2014/03/25 22:32:44]

hmmm. Upon second look I'm not sure this is the right place to throw the error
async. _getObjectProperty is used by PropertyPath, which is just a normal sync
API, and therefore should throw errors synchronously, IMO. So any
catch-rethrow-async must happen at a higher level. 

I think it would be better to leave this code rethrowing, and we can file a bug
if we find cases where template_binding is not rendering other things after a
binding throws. 

BTW, I'm really sorry for leading you down a wild goose chase of async-ifying
this :-(. It didn't click for me until seeing the code in _getObjectProperty why
I had intentionally made it a synchronous throw.

I went ahead and filed https://code.google.com/p/dart/issues/detail?id=17789

[Siggi Cherem (dart-lang), 2014/03/26 00:32:07]

No worries, changed it back to sync. Thanks for filing the bug.

+      }
     }
   }
 
   if (_logger.isLoggable(Level.FINER)) {
     _logger.finer("can't get $property in $object");
   }
   return null;
 }
 
 bool _setObjectProperty(object, property, value) {
   if (object == null) return false;
 
   if (property is int) {
     if (object is List && property >= 0 && property < object.length) {
       object[property] = value;
       return true;
     }
   } else if (property is Symbol) {
-    final type = object.runtimeType;
+    // Support indexer if available, e.g. Maps or polymer_expressions Scope.
+    var name = smoke.symbolToName(property);
+    if (object is StringIndexer ||
+        object is Map && !_MAP_PROPERTIES.contains(name)) {
+      object[name] = value;
+      return true;
+    }
     try {
-      if (smoke.hasSetter(type, property) || smoke.hasNoSuchMethod(type)) {
-        smoke.write(object, property, value);
-        return true;
+      smoke.write(object, property, value);
+      return true;
+    } on NoSuchMethodError catch (e, s) {
+      if (!smoke.hasNoSuchMethod(object.runtimeType)) {
+        new Completer().completeError(e, s);
       }
-      // Support indexer if available, e.g. Maps or polymer_expressions Scope.
-      if (smoke.hasInstanceMethod(type, const Symbol('[]='))) {
-        object[smoke.symbolToName(property)] = value;
-        return true;
-      }
-    } on NoSuchMethodError catch (e) {
-      if (!smoke.hasNoSuchMethod(type)) rethrow;
     }
   }
 
   if (_logger.isLoggable(Level.FINER)) {
     _logger.finer("can't set $property in $object");
   }
   return false;
 }
 
 // From: https://github.com/rafaelw/ChangeSummary/blob/master/change_summary.js
@@ skipping 163 matching lines @@
 
     if (!changed) return false;
 
     // TODO(rafaelw): Having _observed as the third callback arg here is
     // pretty lame API. Fix.
     _report(_value, oldValues, _observed);
     return true;
   }
 }
 
+/// An object accepted by [PropertyPath] where properties are read and written
+/// as indexing operations, just like a [Map].
+abstract class StringIndexer {

[Siggi Cherem (dart-lang), 2014/03/25 21:44:25]

I was not sure about the name. Other ideas/suggestions?

[Jennifer Messerly, 2014/03/25 22:32:44]

Indexable<K, V>? then do the is check for Indexable<String, dynamic>.

[Siggi Cherem (dart-lang), 2014/03/26 00:32:07]

Done.

+  operator [](String name);
+  operator []=(index, value);
+}
+
 const _observerSentinel = const _ObserverSentinel();
 class _ObserverSentinel { const _ObserverSentinel(); }
 
 // A base class for the shared API implemented by PathObserver and
 // CompoundObserver and used in _ObservedSet.
 abstract class _Observer extends Bindable {
   static int _nextBirthId = 0;
 
   /// A number indicating when the object was created.
   final int _birthId = _nextBirthId++;
@@ skipping 169 matching lines @@
     for (var observer in _observers.values.toList(growable: false)) {
       if (observer._isOpen) observer._check();
     }
 
     _resetNeeded = true;
     scheduleMicrotask(reset);
   }
 }
 
 const int _MAX_DIRTY_CHECK_CYCLES = 1000;