fix #29426, class type alias was missing checks

pkg/analyzer/lib/src/generated/error_verifier.dart

 // Copyright (c) 2014, 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 analyzer.src.generated.error_verifier;
 
 import 'dart:collection';
 import "dart:math" as math;
 
 import 'package:analyzer/dart/ast/ast.dart';
@@ skipping 445 matching lines @@
     ClassElementImpl outerClass = _enclosingClass;
     try {
       _isInNativeClass = node.nativeClause != null;
       _enclosingClass = AbstractClassElementImpl.getImpl(node.element);
       _checkDuplicateClassMembers(node);
       _checkForBuiltInIdentifierAsName(
           node.name, CompileTimeErrorCode.BUILT_IN_IDENTIFIER_AS_TYPE_NAME);
       _checkForMemberWithClassName();
       _checkForNoDefaultSuperConstructorImplicit(node);
       _checkForConflictingTypeVariableErrorCodes(node);
-      ExtendsClause extendsClause = node.extendsClause;
+      TypeName superclass = node.extendsClause?.superclass;
       ImplementsClause implementsClause = node.implementsClause;
       WithClause withClause = node.withClause;
+
       // Only do error checks on the clause nodes if there is a non-null clause
       if (implementsClause != null ||
-          extendsClause != null ||
+          superclass != null ||
           withClause != null) {
-        // Only check for all of the inheritance logic around clauses if there
-        // isn't an error code such as "Cannot extend double" already on the
-        // class.
-        if (!_checkForImplementsDisallowedClass(implementsClause) &&
-            !_checkForExtendsDisallowedClass(extendsClause) &&
-            !_checkForAllMixinErrorCodes(withClause)) {
-          _checkForExtendsDeferredClass(extendsClause);
-          _checkForImplementsDeferredClass(implementsClause);
-          _checkForNonAbstractClassInheritsAbstractMember(node.name);
-          _checkForInconsistentMethodInheritance();
-          _checkForRecursiveInterfaceInheritance(_enclosingClass);
-          _checkForConflictingGetterAndMethod();
-          _checkForConflictingInstanceGetterAndSuperclassMember();
-          _checkImplementsSuperClass(node);
-          _checkImplementsFunctionWithoutCall(node);
-          _checkForMixinHasNoConstructors(node);
-          if (_options.strongMode) {
-            _checkForMixinWithConflictingPrivateMember(node);
-          }
-        }
+        _checkClassInheritance(node, superclass, withClause, implementsClause);

[Jennifer Messerly, 2017/04/24 21:41:02]

... a bit of postmortem culture going on here :)

Rather than just fix the problem, take steps to make it less likely to happen in
the future. By factoring out a common method there's now a shared place for the
shared checks.

       }
       visitClassDeclarationIncrementally(node);
       _checkForFinalNotInitializedInClass(node);
       _checkForDuplicateDefinitionInheritance();
       _checkForConflictingInstanceMethodSetter(node);
       _checkForBadFunctionUse(node);
       return super.visitClassDeclaration(node);
     } finally {
       _isInNativeClass = false;
       _initialFieldElementsMap = null;
@@ skipping 23 matching lines @@
     }
   }
 
   @override
   Object visitClassTypeAlias(ClassTypeAlias node) {
     _checkForBuiltInIdentifierAsName(
         node.name, CompileTimeErrorCode.BUILT_IN_IDENTIFIER_AS_TYPEDEF_NAME);
     ClassElementImpl outerClassElement = _enclosingClass;
     try {
       _enclosingClass = AbstractClassElementImpl.getImpl(node.element);
-      ImplementsClause implementsClause = node.implementsClause;
-      // Only check for all of the inheritance logic around clauses if there
-      // isn't an error code such as "Cannot extend double" already on the
-      // class.
-      if (!_checkForExtendsDisallowedClassInTypeAlias(node) &&
-          !_checkForImplementsDisallowedClass(implementsClause) &&
-          !_checkForAllMixinErrorCodes(node.withClause)) {
-        _checkForExtendsDeferredClassInTypeAlias(node);

[Jennifer Messerly, 2017/04/24 21:41:02]

I also attempted to add tests for all the checks that were not present in this
code path.

-        _checkForImplementsDeferredClass(implementsClause);
-        _checkForRecursiveInterfaceInheritance(_enclosingClass);
-        _checkForNonAbstractClassInheritsAbstractMember(node.name);
-        _checkForMixinHasNoConstructors(node);
-      }
+      _checkClassInheritance(
+          node, node.superclass, node.withClause, node.implementsClause);
     } finally {
       _enclosingClass = outerClassElement;
     }
     return super.visitClassTypeAlias(node);
   }
 
   @override
   Object visitComment(Comment node) {
     _isInComment = true;
     try {
@@ skipping 133 matching lines @@
         _checkForReturnOfInvalidType(node.expression, expectedReturnType);
       }
       return super.visitExpressionFunctionBody(node);
     } finally {
       _inAsync = wasInAsync;
       _inGenerator = wasInGenerator;
     }
   }
 
   @override
