feat: Removing implicit map dereference in expressions

Closes dart-archive#915

Before this PR a.b would have match either a.b or a['b']. The reason is
that the context was a Map (before dart-archive#914) and we don't want to write
ctrl['foo'] but ctrl.foo. It was also not possible to access Map
properties, ie map.length would have returned map['length'].

Now "a.b" return a.b and "a['b']" return a['b']

Author: vicb

lib/change_detection/dirty_checking_change_detector.dart

@@ -460,22 +460,17 @@ class DirtyCheckingRecord<H> implements WatchRecord<H> {
       return;
     }
 
-    if (object is Map) {
-      _mode =  _MODE_MAP_FIELD_;
-      _getter = null;
-    } else {
-      while (object is ContextLocals) {
-        var ctx = object as ContextLocals;
-        if (ctx.hasProperty(field)) {
-          _mode =  _MODE_MAP_FIELD_;
-          _getter = null;
-          return;
-        }
-        object = ctx.parentContext;
+    while (object is ContextLocals) {
+      var ctx = object as ContextLocals;
+      if (ctx.hasProperty(field)) {
+        _mode =  _MODE_MAP_FIELD_;
+        _getter = null;
+        return;
       }
-      _mode = _MODE_GETTER_OR_METHOD_CLOSURE_;
-      _getter = _fieldGetterFactory.getter(object, field);
+      object = ctx.parentContext;
     }
+    _mode = _MODE_GETTER_OR_METHOD_CLOSURE_;
+    _getter = _fieldGetterFactory.getter(object, field);
   }
 
   bool check() {

lib/core/parser/eval_access.dart

@@ -47,7 +47,6 @@ abstract class AccessFast {
 
   dynamic _eval(holder) {
     if (holder == null) return null;
-    if (holder is Map) return holder[name];
     return getter(holder);
   }
 
@@ -56,7 +55,6 @@ abstract class AccessFast {
       _assignToNonExisting(scope, value);
       return value;
     } else {
-      if (holder is Map) return holder[name] = value;
       return setter(holder, value);
     }
   }

lib/core/parser/parser_dynamic.dart

@@ -8,24 +8,14 @@ class DynamicClosureMap implements ClosureMap {
   final Map<String, Symbol> symbols = {};
   Getter lookupGetter(String name) {
     var symbol = new Symbol(name);
-    return (o) {
-      if (o is Map) {
-        return o[name];
-      } else {
-        return reflect(o).getField(symbol).reflectee;
-      }
-    };
+    return (o) => reflect(o).getField(symbol).reflectee;
   }
 
   Setter lookupSetter(String name) {
     var symbol = new Symbol(name);
     return (o, value) {
-      if (o is Map) {
-        return o[name] = value;
-      } else {
-        reflect(o).setField(symbol, value);
-        return value;
-      }
+      reflect(o).setField(symbol, value);
+      return value;
     };
   }
 
@@ -37,19 +27,10 @@ class DynamicClosureMap implements ClosureMap {
         var symbol = symbols.putIfAbsent(name, () => new Symbol(name));
         sNamedArgs[symbol] = value;
       });
-      if (o is Map) {
-        var fn = o[name];
-        if (fn is Function) {
-          return Function.apply(fn, posArgs, sNamedArgs);
-        } else {
-          throw "Property '$name' is not of type function.";
-        }
-      } else {
-        try {
-          return reflect(o).invoke(symbol, posArgs, sNamedArgs).reflectee;
-        } on NoSuchMethodError catch (e) {
-          throw 'Undefined function $name';
-        }
+      try {
+        return reflect(o).invoke(symbol, posArgs, sNamedArgs).reflectee;
+      } on NoSuchMethodError catch (e) {
+        throw 'Undefined function $name';
       }
     };
   }

lib/core/parser/parser_static.dart

@@ -27,16 +27,7 @@ class StaticClosureMap extends ClosureMap {
     return (o, posArgs, namedArgs) {
       var sNamedArgs = {};
       namedArgs.forEach((name, value) => sNamedArgs[symbols[name]] = value);
-      if (o is Map) {
-        var fn = o[name];
-        if (fn is Function) {
-          return Function.apply(fn, posArgs, sNamedArgs);
-        } else {
-          throw "Property '$name' is not of type function.";
-        }
-      } else {
-        return Function.apply(fn(o), posArgs, sNamedArgs);
-      }
+      return Function.apply(fn(o), posArgs, sNamedArgs);
     };
   }
 

lib/formatter/filter.dart

@@ -1,8 +1,7 @@
 part of angular.formatter_internal;
 
-// Too bad you can't stick typedef's inside a class.
-typedef bool _Predicate(e);
-typedef bool _Equals(a, b);
+typedef bool _PredicateFn(e);
+typedef bool _CompareFn(a, b);
 
 /**
  * Selects a subset of items from the provided [List] and returns it as a new
@@ -17,37 +16,37 @@ typedef bool _Equals(a, b);
  *
  * The expression can be of the following types:
  *
- * - [String], [bool] and [num]:  Only items in the list that directly
+ * - [String], [bool] and [num]: Only items in the list that directly
  *   match this expression, items that are Maps with any value matching this
  *   item, and items that are lists containing a matching items are returned.
  *
- * - [Map]:  This defines a pattern map.  Filters specific properties on objects
+ * - [Map]:  This defines a pattern map. Filters specific properties on objects
  *   contained in the input list.  For example `{name:"M", phone:"1"}` predicate
  *   will return a list of items which have property `name` containing "M" and
  *   property `phone` containing "1".  A special property name, `$`, can be used
  *   (as in `{$: "text"}`) to accept a match against any property of the object.
  *   That's equivalent to the simple substring match with a `String` as
  *   described above.
  *
- * - [Function]:  This allows you to supply a custom function to filter the
- *   List.  The function is called for each element of the List.  The returned
+ * - [Function]: This allows you to supply a custom function to filter the
+ *   List.  The function is called for each element of the List. The returned
  *   List contains exactly those elements for which this function returned
  *   `true`.
  *
  *
  * The comparator is optional and can be one of the following:
  *
- * - `bool comparator(expected, actual)`:  The function will be called with the
+ * - `bool comparator(expected, actual)`: The function will be called with the
  *   object value and the predicate value to compare and should return true if
  *   the item should be included in filtered result.
  *
  * - `true`:  Specifies that only identical objects matching the expression
- *   exactly should be considered matches.  Two strings are considered identical
- *   if they are equal.  Two numbers are considered identical if they are either
- *   equal or both are `NaN`.  All other objects are identical iff
+ *   exactly should be considered matches. Two strings are considered identical
+ *   if they are equal. Two numbers are considered identical if they are either
+ *   equal or both are `NaN`. All other objects are identical iff
  *   identical(expected, actual) is true.
  *
- * - `false|null`:  Specifies case insensitive substring matching.
+ * - `false|null`: Specifies case insensitive substring matching.
  *
  *
  * # Example ([view in plunker](http://plnkr.co/edit/6Mxz6r?p=info)):
@@ -105,27 +104,30 @@ typedef bool _Equals(a, b);
 @Formatter(name: 'filter')
 class Filter implements Function {
   Parser _parser;
-  _Equals _comparator;
-  _Equals _stringComparator;
+  _CompareFn _comparator;
+  _CompareFn _stringComparator;
 
   static _nop(e) => e;
+
   static _ensureBool(val) => (val is bool && val);
+
   static _isSubstringCaseInsensitive(String a, String b) =>
       a != null && b != null && a.toLowerCase().contains(b.toLowerCase());
+
   static _identical(a, b) => identical(a, b) ||
                              (a is String && b is String && a == b) ||
                              (a is num && b is num && a.isNaN && b.isNaN);
 
   Filter(this._parser);
 
-  void _configureComparator(var comparatorExpression) {
+  void _configureComparator(comparatorExpression) {
     if (comparatorExpression == null || comparatorExpression == false) {
       _stringComparator = _isSubstringCaseInsensitive;
       _comparator = _defaultComparator;
     } else if (comparatorExpression == true) {
       _stringComparator = _identical;
       _comparator = _defaultComparator;
-    } else if (comparatorExpression is _Equals) {
+    } else if (comparatorExpression is _CompareFn) {
       _comparator = (a, b) => _ensureBool(comparatorExpression(a, b));
     } else {
       _comparator = null;
@@ -135,67 +137,58 @@ class Filter implements Function {
   // Preconditions
   // - what: NOT a Map
   // - item: neither a Map nor a List
-  bool _defaultComparator(var item, var what) {
-    if (what == null) {
-      return false;
-    } else if (item == null) {
-      return what == '';
-    } else if (what is String && what.startsWith('!')) {
-      return !_search(item, what.substring(1));
-    } else if (item is String) {
-      return (what is String) && _stringComparator(item, what);
-    } else if (item is bool) {
+  bool _defaultComparator(item, what) {
+    if (what == null) return false;
+    if (item == null) return what == '';
+    if (what is String && what.startsWith('!')) return !_search(item, what.substring(1));
+    if (item is String) return (what is String) && _stringComparator(item, what);
+    if (item is bool) {
       if (what is bool) {
         return item == what;
       } else if (what is String) {
         what = (what as String).toLowerCase();
         return item ? (what == "true"  || what == "yes" || what == "on")
                     : (what == "false" || what == "no"  || what == "off");
-      } else {
-        return false;
       }
-    } else if (item is num) {
-      if (what is num) {
-        return item == what || (item.isNaN && what.isNaN);
-      } else {
-        return what is String && _stringComparator('$item', what);
-      }
-    } else {
-      return false; // Unsupported item type.
+      return false;
     }
+    if (item is num) {
+      return what is num ?
+          item == what || (item.isNaN && what.isNaN) :
+          what is String && _stringComparator('$item', what);
+    }
+    return false; // Unsupported item type.
   }
 
-  bool _search(var item, var what) {
+  bool _search(item, what) {
     if (what is Map) {
       return what.keys.every((key) => _search(
               (key == r'$') ? item : _parser(key).eval(item), what[key]));
-    } else if (item is Map) {
-      return item.keys.any((k) => !k.startsWith(r'$') && _search(item[k], what));
-    } else if (item is List) {
-      return item.any((i) => _search(i, what));
-    } else {
-      return _comparator(item, what);
     }
+    if (item is Map) return item.keys.any((k) => !k.startsWith(r'$') && _search(item[k], what));
+    if (item is List) return item.any((i) => _search(i, what));
+    return _comparator(item, what);
   }
 
-  _Predicate _toPredicate(var expression) {
-    if (expression is _Predicate) {
-      return (item) => _ensureBool(expression(item));
-    } else if (_comparator == null) {
-      return (item) => false; // Bad comparator → no items for you!
-    } else {
-      return (item) => _search(item, expression);
-    }
+  _PredicateFn _toPredicate(var expression) {
+    if (expression is _PredicateFn) return (item) => _ensureBool(expression(item));
+    if (_comparator == null) return (_) => false; // Bad comparator → no items for you!
+    return (item) => _search(item, expression);
   }
 
-  List call(List items, var expression, [var comparator]) {
-    if (expression == null) {
-      return items.toList(growable: false); // Missing expression → passthrough.
-    } else if (expression is! Map && expression is! Function &&
-               expression is! String && expression is! bool &&
-               expression is! num) {
-      return const []; // Bad expression → no items for you!
+  List call(List items, expression, [comparator]) {
+    // Missing expression → passthrough.
+    if (expression == null) return items.toList(growable: false);
+
+    // Bad expression → no items for you!
+    if (expression is! Map &&
+        expression is! Function &&
+        expression is! String &&
+        expression is! bool &&
+        expression is! num) {
+      return const [];
     }
+
     _configureComparator(comparator);
     List results = items.where(_toPredicate(expression)).toList(growable: false);
     _comparator = null;

lib/mock/module.dart

@@ -17,6 +17,7 @@ import 'dart:async' as dart_async;
 import 'dart:collection' show ListBase;
 import 'dart:html';
 import 'dart:js' as js;
+import 'dart:mirrors' as mirrors;
 
 import 'package:angular/angular.dart';
 import 'package:angular/core/module_internal.dart';
@@ -60,10 +61,57 @@ class AngularMockModule extends Module {
     bind(Element, toValue: document.body);
     bind(Node, toValue: document.body);
     bind(HttpBackend, toFactory: (Injector i) => i.get(MockHttpBackend));
-    bind(VmTurnZone, toFactory: (_) {
-      return new VmTurnZone()
-        ..onError = (e, s, LongStackTrace ls) => dump('EXCEPTION: $e\n$s\n$ls');
-    });
+    bind(VmTurnZone, toFactory: (_) =>
+        new VmTurnZone()..onError = (e, s, LongStackTrace ls) => dump('EXCEPTION: $e\n$s\n$ls'));
     bind(Window, toImplementation: MockWindow);
+    bind(Object, toImplementation: DynamicObject);
+  }
+}
+
+/**
+ * [DynamicObject] helps testing angular.dart.
+ *
+ * Before GH-915, the `foo.bar` expression would evaluate either `foo.bar` or `foo["bar"]`. A lot
+ * of unit tests are written using `scope.context["foo"]=bar`. To test this after GH-915, you should
+ * create a context class with a `foo` property and initialize it with `scope.context.foo = bar`.
+ *
+ * Setting the test context to an instance of [DynamicObject] avoid having to write a specific class
+ * for every new test by allowing the dynamic addition of properties through the use of
+ * [Object.noSuchMethod]
+ *
+ */
+class DynamicObject {
+  Map _locals = {};
+
+  void addProperties(Map<String, dynamic> locals) {
+    assert(locals != null);
+    locals.forEach((k, v) => this[k] = v);
+  }
+
+  // todo(vicb) - It would be great to remove this operator overloading when all tests are
+  // converted from `scope['foo'] = bar` to `scope.foo = bar`
+  void operator []=(k, v) {
+    _locals[k] = v;
+  }
+
+  // todo(vicb) - It would be great to remove this operator overloading when all tests are
+  // converted from `scope['foo']` to `scope.foo`
+  dynamic operator[](k) => _locals[k];
+
+  noSuchMethod(Invocation invocation) {
+    var pArgs = invocation.positionalArguments;
+    var field = mirrors.MirrorSystem.getName(invocation.memberName);
+    if (invocation.isGetter) {
+      return _locals[field];
+    }
+    if (invocation.isSetter) {
+      field = field.substring(0, field.length - 1);
+      _locals[field] = pArgs[0];
+      return;
+    }
+    if (invocation.isMethod) {
+      return Function.apply(_locals[field], pArgs, invocation.namedArguments);
+    }
+    throw new UnimplementedError(field);
   }
 }