generic method type parameter inference

generic method type parameter inference

This replaces the existing hard-coded "pseudo generic" methods with actually generic ones.

This updates the mock SDK's as well. It does not update the real SDK, that will be in another change that should be landed before this one.

R=brianwilkerson@google.com, leafp@google.com

Committed: https://github.com/dart-lang/sdk/commit/0e6f692c80a7a208739140edb21c6d9fb9185c3b

[Jennifer Messerly, 2015-11-13 23:40:30]

Description was changed from

==========
generic method type parameter inference

This replaces the existing hard-coded "psuedo generic" methods with actually
generic ones.
==========

to

==========
generic method type parameter inference

This replaces the existing hard-coded "pseudo generic" methods with actually
generic ones.

This updates the mock SDK's only. So we'll need to add the generic method
comments if we want to land this without regressing the hard-coded methods.
==========

[Jennifer Messerly, 2015-11-13 23:40:30]

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

[Jennifer Messerly, 2015-11-13 23:40:51]

jmesserly@google.com changed reviewers:
- brianwilkerson@google.com, leafp@google.com

[Jennifer Messerly, 2015-11-18 21:44:23]

Description was changed from

==========
generic method type parameter inference

This replaces the existing hard-coded "pseudo generic" methods with actually
generic ones.

This updates the mock SDK's only. So we'll need to add the generic method
comments if we want to land this without regressing the hard-coded methods.
==========

to

==========
generic method type parameter inference

This replaces the existing hard-coded "pseudo generic" methods with actually
generic ones.

This updates the mock SDK's as well. It does not update the real SDK, that will
be in another change that should be landed before this one.
==========

[Jennifer Messerly, 2015-11-18 21:46:26]

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

[Jennifer Messerly, 2015-11-18 21:46:26]

Here's some basic upwards inference for generic method type parameters, intended
to replace our existing hard-coded logic.

Note: we'll need to add the generic method comments to the SDK, if we want to
land this without regressing the methods that were previously hard-coded.

Also, I didn't want to move the code around more, but let me know if I should
include renames for "psuedo" and "quasi" generic in tests. Otherwise I can do
that in a follow up.

[Jennifer Messerly, 2015-11-18 22:11:48]

https://codereview.chromium.org/1442883003/diff/60001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1442883003/diff/60001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:122: inferred =
inferred.substitute2(inferredTypes, fnTypeParams);
oops, this won't work in some cases. Working on a fix, with a better comment &
more tests.

[Jennifer Messerly, 2015-11-18 23:04:43]

PTAL. New patch set uploaded, with some new tests like recursively bounded
quantification.

[Brian Wilkerson, 2015-11-19 00:10:17]

LGTM

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1702: ts is
StrongTypeSystemImpl) {
Is this test here to determine whether we're running in strong mode? If so, it
would be better to use the field _strongMode instead.

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:27: _TypeParameterBounds
_typeParamBounds;
Storing temporary state in an instance field seems like an error-prone approach.
Are we sure that we will never need to invoke inferCallFromArguments in the
process of invoking inferCallFromArguments?

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/test/src/t...
File pkg/analyzer/test/src/task/strong/strong_test_helper.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/test/src/t...
pkg/analyzer/test/src/task/strong/strong_test_helper.dart:373: //
TODO(jmesserly): can we reuse the same mock SDK as Analyzer tests?
I certainly hope so! We control both.

[Jennifer Messerly, 2015-11-19 00:25:26]

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:27: _TypeParameterBounds
_typeParamBounds;
On 2015/11/19 00:10:17, Brian Wilkerson wrote:
> Storing temporary state in an instance field seems like an error-prone
approach.

Yeah, I can change it to pass it down if y'all prefer. It just seemed like a lot
of churn to pass it everywhere (there's a lot of recursion within the *subtypeOf
functions).

Maybe an alternative is to move the *subtypeOf functions to a class, which would
also provide a place to store the "visited" set. 

> Are we sure that we will never need to invoke inferCallFromArguments in the
> process of invoking inferCallFromArguments?

I think the question is, would we ever invoke inferCallFromArguments from
"isSubtypeOf" ... I certainly hope that is never the case. :)

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:89: // To decide on which type
to assign, we use the following heuristic.
Leaf pointed out that the code doesn't implement this anymore.

At one point I had code for this (probably still there in earlier patch sets),
but it didn't seem to be used in any tests, so I got rid of it. But there is a
test case where it matters (parameter type of returned function). Fixing...

