Avoid issuing incorrect errors when super mixins are enabled

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 1388 matching lines @@
 
     // For members of the superclass, we may need to add checks because this
     // class adds a new unsafe interface. Collect those checks.
     checks = _findSuperclassCovariantChecks(
         element, allCovariant, seenConcreteMembers);
     // Store the checks on the class declaration, it will need to ensure the
     // inherited members are appropriately guarded to ensure soundness.
     setSuperclassCovariantParameters(node, checks);
   }
 
   /// For each member of this class and non-overridden inherited member, we

[Jennifer Messerly, 2017/07/27 21:07:58]

fyi: this function is also doing a "concrete member" walk ... so it might have a
bug related to super mixins

   /// check to see if any generic super interface permits an unsound call to the
   /// concrete member. For example:
   ///
   /// We must check non-overridden inherited members because this class could
   /// contain a new interface that permits unsound access to that member. In
   /// those cases, the class is expected to insert stub that checks the type
   /// before calling `super`. For example:
   ///
   ///     class C<T> {
   ///       add(T t) {}
@@ skipping 20 matching lines @@
       var oldCovariant = _findAllGenericInterfaces(type);
       var newCovariant = allCovariant.difference(oldCovariant);
       if (newCovariant.isEmpty) return;
 
       void visitSuper(InterfaceType type) {
         var element = type.element;
         if (visited.add(element)) {
           var members = _getConcreteMembers(type, seenConcreteMembers);
           _findCovariantChecks(members, newCovariant, superChecks);
           _findCovariantChecks(members, oldCovariant, existingChecks);
           element.mixins.reversed.forEach(visitSuper);

[Jennifer Messerly, 2017/07/27 21:07:58]

this could be problematic

           var s = element.supertype;
           if (s != null) visitSuper(s);
         }
       }
 
       visitSuper(type);
     }
 
     element.mixins.reversed.forEach(visitImmediateSuper);
     var s = element.supertype;
@@ skipping 169 matching lines @@
     var interfaces = _collectInterfacesToCheck(element.type);
     var visitedClasses = new Set<InterfaceType>();
     var visitedMembers = new HashSet<String>();
 
     // 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.
     //
     // Because of that, it is important we visit types in the order that they
     // will override members.
-    void checkType(InterfaceType type, AstNode location) {
+    // If checkingMixin is true, then we are checking [type] in a mixin position
+    // and hence should consider its own mixins and superclass as abstract.
+    void checkType(InterfaceType type, AstNode location,
+        {bool checkingMixin: false}) {
       // 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 (!visitedMembers.add(member.name)) return;
         for (var interface in interfaces) {
           if (_checkMemberOverride(member, interface, loc ?? location) ==
@@ skipping 11 matching lines @@
       // Otherwise, we'll use the `extends` or `with` clause.
       var isRootClass = identical(location, node);
 
       // Check direct overrides on the class.
       if (isRootClass) {
         _checkClassMembers(node, checkOverride);
       } else {
         _checkTypeMembers(type, checkOverride);
       }
 
-      // Check mixin members against interfaces.
-      //
-      // 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);
+      // If we are currently checking a mixin, then its own mixins and
+      // superclass are abstract, and we should not check their members.
+      // This should only happen when super mixins is enabled, and we
+      // don't do proper checking for super mixins (we don't check that
+      // the contract implied by the mixin declaration is satisfied by
+      // the mixin use), but this prevents us from erroneously
+      // rejecting some super mixin patterns.
+      // If this is a mixin application (class A = Object with B)
+      // however, then we do still need to treat the mixin as concrete.
+      if (!checkingMixin || type.element.isMixinApplication) {
+        // Check mixin members against interfaces.
+        //
+        // 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,
+              checkingMixin: true);
+        }
+
+        // Check members on the superclass.
+        checkType(type.superclass,
+            isRootClass ? _extendsErrorLocation(node) : location,
+            checkingMixin: checkingMixin);
       }
-
-      // Check members on the superclass.
-      checkType(type.superclass,
-          isRootClass ? _extendsErrorLocation(node) : location);
     }
 
     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).
@@ skipping 23 matching lines @@
     // Check all interfaces reachable from the `implements` clause in the
     // current class against definitions here and in superclasses.
     type.interfaces.forEach(collectInterfaces);
 
     // Also collect interfaces from any abstract mixins or superclasses.
     //
     // For a concrete mixin/superclass, we'll check that we override the
     // concrete members in _checkSuperOverrides and
     // _checkMixinApplicationOverrides. But for abstract classes, we need to
     // consider any abstract members it got from its interfaces.
     for (var s in _getSuperclasses(type, (t) => t.element.isAbstract)) {

[Jennifer Messerly, 2017/07/27 21:07:58]

based on comments below, this call site may need to change

       s.interfaces.forEach(collectInterfaces);
     }
     return interfaces;
   }
 
   /// Visits each member on the class [node] and calls [checkMember] with the
   /// corresponding instance element and AST node (for error reporting).
   ///
   /// See also [_checkTypeMembers], which is used when the class AST node is not
   /// available.
@@ skipping 164 matching lines @@
   ///     class Grandparent {
   ///         m(A a) {}
   ///     }
   ///     class Parent extends Grandparent {
   ///         m(A a) {}
   ///     }
   ///     class Test extends Parent {
   ///         m(B a) {} // invalid override
   ///     }
   void _checkSuperOverrides(Declaration node, ClassElement element) {
     var superclasses = _getSuperclasses(element.type);

[Jennifer Messerly, 2017/07/27 21:07:58]

this set of concrete superclasses also contains mixins recursively.

     _checkClassMembers(node, (member, loc) {
       for (var s in superclasses) {
         if (_checkConcreteMemberOverride(member, s, loc)) break;

[Jennifer Messerly, 2017/07/27 21:07:58]

in particular, we'll break out of the loop once we hit a concrete member.

I think with super mixins, we may hit an incorrect member (a member that is not
actually concrete)

       }
     });
   }
 
   /// Collects all superclasses of [type], including any mixin application
   /// classes.
   ///
   /// The search can be pruned by passing a [visitSuperclasses] function and
   /// having it return `false` for types that should not be further explored.
   Iterable<InterfaceType> _getSuperclasses(InterfaceType type,
       [bool visitSuperclasses(InterfaceType t)]) {
     var superclasses = new Set<InterfaceType>();
     visit(InterfaceType t) {
       if ((visitSuperclasses == null || visitSuperclasses(t)) &&
           superclasses.add(t)) {
         t.mixins.reversed.forEach(visit);

[Jennifer Messerly, 2017/07/27 21:07:58]

this may be the problematic location

         var s = t.superclass;
         if (s != null && !s.isObject) visit(s);
       }
     }
 
     type.mixins.reversed.forEach(visit);
     var s = type.superclass;
     if (s != null && !s.isObject) visit(s);
 
     // Make sure we record Object last, and not when we visit our mixins.
@@ skipping 16 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;
   }
 }