Add checks for type bounds on generic methods

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 752 matching lines @@
   }
 
   @override
   Object visitFunctionExpressionInvocation(FunctionExpressionInvocation node) {
     Expression functionExpression = node.function;
     DartType expressionType = functionExpression.staticType;
     if (!_isFunctionType(expressionType)) {
       _errorReporter.reportErrorForNode(
           StaticTypeWarningCode.INVOCATION_OF_NON_FUNCTION_EXPRESSION,
           functionExpression);
+    } else if (expressionType is FunctionType) {
+      _checkTypeArguments(expressionType.element, node.typeArguments);
     }
     return super.visitFunctionExpressionInvocation(node);
   }
 
   @override
   Object visitFunctionTypeAlias(FunctionTypeAlias node) {
     _checkForBuiltInIdentifierAsName(
         node.name, CompileTimeErrorCode.BUILT_IN_IDENTIFIER_AS_TYPEDEF_NAME);
     _checkForDefaultValueInFunctionTypeAlias(node);
     _checkForTypeAliasCannotReferenceItself_function(node);
@@ skipping 153 matching lines @@
     SimpleIdentifier methodName = node.methodName;
     if (target != null) {
       ClassElement typeReference = ElementResolver.getTypeReference(target);
       _checkForStaticAccessToInstanceMember(typeReference, methodName);
       _checkForInstanceAccessToStaticMember(typeReference, methodName);
     } else {
       _checkForUnqualifiedReferenceToNonLocalStaticMember(methodName);
     }
     _checkForMissingRequiredParam(
         node.staticInvokeType, node.argumentList, methodName);
+    _checkTypeArguments(
+        node.methodName.staticElement, node.typeArguments, target?.staticType);
     return super.visitMethodInvocation(node);
   }
 
   @override
   Object visitNativeClause(NativeClause node) {
     // TODO(brianwilkerson) Figure out the right rule for when 'native' is
     // allowed.
     if (!_isInSystemLibrary) {
       _errorReporter.reportErrorForNode(
           ParserErrorCode.NATIVE_CLAUSE_IN_NON_SDK_CODE, node);
@@ skipping 4254 matching lines @@
     if (element is ClassElement) {
       // prepare type parameters
       List<DartType> typeParameters = element.type.typeArguments;
       List<TypeParameterElement> boundingElts = element.typeParameters;
       // iterate over each bounded type parameter and corresponding argument
       NodeList<TypeName> typeNameArgList = typeName.typeArguments.arguments;
       List<DartType> typeArguments = (type as InterfaceType).typeArguments;
       int loopThroughIndex =
           math.min(typeNameArgList.length, boundingElts.length);
 
+      bool shouldSubstitute = typeArguments.length != 0 &&
+          typeArguments.length == typeParameters.length;
       for (int i = 0; i < loopThroughIndex; i++) {
         TypeName argTypeName = typeNameArgList[i];
         DartType argType = argTypeName.type;
         DartType boundType = boundingElts[i].bound;
         if (argType != null && boundType != null) {
-          if (typeArguments.length != 0 &&
-              typeArguments.length == typeParameters.length) {
+          if (shouldSubstitute) {
             boundType = boundType.substitute2(typeArguments, typeParameters);
           }
           if (!_typeSystem.isSubtypeOf(argType, boundType)) {
             ErrorCode errorCode;
             if (_isInConstInstanceCreation) {
               errorCode =
                   CompileTimeErrorCode.TYPE_ARGUMENT_NOT_MATCHING_BOUNDS;
             } else {
               errorCode =
                   StaticTypeWarningCode.TYPE_ARGUMENT_NOT_MATCHING_BOUNDS;
@@ skipping 402 matching lines @@
     for (TypeName interfaceNode in implementsClause.interfaces) {
       if (interfaceNode.type == superType) {
         _errorReporter.reportErrorForNode(
             CompileTimeErrorCode.IMPLEMENTS_SUPER_CLASS,
             interfaceNode,
             [superType.displayName]);
       }
     }
   }
 
+  /**
+   * Verify that the given [typeArguments] are all within their bounds, as
+   * defined by the given [element].
+   *
+   * See [StaticTypeWarningCode.TYPE_ARGUMENT_NOT_MATCHING_BOUNDS].
+   */
+  void _checkTypeArguments(Element element, TypeArgumentList typeArguments,

[Jennifer Messerly, 2016/04/28 17:30:02]

one idea here: FunctionExpressionInvocation and MethodInvocation both implement
InvocationExpression, which would allow passing in the "node" and  having it be
like:

void _checkTypeArguments(InvocationExpression node) {
  Element element = node.function.staticElement;
  TypeArgumentList typeArguments = node.typeArguments;
  ... existing body ...
}

(I just rediscovered this trick yesterday working on generic methods in DDC. Was
quite pleased how much code I could share between FunctionExprInvocation and
MethodInvocation :) )

[Brian Wilkerson, 2016/04/28 20:46:11]

Yes, but I was hoping to use the same method for tear-offs, which don't
implement InvocationExpression.

+      [DartType targetType]) {
+    if (element == null || typeArguments == null) {
+      return;
+    }
+    void reportError(
+        TypeName argument, DartType argumentType, DartType parameterType) {
+      _errorReporter.reportTypeErrorForNode(
+          StaticTypeWarningCode.TYPE_ARGUMENT_NOT_MATCHING_BOUNDS,
+          argument,
+          [argumentType, parameterType]);
+    }
+    if (element is FunctionTypedElement) {

[Jennifer Messerly, 2016/04/28 17:30:02]

Would it make sense to use the type instead of the element? e.g.

DartType invokeType = node.function.staticType;
if (invokeType is FunctionType && invokeType.typeFormals.isNotEmpty) {
  // check type args match bounds
}

that way it works for any kind of element, since the element-to-staticType step
was already handled by element_resolver (_getInvokeType)

BTW, there is code already to check for number of type arguments passed:
https://github.com/dart-lang/sdk/blob/ee8bb2cea93b9b6e87d8c9309e2f22b46331864...

Not sure if it would make sense to consolidate?

[Brian Wilkerson, 2016/04/28 20:46:11]

Not sure. I'll look into that as part of the next CL.

+      _checkTypeArgumentsAgainstBounds(
+          element.typeParameters, typeArguments, targetType, reportError);
+    } else if (element is ClassElement) {

[Jennifer Messerly, 2016/04/28 17:30:02]

is ClassElement possible here? (it looks like it's used for generic
methods/functions)

[Brian Wilkerson, 2016/04/28 20:46:11]

Sorry, you're seeing an incomplete thought.

It isn't possible yet, but I want to convert
_checkForTypeArgumentNotMatchingBounds to share this code, at which point it
will be possible.

+      _checkTypeArgumentsAgainstBounds(
+          element.typeParameters, typeArguments, targetType, reportError);
+    } else if (element is ParameterElement || element is LocalVariableElement) {
+      // TODO(brianwilkerson) Implement this case
+    } else {
+      print('Unhandled element type: ${element.runtimeType}');
+    }
+  }
+
+  void _checkTypeArgumentsAgainstBounds(
+      List<TypeParameterElement> typeParameters,
+      TypeArgumentList typeArgumentList,
+      DartType targetType,
+      void reportError(
+          TypeName argument, DartType argumentType, DartType parameterType)) {
+    NodeList<TypeName> typeArguments = typeArgumentList.arguments;
+    int argumentsLength = typeArguments.length;
+    int maxIndex = math.min(typeParameters.length, argumentsLength);
+
+    bool shouldSubstitute =
+        argumentsLength != 0 && argumentsLength == typeParameters.length;
+    List<DartType> argumentTypes = shouldSubstitute
+        ? typeArguments.map((TypeName typeName) => typeName.type).toList()
+        : null;
+    List<DartType> parameterTypes = shouldSubstitute
+        ? typeParameters
+            .map((TypeParameterElement element) => element.type)
+            .toList()
+        : null;
+    for (int i = 0; i < maxIndex; i++) {
+      TypeName argTypeName = typeArguments[i];
+      DartType argType = argTypeName.type;
+      DartType boundType = typeParameters[i].bound;
+      if (argType != null && boundType != null) {
+        if (targetType is ParameterizedType) {
+          boundType = boundType.substitute2(
+              targetType.typeArguments,
+              targetType.typeParameters
+                  .map((TypeParameterElement element) => element.type)
+                  .toList());

[scheglov, 2016/04/28 17:14:52]

It looks that this expression can be extracted.

targetType.typeParameters
                  .map((TypeParameterElement element) => element.type)
                   .toList()

Consider computing it lazily.

[Brian Wilkerson, 2016/04/28 20:46:11]

Done

+        }
+        if (shouldSubstitute) {
+          boundType = boundType.substitute2(argumentTypes, parameterTypes);
+        }
+        if (!_typeSystem.isSubtypeOf(argType, boundType)) {
+          reportError(argTypeName, argType, boundType);
+        }
+      }
+    }
+  }
+
   DartType _computeReturnTypeForMethod(Expression returnExpression) {
     // This method should never be called for generators, since generators are
     // never allowed to contain return statements with expressions.
     assert(!_inGenerator);
     if (returnExpression == null) {
       if (_enclosingFunction.isAsynchronous) {
         return _typeProvider.futureNullType;
       } else {
         return VoidTypeImpl.instance;
       }
@@ skipping 479 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);
     }
   }
 }