Add checks for type bounds on generic methods

Add checks for type bounds on generic methods

R=scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/1601b9b26417b66c09ed41e90456bc5cea5ee168

[Brian Wilkerson, 2016-04-28 15:01:32]

brianwilkerson@google.com changed reviewers:
+ jmesserly@google.com, scheglov@google.com

[Brian Wilkerson, 2016-04-28 15:01:33]

This doesn't cover all of the cases that need to be checked (tear-offs come to
mind), but I think it's a good first step.

[Brian Wilkerson, 2016-04-28 17:08:51]

ping

[scheglov, 2016-04-28 17:14:53]

LGTM

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5715: .toList());
It looks that this expression can be extracted.

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

Consider computing it lazily.

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/static_type_warning_code_test.dart (right):

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/static_type_warning_code_test.dart:1300:
Point/*<T>*/ f/*<T extends num>*/(num/*=T*/ x, num/*=T*/ y) {
I'm not sure why we need to use Point in these tests.
We test just compatibility between the function type parameter bounds and type
arguments.
Why not just make the function "void"?

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/src/dart/...
File pkg/analyzer/test/src/dart/element/element_test.dart (right):

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/src/dart/...
pkg/analyzer/test/src/dart/element/element_test.dart:2103:
function.typeParameters = ElementFactory.typeParameters(['T']);
'U' maybe?

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/src/dart/...
pkg/analyzer/test/src/dart/element/element_test.dart:2182: method.type = new
FunctionTypeImpl(method);
Why don't we set <T> anywhere in this and the next test?

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

Nice! a few thoughts

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5660: void
_checkTypeArguments(Element element, TypeArgumentList typeArguments,
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 :) )

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5672: if (element is
FunctionTypedElement) {
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?

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5675: } else if (element is
ClassElement) {
is ClassElement possible here? (it looks like it's used for generic
methods/functions)

[Brian Wilkerson, 2016-04-28 19:46:08]

Description was changed from

==========
Add checks for type bounds on generic methods
==========

to

==========
Add checks for type bounds on generic methods

R=scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1601b9b26417b66c09ed41e90456bc5cea5ee168
==========

[Brian Wilkerson, 2016-04-28 19:46:09]

Committed patchset #1 (id:1) manually as
1601b9b26417b66c09ed41e90456bc5cea5ee168 (presubmit successful).

[Brian Wilkerson, 2016-04-28 20:46:12]

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/error_verifier.dart (right):

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5660: void
_checkTypeArguments(Element element, TypeArgumentList typeArguments,
> FunctionExpressionInvocation and MethodInvocation both implement
> InvocationExpression ...

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

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5672: if (element is
FunctionTypedElement) {
Not sure. I'll look into that as part of the next CL.

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5675: } else if (element is
ClassElement) {
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.

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/error_verifier.dart:5715: .toList());
Done

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/static_type_warning_code_test.dart (right):

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/static_type_warning_code_test.dart:1300:
Point/*<T>*/ f/*<T extends num>*/(num/*=T*/ x, num/*=T*/ y) {
I just copied the test without thinking about it too hard. Unless you object,
though, I'm going to just leave it as is.

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/src/dart/...
File pkg/analyzer/test/src/dart/element/element_test.dart (right):

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/src/dart/...
pkg/analyzer/test/src/dart/element/element_test.dart:2103:
function.typeParameters = ElementFactory.typeParameters(['T']);
Done

https://codereview.chromium.org/1927103002/diff/1/pkg/analyzer/test/src/dart/...
pkg/analyzer/test/src/dart/element/element_test.dart:2182: method.type = new
FunctionTypeImpl(method);
Oversight. Done.