[Jennifer Messerly, 2015-11-19 01:28:31]

PTAL. Now handles the return type heuristic we had intended (includes some
examples).

Wasn't 100% sure what to do when the return type is:

    T -> T

I picked upper bound, but easy to switch to lower.

[Jennifer Messerly, 2015-11-19 01:36:39]

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:27: _TypeParameterBounds
_typeParamBounds;
I have an idea how to fix this. Working on it.

[Jennifer Messerly, 2015-11-19 01:43:26]

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/type_system.dart:27: _TypeParameterBounds
_typeParamBounds;
On 2015/11/19 01:36:39, John Messerly wrote:
> I have an idea how to fix this. Working on it.

PTAL, I fixed this by creating a subclass that tracks constraints when we are
inferring. It's not perfect but definitely feels a lot cleaner. Thanks for
catching this.

[Brian Wilkerson, 2015-11-19 15:04:55]

LGTM

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1702: ts is
StrongTypeSystemImpl) {
I'll ask again: Is this test here to determine whether we're running in strong
mode? If so, it would be better to use the field _strongMode instead. If not,
why is it necessary?

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:68: var typeBounds = new
_StrongInferenceTypeSystem(
Rename the variable, given that the type of the value has been renamed?

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:565: class
_StrongInferenceTypeSystem extends StrongTypeSystemImpl {
I think this is much cleaner, thanks!

[Jennifer Messerly, 2015-11-19 18:12:47]

oops, sorry I forgot to reply to that comment :)

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1702: ts is
StrongTypeSystemImpl) {
On 2015/11/19 15:04:55, Brian Wilkerson wrote:
> I'll ask again: Is this test here to determine whether we're running in strong
> mode? If so, it would be better to use the field _strongMode instead. If not,
> why is it necessary?

inferCallFromArguments only exists on StrongTypeSystemImpl.

[Jennifer Messerly, 2015-11-19 18:15:04]

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:68: var typeBounds = new
_StrongInferenceTypeSystem(
On 2015/11/19 15:04:55, Brian Wilkerson wrote:
> Rename the variable, given that the type of the value has been renamed?

yup, can do.

[Leaf, 2015-11-20 02:09:43]

This LGTM, thanks John!

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/element.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:9286: int get hashCode =>
baseElement.hashCode;
@override?  Seems to be used elsewhere this is overriden.

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:9317: bool operator ==(Object
object) =>
Was this a bug?  Why did this need to change?

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1694: * type for the
result using an ad-hoc list of psuedo-generic methods.
comment is out of date?

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:587: // upper bound is already
covers this. Nothing to do.
delete 'is'

[Jennifer Messerly, 2015-11-20 17:09:46]

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/element.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:9286: int get hashCode =>
baseElement.hashCode;
On 2015/11/20 02:09:43, Leaf wrote:
> @override?  Seems to be used elsewhere this is overriden.

Done.

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/element.dart:9317: bool operator ==(Object
object) =>
On 2015/11/20 02:09:43, Leaf wrote:
> Was this a bug?  Why did this need to change?

yeah, generally these exist on Elements, and they can be compared for equality.

The bug with ParameterMember: every time you call FunctionTypeImpl.parameters,
it will make new ones, right? Because that's how it works: compute stuff on the
fly. So every time you call that, you get a new set of parameters. Since they
don't compare equal, you can never find what position you came from.

If you have a look over in this file, around line 1718:
https://codereview.chromium.org/1442883003/diff/140001/pkg/analyzer/lib/src/g...

you will notice I'm using .indexOf to find where the corresponding parameter
came from. That's not possible without this fix here.

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/static_type_analyzer.dart (right):

https://codereview.chromium.org/1442883003/diff/80001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/static_type_analyzer.dart:1694: * type for the
result using an ad-hoc list of psuedo-generic methods.
On 2015/11/20 02:09:43, Leaf wrote:
> comment is out of date?

Done.

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
File pkg/analyzer/lib/src/generated/type_system.dart (right):

https://codereview.chromium.org/1442883003/diff/120001/pkg/analyzer/lib/src/g...
pkg/analyzer/lib/src/generated/type_system.dart:587: // upper bound is already
covers this. Nothing to do.
On 2015/11/20 02:09:43, Leaf wrote:
> delete 'is'

Done.

[Jennifer Messerly, 2015-11-24 17:34:52]

Committed patchset #10 (id:180001) manually as
0e6f692c80a7a208739140edb21c6d9fb9185c3b (presubmit successful).