-  Object visitExtendsClause(ExtendsClause node) {
-    _checkForImplicitDynamicType(node.superclass);

[Jennifer Messerly, 2017/04/24 21:41:02]

done in _checkClassInheritance now

-    return super.visitExtendsClause(node);
-  }
-
-  @override
   Object visitFieldDeclaration(FieldDeclaration node) {
     _isInStaticVariableDeclaration = node.isStatic;
     _isInInstanceVariableDeclaration = !_isInStaticVariableDeclaration;
     if (_isInInstanceVariableDeclaration) {
       VariableDeclarationList variables = node.fields;
       if (variables.isConst) {
         _errorReporter.reportErrorForToken(
             CompileTimeErrorCode.CONST_INSTANCE_FIELD, variables.keyword);
       }
     }
@@ skipping 585 matching lines @@
         errorCode = CompileTimeErrorCode.YIELD_EACH_IN_NON_GENERATOR;
       } else {
         errorCode = CompileTimeErrorCode.YIELD_IN_NON_GENERATOR;
       }
       _errorReporter.reportErrorForNode(errorCode, node);
     }
     return super.visitYieldStatement(node);
   }
 
   /**
+   * Checks the class for problems with the superclass, mixins, or implemented
+   * interfaces.
+   */
+  void _checkClassInheritance(
+      NamedCompilationUnitMember node,
+      TypeName superclass,
+      WithClause withClause,
+      ImplementsClause implementsClause) {
+    // Only check for all of the inheritance logic around clauses if there
+    // isn't an error code such as "Cannot extend double" already on the
+    // class.
+    if (!_checkForExtendsDisallowedClass(superclass) &&
+        !_checkForImplementsDisallowedClass(implementsClause) &&
+        !_checkForAllMixinErrorCodes(withClause)) {
+      _checkForImplicitDynamicType(superclass);
+      _checkForExtendsDeferredClass(superclass);
+      _checkForImplementsDeferredClass(implementsClause);
+      _checkForNonAbstractClassInheritsAbstractMember(node.name);
+      _checkForInconsistentMethodInheritance();
+      _checkForRecursiveInterfaceInheritance(_enclosingClass);
+      _checkForConflictingGetterAndMethod();
+      _checkForConflictingInstanceGetterAndSuperclassMember();
+      _checkImplementsSuperClass(implementsClause);
+      _checkImplementsFunctionWithoutCall(node.name);
+      _checkForMixinHasNoConstructors(node);
+
+      if (_options.strongMode) {
+        _checkForMixinWithConflictingPrivateMember(withClause, superclass);
+      }
+    }
+  }
+
+  /**
    * Given a list of [directives] that have the same prefix, generate an error
    * if there is more than one import and any of those imports is deferred.
    *
    * See [CompileTimeErrorCode.SHARED_DEFERRED_PREFIX].
    */
   void _checkDeferredPrefixCollision(List<ImportDirective> directives) {
     int count = directives.length;
     if (count > 1) {
       for (int i = 0; i < count; i++) {
         Token deferredToken = directives[i].deferredKeyword;
@@ skipping 2259 matching lines @@
         CompileTimeErrorCode.EXPORT_INTERNAL_LIBRARY,
         directive,
         [directive.uri]);
   }
 
   /**
    * Verify that the given extends [clause] does not extend a deferred class.
    *
    * See [CompileTimeErrorCode.EXTENDS_DEFERRED_CLASS].
    */
-  void _checkForExtendsDeferredClass(ExtendsClause clause) {
-    if (clause == null) {
+  void _checkForExtendsDeferredClass(TypeName superclass) {
+    if (superclass == null) {
       return;
     }
     _checkForExtendsOrImplementsDeferredClass(
-        clause.superclass, CompileTimeErrorCode.EXTENDS_DEFERRED_CLASS);
+        superclass, CompileTimeErrorCode.EXTENDS_DEFERRED_CLASS);
   }
 
   /**
-   * Verify that the given type [alias] does not extend a deferred class.
-   *
-   * See [CompileTimeErrorCode.EXTENDS_DISALLOWED_CLASS].
-   */
-  void _checkForExtendsDeferredClassInTypeAlias(ClassTypeAlias alias) {
-    if (alias == null) {
-      return;
-    }
-    _checkForExtendsOrImplementsDeferredClass(
-        alias.superclass, CompileTimeErrorCode.EXTENDS_DEFERRED_CLASS);
-  }
-
-  /**
    * Verify that the given extends [clause] does not extend classes such as
    * 'num' or 'String'.
    *
    * See [CompileTimeErrorCode.EXTENDS_DISALLOWED_CLASS].
    */
-  bool _checkForExtendsDisallowedClass(ExtendsClause clause) {
-    if (clause == null) {
+  bool _checkForExtendsDisallowedClass(TypeName superclass) {
+    if (superclass == null) {
       return false;
     }
     return _checkForExtendsOrImplementsDisallowedClass(
-        clause.superclass, CompileTimeErrorCode.EXTENDS_DISALLOWED_CLASS);
+        superclass, CompileTimeErrorCode.EXTENDS_DISALLOWED_CLASS);
   }
 
   /**
-   * Verify that the given type [alias] does not extend classes such as 'num' or
-   * 'String'.
-   *
-   * See [CompileTimeErrorCode.EXTENDS_DISALLOWED_CLASS].
-   */
-  bool _checkForExtendsDisallowedClassInTypeAlias(ClassTypeAlias alias) {
-    if (alias == null) {
-      return false;
-    }
-    return _checkForExtendsOrImplementsDisallowedClass(
-        alias.superclass, CompileTimeErrorCode.EXTENDS_DISALLOWED_CLASS);
-  }
-
-  /**
    * Verify that the given [typeName] does not extend, implement or mixin
    * classes that are deferred.
    *
    * See [_checkForExtendsDeferredClass],
    * [_checkForExtendsDeferredClassInTypeAlias],
    * [_checkForImplementsDeferredClass],
    * [_checkForAllMixinErrorCodes],
    * [CompileTimeErrorCode.EXTENDS_DEFERRED_CLASS],
    * [CompileTimeErrorCode.IMPLEMENTS_DEFERRED_CLASS], and
    * [CompileTimeErrorCode.MIXIN_DEFERRED_CLASS].
@@ skipping 1237 matching lines @@
           [mixinElement.name]);
     }
     return false;
   }
 
   /**
    * Check for the declaration of a mixin from a library other than the current
    * library that defines a private member that conflicts with a private name
    * from the same library but from a superclass or a different mixin.
    */
-  void _checkForMixinWithConflictingPrivateMember(ClassDeclaration node) {
-    WithClause withClause = node.withClause;
+  void _checkForMixinWithConflictingPrivateMember(
+      WithClause withClause, TypeName superclassName) {
     if (withClause == null) {
       return;
     }
-    DartType declaredSupertype = node.extendsClause?.superclass?.type;
+    DartType declaredSupertype = superclassName?.type;
     if (declaredSupertype is! InterfaceType) {
       return;
     }
     InterfaceType superclass = declaredSupertype;
     Map<LibraryElement, Map<String, String>> mixedInNames =
         <LibraryElement, Map<String, String>>{};
 
     /**
      * Report an error and return `true` if the given [name] is a private name
      * (which is defined in the given [library]) and it conflicts with another
@@ skipping 1272 matching lines @@
       }
     }
   }
 
   /**
    * Verify that if the given class [declaration] implements the class Function
    * that it has a concrete implementation of the call method.
    *
    * See [StaticWarningCode.FUNCTION_WITHOUT_CALL].
    */
-  void _checkImplementsFunctionWithoutCall(ClassDeclaration declaration) {
-    if (declaration.isAbstract) {
-      return;
-    }
-    ClassElement classElement = declaration.element;
+  void _checkImplementsFunctionWithoutCall(AstNode className) {
+    ClassElement classElement = _enclosingClass;
     if (classElement == null) {
       return;
     }
+    if (classElement.isAbstract) {
+      return;
+    }
     if (!_typeSystem.isSubtypeOf(
         classElement.type, _typeProvider.functionType)) {
       return;
     }
     // If there is a noSuchMethod method, then don't report the warning,
     // see dartbug.com/16078
     if (_enclosingClass.hasNoSuchMethod) {
       return;
     }
     ExecutableElement callMethod = _inheritanceManager.lookupMember(
         classElement, FunctionElement.CALL_METHOD_NAME);
     if (callMethod == null ||
         callMethod is! MethodElement ||
         (callMethod as MethodElement).isAbstract) {
       _errorReporter.reportErrorForNode(
-          StaticWarningCode.FUNCTION_WITHOUT_CALL, declaration.name);
+          StaticWarningCode.FUNCTION_WITHOUT_CALL, className);
     }
   }
 
   /**
    * Verify that the given class [declaration] does not have the same class in
    * the 'extends' and 'implements' clauses.
    *
    * See [CompileTimeErrorCode.IMPLEMENTS_SUPER_CLASS].
    */
-  void _checkImplementsSuperClass(ClassDeclaration declaration) {
+  void _checkImplementsSuperClass(ImplementsClause implementsClause) {
     // prepare super type
     InterfaceType superType = _enclosingClass.supertype;
     if (superType == null) {
       return;
     }
     // prepare interfaces
-    ImplementsClause implementsClause = declaration.implementsClause;
     if (implementsClause == null) {
       return;
     }
     // check interfaces
     for (TypeName interfaceNode in implementsClause.interfaces) {
       if (interfaceNode.type == superType) {
         _errorReporter.reportErrorForNode(
             CompileTimeErrorCode.IMPLEMENTS_SUPER_CLASS,
             interfaceNode,
             [superType.displayName]);
@@ skipping 911 matching lines @@
 class _InvocationCollector extends RecursiveAstVisitor {
   final List<String> superCalls = <String>[];
 
   @override
   visitMethodInvocation(MethodInvocation node) {
     if (node.target is SuperExpression) {
       superCalls.add(node.methodName.name);
     }
   }
 }