fix #29233, final fields can be settable in a mock

also fix #29273, user mixins can override native methods

R=vsm@google.com

Review-Url: https://codereview.chromium.org/2803673007 .

pkg/dev_compiler/lib/src/compiler/code_generator.dart

@@ -910,10 +910,9 @@ class CodeGenerator extends GeneralizingAstVisitor
       className = _emitTopLevelName(classElem);
     }
 
-    var extensions = _extensionsToImplement(classElem);
     var savedClassProperties = _classProperties;
     _classProperties =
-        new ClassPropertyModel.build(virtualFields, classElem, extensions);
+        new ClassPropertyModel.build(_extensionTypes, virtualFields, classElem);
 
     var classExpr = _emitClassExpression(
         classElem, _emitClassMethods(node, ctors, fields),
@@ -935,9 +934,8 @@ class CodeGenerator extends GeneralizingAstVisitor
 
     _defineNamedConstructors(ctors, body, className, isCallableTransitive);
     _emitVirtualFieldSymbols(classElem, body);
-    _emitClassSignature(
-        methods, allFields, classElem, ctors, extensions, className, body);
-    _defineExtensionMembers(extensions, className, body);
+    _emitClassSignature(methods, allFields, classElem, ctors, className, body);
+    _defineExtensionMembers(className, body);
     _emitClassMetadata(node.metadata, className, body);
 
     JS.Statement classDef = _statement(body);
@@ -1484,13 +1482,14 @@ class CodeGenerator extends GeneralizingAstVisitor
         if (m.isStatic) continue;
         for (VariableDeclaration field in m.fields.variables) {
           if (virtualFields.containsKey(field.element)) {
-            jsMethods.addAll(_emitVirtualFieldAccessor(field, virtualFields));
+            jsMethods.addAll(_emitVirtualFieldAccessor(field));
           }
         }
       }
     }
 
-    jsMethods.addAll(_implementMockInterfaces(type));
+    jsMethods.addAll(_classProperties.mockMembers.values
+        .map((e) => _implementMockMember(e, type)));
 
     // If the type doesn't have an `iterator`, but claims to implement Iterable,
     // we inject the adaptor method here, as it's less code size to put the
@@ -1511,64 +1510,6 @@ class CodeGenerator extends GeneralizingAstVisitor
     return jsMethods.where((m) => m != null).toList(growable: false);
   }
 
-  Iterable<ExecutableElement> _collectMockMethods(InterfaceType type) {
-    var element = type.element;
-    if (!_hasNoSuchMethod(element)) {
-      return [];
-    }
-
-    // Collect all unimplemented members.
-    //
-    // Initially, we track abstract and concrete members separately, then
-    // remove concrete from the abstract set. This is done because abstract
-    // members are allowed to "override" concrete ones in Dart.
-    // (In that case, it will still be treated as a concrete member and can be
-    // called at run time.)
-    var abstractMembers = new Map<String, ExecutableElement>();
-    var concreteMembers = new HashSet<String>();
-
-    void visit(InterfaceType type, bool isAbstract) {
-      if (type == null) return;
-      visit(type.superclass, isAbstract);
-      for (var m in type.mixins) visit(m, isAbstract);
-      for (var i in type.interfaces) visit(i, true);
-
-      var members = <ExecutableElement>[]
-        ..addAll(type.methods)
-        ..addAll(type.accessors);
-      for (var m in members) {
-        if (isAbstract || m.isAbstract) {
-          // Inconsistent signatures are disallowed, even with nSM, so we don't
-          // need to worry too much about which abstract member we save.
-          abstractMembers[m.name] = m;
-        } else {
-          concreteMembers.add(m.name);
-        }
-      }
-    }
-
-    visit(type, false);
-
-    concreteMembers.forEach(abstractMembers.remove);
-    return abstractMembers.values;
-  }
-
-  Iterable<JS.Method> _implementMockInterfaces(InterfaceType type) {
-    // TODO(jmesserly): every type with nSM will generate new stubs for all
-    // abstract members. For example:
-    //
-    //     class C { m(); noSuchMethod(...) { ... } }
-    //     class D extends C { m(); noSuchMethod(...) { ... } }
-    //
-    // We'll generate D.m even though it is not necessary.
-    //
-    // Doing better is a bit tricky, as our current codegen strategy for the
-    // mock methods encodes information about the number of arguments (and type
-    // arguments) that D expects.
-    return _collectMockMethods(type)
-        .map((method) => _implementMockMethod(method, type));
-  }
-
   /// Given a class C that implements method M from interface I, but does not
   /// declare M, this will generate an implementation that forwards to
   /// noSuchMethod.
