fix override checking of mixins

pkg/analyzer/lib/src/task/strong/checker.dart

 // Copyright (c) 2015, 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.
 
 // TODO(jmesserly): this was ported from package:dev_compiler, and needs to be
 // refactored to fit into analyzer.
 library analyzer.src.task.strong.checker;
 
 import 'dart:collection';
 import 'package:analyzer/analyzer.dart';
@@ skipping 1647 matching lines @@
         element.mixins.forEach(visitTypeAndSupertypes);
         element.interfaces.forEach(visitTypeAndSupertypes);
       }
     }
 
     visitTypeAndSupertypes(type);
 
     return genericSupertypes;
   }
 
-  /// Checks that members on this class correctly override all reachable
-  /// interfaces.
+  /// Checks that most-derived concrete members on this class correctly override
+  /// all reachable interfaces, and reports errors if all interfaces are not
+  /// correctly implemented.
+  ///
+  /// This checks the soundness property: for all interfaces implemented by this
+  /// class (including inherited interfaces), we ensure that calls through that
+  /// interface will be sound.
   void _checkAllInterfaceOverrides(Declaration node, ClassElement element) {
     var interfaces = _collectInterfacesToCheck(element.type);
     var visitedClasses = new Set<InterfaceType>();
+    var visitedMembers = new HashSet<String>();
 
-    // Visit all members on `type` (including inherited) and report errors if
-    // all `interfaces` are not correctly overridden.
+    // Checks all most-derived concrete members on this `type`. We skip over
+    // members that are already `visitedMembers`, because they were overridden
+    // and we've already checked that member.
     //
-    // Generally we only need to check the most derived concrete member, and
-    // we record members that have been seen already in `seen`. However we do
-    // report errors for every mixin member (unless it is overridden) by
-    // the class itself.
-    //
-    // TODO(jmesserly): what should mixins be doing? This behavior does not
-    // seem correct, but it is preserved for backwards compatibility.
-    void checkType(InterfaceType type, Set<String> seen, AstNode location) {
+    // Because of that, it is important we visit types in the order that they
+    // will override members.
+    void checkType(InterfaceType type, AstNode location) {
+      // Skip `Object` because we don't need to check those members here.
+      // (because `Object` is the root of everything, it will be checked in
+      // _checkSuperOverrides for all classes).
       if (type == null || type.isObject || !visitedClasses.add(type)) return;
 
       // Check `member` against all `interfaces`.
       void checkOverride(ExecutableElement member, [AstNode loc]) {
-        if (!seen.add(member.name)) return;
+        if (!visitedMembers.add(member.name)) return;
         for (var interface in interfaces) {
           if (_checkMemberOverride(member, interface, loc ?? location) ==
               false) {
             // Only report one error per member for interfaces.
             // TODO(jmesserly): this is for backwards compatibility. Remove it?
             break;
           }
         }
       }
 
+      // When we're checking the class declaration node we started from, we
+      // can use a more precise error location for reporting override errors.
+      //
+      // Otherwise, we'll use the `extends` or `with` clause.
+      var isRootClass = identical(location, node);
+
       // Check direct overrides on the class.
-      var isRootClass = identical(location, node);
       if (isRootClass) {
         _checkClassMembers(node, checkOverride);
       } else {
         _checkTypeMembers(type, checkOverride);
       }
 
       // Check mixin members against interfaces.
       //
-      // We want to check each mixin application class separately, so we report
-      // errors for any invalid overrides, even if multiple mixins have a member
-      // of the same name.
-      for (int i = 0; i < type.mixins.length; i++) {
-        checkType(type.mixins[i], new Set.from(seen),
+      // We visit mixins in reverse order to reflect how they override
+      // eachother.
+      for (int i = type.mixins.length - 1; i >= 0; i--) {
+        checkType(type.mixins[i],
             isRootClass ? _withClause(node).mixinTypes[i] : location);
       }
 
       // Check members on the superclass.
-      checkType(type.superclass, seen,
+      checkType(type.superclass,
           isRootClass ? _extendsErrorLocation(node) : location);
     }
 
-    checkType(element.type, new Set(), node);
+    checkType(element.type, node);
   }
 
   /// Gets the set of all interfaces on [type] that should be checked to see
   /// if type's members are overriding them correctly.
   ///
   /// In particular, we need to check these overrides for the definitions in
   /// the class itself and each its superclasses (and mixins).
   /// If a superclass (or mixin) is concrete, then we can skip its transitive
   /// interfaces, but if it is abstract we must check them. For example, in:
   ///
@@ skipping 261 matching lines @@
   }
 
   /// If node is a [ClassDeclaration] returns its members, otherwise if node is
   /// a [ClassTypeAlias] this returns an empty list.
   WithClause _withClause(Declaration node) {
     return node is ClassDeclaration
         ? node.withClause
         : (node as ClassTypeAlias).withClause;
   }
 }