@@ -1588,7 +1529,7 @@ class CodeGenerator extends GeneralizingAstVisitor
   ///       return core.bool.as(this.noSuchMethod(
   ///           new dart.InvocationImpl('eatFood', args)));
   ///     }
-  JS.Method _implementMockMethod(ExecutableElement method, InterfaceType type) {
+  JS.Method _implementMockMember(ExecutableElement method, InterfaceType type) {
     var invocationProps = <JS.Property>[];
     addProperty(String name, JS.Expression value) {
       invocationProps.add(new JS.Property(js.string(name), value));
@@ -1646,19 +1587,6 @@ class CodeGenerator extends GeneralizingAstVisitor
         isStatic: false);
   }
 
-  /// Return `true` if the given [classElement] has a noSuchMethod() method
-  /// distinct from the one declared in class Object, as per the Dart Language
-  /// Specification (section 10.4).
-  // TODO(jmesserly): this was taken from error_verifier.dart
-  bool _hasNoSuchMethod(ClassElement classElement) {
-    // TODO(jmesserly): this is slow in Analyzer. It's a linear scan through all
-    // methods, up through the class hierarchy.
-    MethodElement method = classElement.lookUpMethod(
-        FunctionElement.NO_SUCH_METHOD_METHOD_NAME, classElement.library);
-    var definingClass = method?.enclosingElement;
-    return definingClass != null && !definingClass.type.isObject;
-  }
-
   /// This is called whenever a derived class needs to introduce a new field,
   /// shadowing a field or getter/setter pair on its parent.
   ///
@@ -1666,21 +1594,26 @@ class CodeGenerator extends GeneralizingAstVisitor
   /// would end up calling the getter or setter, and one of those might not even
   /// exist, resulting in a runtime error. Even if they did exist, that's the
   /// wrong behavior if a new field was declared.
-  List<JS.Method> _emitVirtualFieldAccessor(VariableDeclaration field,
-      Map<FieldElement, JS.TemporaryId> virtualFields) {
-    var virtualField = virtualFields[field.element];
+  List<JS.Method> _emitVirtualFieldAccessor(VariableDeclaration field) {
+    var element = field.element as FieldElement;
+    var virtualField = _classProperties.virtualFields[element];
     var result = <JS.Method>[];
-    var name = _declareMemberName((field.element as FieldElement).getter);
-    var getter = js.call('function() { return this[#]; }', [virtualField]);
-    result.add(new JS.Method(name, getter, isGetter: true));
+    var name = _declareMemberName(element.getter);
 
-    if (field.isFinal) {
-      var setter = js.call('function(value) { super[#] = value; }', [name]);
-      result.add(new JS.Method(name, setter, isSetter: true));
-    } else {
-      var setter =
-          js.call('function(value) { this[#] = value; }', [virtualField]);
-      result.add(new JS.Method(name, setter, isSetter: true));
+    var mocks = _classProperties.mockMembers;
+    if (!mocks.containsKey(element.name)) {
+      var getter = js.call('function() { return this[#]; }', [virtualField]);
+      result.add(new JS.Method(name, getter, isGetter: true));
+    }
+
+    if (!mocks.containsKey(element.name + '=')) {
+      var args = field.isFinal
+          ? [new JS.Super(), name]
+          : [new JS.This(), virtualField];
+
+      result.add(new JS.Method(
+          name, js.call('function(value) { #[#] = value; }', args),
+          isSetter: true));
     }
 
     return result;
@@ -1861,20 +1794,25 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   /// If a concrete class implements one of our extensions, we might need to
   /// add forwarders.
-  void _defineExtensionMembers(List<ExecutableElement> extensions,
+  void _defineExtensionMembers(
       JS.Expression className, List<JS.Statement> body) {
-    // If a concrete class implements one of our extensions, we might need to
-    // add forwarders.
-    if (extensions.isNotEmpty) {
-      var methodNames = <JS.Expression>[];
-      for (var e in extensions) {
-        methodNames.add(_declareMemberName(e, useExtension: false));
-      }
+    void emitExtensions(
+        JS.Expression target, Iterable<ExecutableElement> extensions) {
+      if (extensions.isEmpty) return;
+
+      var names = extensions
+          .map((e) => _declareMemberName(e, useExtension: false))
+          .toList();
       body.add(_callHelperStatement('defineExtensionMembers(#, #);', [
-        className,
-        new JS.ArrayInitializer(methodNames, multiline: methodNames.length > 4)
+        target,
+        new JS.ArrayInitializer(names, multiline: names.length > 4)
       ]));
     }
+
+    // Define mixin members (if any) on the mixin class.
+    var mixinClass = js.call('#.__proto__', [className]);
+    emitExtensions(mixinClass, _classProperties.mixinExtensionMembers);
+    emitExtensions(className, _classProperties.extensionMembers);
   }
 
   JS.Property _buildSignatureField(String name, List<JS.Property> elements) {
@@ -1890,7 +1828,6 @@ class CodeGenerator extends GeneralizingAstVisitor
       List<FieldDeclaration> fields,
       ClassElement classElem,
       List<ConstructorDeclaration> ctors,
-      List<ExecutableElement> extensions,
       JS.Expression className,
       List<JS.Statement> body) {
     if (classElem.interfaces.isNotEmpty) {
@@ -2039,7 +1976,11 @@ class CodeGenerator extends GeneralizingAstVisitor
       sigFields.add(_buildSignatureField('statics', tStaticMethods));
       sigFields.add(aNames);
     }
-    if (!sigFields.isEmpty || extensions.isNotEmpty) {
+    // We set signature here, even if empty, to simplify the work of
+    // defineExtensionMembers at runtime. See _defineExtensionMembers.
+    if (!sigFields.isEmpty ||
+        _classProperties.extensionMembers.isNotEmpty ||
+        _classProperties.mixinExtensionMembers.isNotEmpty) {
       var sig = new JS.ObjectInitializer(sigFields);
       body.add(_callHelperStatement('setSignature(#, #);', [className, sig]));
     }
@@ -2083,34 +2024,6 @@ class CodeGenerator extends GeneralizingAstVisitor
     }
   }
 
-  List<ExecutableElement> _extensionsToImplement(ClassElement element) {
-    if (_extensionTypes.isNativeClass(element)) return [];
-
-    // Collect all extension types we implement.
-    var type = element.type;
-    var types = _extensionTypes.collectNativeInterfaces(element);
-    if (types.isEmpty) return [];
-
-    var members = new Set<ExecutableElement>();
-    // Collect all possible extension method names.
-    var extensionMembers = new HashSet<String>();
-    for (var t in types) {
-      for (var m in [t.methods, t.accessors].expand((e) => e)) {
-        if (!m.isStatic && m.isPublic) extensionMembers.add(m.name);
-      }
-    }
-
-    // Collect all of extension methods this type implements.
-    for (var m in [type.methods, type.accessors].expand((e) => e)) {
-      if (!m.isStatic && !m.isAbstract && extensionMembers.contains(m.name)) {
-        members.add(m);
-      }
-    }
-    members.addAll(_collectMockMethods(type)
-        .where((m) => extensionMembers.contains(m.name)));
-    return members.toList();
-  }
-
   /// Generates the implicit default constructor for class C of the form
   /// `C() : super() {}`.
   JS.Method _emitImplicitConstructor(

pkg/dev_compiler/lib/src/compiler/element_helpers.dart

@@ -155,5 +155,21 @@ List<ClassElement> getImmediateSuperclasses(ClassElement c) {
   return result;
 }
 
+/// Returns true if the library [l] is dart:_runtime.
+// TODO(jmesserly): unlike other methods in this file, this one wouldn't be
+// suitable for upstream to Analyzer, as it's DDC specific.
 bool isSdkInternalRuntime(LibraryElement l) =>
     l.isInSdk && l.source.uri.toString() == 'dart:_runtime';
+
+/// Return `true` if the given [classElement] has a noSuchMethod() method
+/// distinct from the one declared in class Object, as per the Dart Language
+/// Specification (section 10.4).
+// TODO(jmesserly): this was taken from error_verifier.dart
+bool hasNoSuchMethod(ClassElement classElement) {
+  // TODO(jmesserly): this is slow in Analyzer. It's a linear scan through all
+  // methods, up through the class hierarchy.
+  var method = classElement.lookUpMethod(
+      FunctionElement.NO_SUCH_METHOD_METHOD_NAME, classElement.library);
+  var definingClass = method?.enclosingElement;
+  return definingClass != null && !definingClass.type.isObject;
+}

pkg/dev_compiler/lib/src/compiler/js_metalet.dart

@@ -4,7 +4,6 @@
 
 // TODO(jmesserly): import from its own package
 import '../js_ast/js_ast.dart';
-import '../js_ast/precedence.dart';
 
 import 'js_names.dart' show TemporaryId;
 

pkg/dev_compiler/lib/src/compiler/property_model.dart

@@ -4,12 +4,13 @@
 
 import 'dart:collection' show HashMap, HashSet, Queue;
 
-import 'package:analyzer/dart/ast/ast.dart' show Identifier;
 import 'package:analyzer/dart/element/element.dart';
+import 'package:analyzer/dart/element/type.dart' show InterfaceType;
 import 'package:analyzer/src/dart/element/element.dart' show FieldElementImpl;
 
 import '../js_ast/js_ast.dart' as JS;
 import 'element_helpers.dart';
+import 'extension_types.dart';
 import 'js_names.dart' as JS;
 
 /// Dart allows all fields to be overridden.
@@ -184,8 +185,13 @@ class ClassPropertyModel {
   /// super.
   final inheritedSetters = new HashSet<String>();
 
-  ClassPropertyModel.build(VirtualFieldModel fieldModel, ClassElement classElem,
-      Iterable<ExecutableElement> extensionMembers) {
+  final mockMembers = <String, ExecutableElement>{};
+
+  final extensionMembers = new Set<ExecutableElement>();
+  final mixinExtensionMembers = new Set<ExecutableElement>();
+
+  ClassPropertyModel.build(ExtensionTypeSet extensionTypes,
+      VirtualFieldModel fieldModel, ClassElement classElem) {
     // Visit superclasses to collect information about their fields/accessors.
     // This is expensive so we try to collect everything in one pass.
     for (var base in getSuperclasses(classElem)) {
@@ -194,20 +200,26 @@ class ClassPropertyModel {
         if (accessor.correspondingGetter != null) continue;
 
         var field = accessor.variable;
-        var name = field.name;
         // Ignore private names from other libraries.
-        if (Identifier.isPrivateName(name) &&
-            accessor.library != classElem.library) {
+        if (field.isPrivate && accessor.library != classElem.library) {
           continue;
         }
 
-        if (field.getter?.isAbstract == false) inheritedGetters.add(name);
-        if (field.setter?.isAbstract == false) inheritedSetters.add(name);
+        if (field.getter?.isAbstract == false) inheritedGetters.add(field.name);
+        if (field.setter?.isAbstract == false) inheritedSetters.add(field.name);
       }
     }
 
-    var extensionNames =
-        new HashSet<String>.from(extensionMembers.map((e) => e.name));
+    _collectMockMembers(classElem.type);
+    _collectExtensionMembers(extensionTypes, classElem);
+
+    var virtualAccessorNames = new HashSet<String>()
+      ..addAll(inheritedGetters)
+      ..addAll(inheritedSetters)
+      ..addAll(extensionMembers
+          .map((m) => m is PropertyAccessorElement ? m.variable.name : m.name))
+      ..addAll(mockMembers.values
+          .map((m) => m is PropertyAccessorElement ? m.variable.name : m.name));
 
     // Visit accessors in the current class, and see if they need to be
     // generated differently based on the inherited fields/accessors.
@@ -221,9 +233,7 @@ class ClassPropertyModel {
       var name = field.name;
       // Is it a field?
       if (!field.isSynthetic && field is FieldElementImpl) {
-        if (inheritedGetters.contains(name) ||
-            inheritedSetters.contains(name) ||
-            extensionNames.contains(name) ||
+        if (virtualAccessorNames.contains(name) ||
             fieldModel.isVirtual(field)) {
           if (field.isStatic) {
             staticFieldOverrides.add(field);
@@ -234,4 +244,87 @@ class ClassPropertyModel {
       }
     }
   }
+
+  void _collectMockMembers(InterfaceType type) {
+    // TODO(jmesserly): every type with nSM will generate new stubs for all
+    // abstract members. For example:
+    //
+    //     class C { m(); noSuchMethod(...) { ... } }
+    //     class D extends C { m(); noSuchMethod(...) { ... } }
+    //
+    // We'll generate D.m even though it is not necessary.
+    //
+    // Doing better is a bit tricky, as our current codegen strategy for the
+    // mock methods encodes information about the number of arguments (and type
+    // arguments) that D expects.
+    var element = type.element;
+    if (!hasNoSuchMethod(element)) return;
+
+    // Collect all unimplemented members.
+    //
+    // Initially, we track abstract and concrete members separately, then
+    // remove concrete from the abstract set. This is done because abstract
+    // members are allowed to "override" concrete ones in Dart.
+    // (In that case, it will still be treated as a concrete member and can be
+    // called at runtime.)
+    var concreteMembers = new HashSet<String>();
+
+    void visit(InterfaceType type, bool isAbstract) {
+      if (type == null) return;
+      visit(type.superclass, isAbstract);
+      for (var m in type.mixins) visit(m, isAbstract);
+      for (var i in type.interfaces) visit(i, true);
+
+      for (var m in [type.methods, type.accessors].expand((m) => m)) {
+        if (isAbstract || m.isAbstract) {
+          mockMembers[m.name] = m;
+        } else if (!m.isStatic) {
+          concreteMembers.add(m.name);
+        }
+      }
+    }
+
+    visit(type, false);
+
+    concreteMembers.forEach(mockMembers.remove);
+  }
+
+  void _collectExtensionMembers(
+      ExtensionTypeSet extensionTypes, ClassElement element) {
+    if (extensionTypes.isNativeClass(element)) return;
+
+    // Collect all extension types we implement.
+    var type = element.type;
+    var types = extensionTypes.collectNativeInterfaces(element);
+    if (types.isEmpty) return;
+
+    // Collect all possible extension method names.
+    var possibleExtensions = new HashSet<String>();
+    for (var t in types) {
+      for (var m in [t.methods, t.accessors].expand((m) => m)) {
+        if (!m.isStatic && m.isPublic) possibleExtensions.add(m.name);
+      }
+    }
+
+    // Collect all of extension methods this type and its mixins implement.
+
+    void visitType(InterfaceType type, bool isMixin) {
+      for (var mixin in type.mixins) {
+        // If the mixin isn't native, make sure to visit it too, because those
+        // methods haven't been accounted for yet.
+        if (!extensionTypes.hasNativeSubtype(mixin)) visitType(mixin, true);
+      }
+      for (var m in [type.methods, type.accessors].expand((m) => m)) {
+        if (!m.isAbstract && possibleExtensions.contains(m.name)) {
+          (isMixin ? mixinExtensionMembers : extensionMembers).add(m);
+        }
+      }
+    }
+
+    visitType(type, false);
+
+    for (var m in mockMembers.values) {
+      if (possibleExtensions.contains(m.name)) extensionMembers.add(m);
+    }
+  }
 }

tests/language_strong/mixin_and_extension_member_test.dart

@@ -0,0 +1,32 @@
+// Copyright (c) 2017, 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.
+
+import 'dart:collection';
+import 'package:expect/expect.dart';
+
+class OverrideFirstGetter {
+  get first => 9999;
+}
+
+class ListMock extends ListBase with OverrideFirstGetter {
+  final _list = [];
+  int get length => _list.length;
+  void set length(int x) {
+    _list.length = x;
+  }
+
+  operator [](x) => _list[x];
+  void operator []=(x, y) {
+    _list[x] = y;
+  }
+}
+
+// Regression test for
+// https://github.com/dart-lang/sdk/issues/29273#issuecomment-292384130
+main() {
+  List x = new ListMock();
+  x.add(42);
+  Expect.equals(x[0], 42);
+  Expect.equals(x.first, 9999);
+}

tests/language_strong/mock_writable_final_field_test.dart

@@ -0,0 +1,40 @@
+// Copyright (c) 2017, 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.
+
+import 'package:expect/expect.dart';
+
+final values = <int>[];
+
+class Mock {
+  noSuchMethod(Invocation i) {
+    Expect.equals(i.memberName.toString(), 'Symbol("_x")');
+    values.add(i.positionalArguments[0]);
+  }
+}
+
+class Foo {
+  int _x;
+}
+
+class Bar extends Mock implements Foo {
+  final int _x = 42;
+}
+
+void main() {
+  {
+    Bar b = new Bar();
+    Expect.equals(b._x, 42);
+    b._x = 123;
+    Expect.listEquals(values, [123]);
+    values.clear();
+  }
+  {
+    // It works the same if called statically through the Foo interface.
+    Foo b = new Bar();
+    Expect.equals(b._x, 42);
+    b._x = 123;
+    Expect.listEquals(values, [123]);
+    values.clear();
+  }
+}

tests/lib_strong/html/html_mock_test.dart

@@ -16,13 +16,19 @@ class MockBodyElement extends Mock implements BodyElement {
   Node append(Node e) => e;
 }
 
+class _EventListeners {
+  Stream<Event> get onBlur => new Stream.fromIterable([]);
+}
+
 @proxy
-class MockHtmlDocument extends Mock implements HtmlDocument {
+class MockHtmlDocument extends Mock
+    with _EventListeners
+    implements HtmlDocument {
   BodyElement get body => new MockBodyElement();
 }
 
 @proxy
-class MockWindow extends Mock implements Window {
+class MockWindow extends Mock with _EventListeners implements Window {
   Stream<Event> get onBeforeUnload => new Stream.fromIterable([]);
 
   String name = "MOCK_NAME";
@@ -65,4 +71,11 @@ main() {
     HtmlDocument doc = new MockHtmlDocument();
     expect(doc.body.append(null), equals(null));
   });
+
+  test('mixin', () {
+    Window win = new MockWindow();
+    expect(win.onBlur is Stream, isTrue, reason: 'onBlur should be a stream');
+    HtmlDocument doc = new MockHtmlDocument();
+    expect(doc.onBlur is Stream, isTrue, reason: 'onBlur should be a stream');
+  });
 }